Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2232683002:
[Tracing] Minor bugs fix. (Closed)
Description[Tracing] Minor bugs fix.
1. The third parameter of strncpy should be the length of source string.
2. Value contains " should be valid.
BUG=v8:4561
Committed: https://crrev.com/375bf0eb19a31c8d084f199fe66b273ec2885d28
Cr-Commit-Position: refs/heads/master@{#38563}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Update test #
Total comments: 1
Patch Set 3 : remove end #
Messages
Total messages: 28 (17 generated)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lpy@chromium.org changed reviewers: + fmeawad@chromium.org, mattloring@google.com
PTAL.
https://codereview.chromium.org/2232683002/diff/1/src/libplatform/tracing/tra... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2232683002/diff/1/src/libplatform/tracing/tra... src/libplatform/tracing/trace-object.cc:28: strncpy(*buffer, *member, length); How does this behave when there is not enough space in the buffer to accommodate *member?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
On 2016/08/10 00:41:54, mattloring wrote: > https://codereview.chromium.org/2232683002/diff/1/src/libplatform/tracing/tra... > File src/libplatform/tracing/trace-object.cc (right): > > https://codereview.chromium.org/2232683002/diff/1/src/libplatform/tracing/tra... > src/libplatform/tracing/trace-object.cc:28: strncpy(*buffer, *member, length); > How does this behave when there is not enough space in the buffer to accommodate > *member? The buffer is allocated with the size of all string copies, so my understanding here is this won't happen.
On 2016/08/10 00:48:35, lpy wrote: > On 2016/08/10 00:41:54, mattloring wrote: > > > https://codereview.chromium.org/2232683002/diff/1/src/libplatform/tracing/tra... > > File src/libplatform/tracing/trace-object.cc (right): > > > > > https://codereview.chromium.org/2232683002/diff/1/src/libplatform/tracing/tra... > > src/libplatform/tracing/trace-object.cc:28: strncpy(*buffer, *member, length); > > How does this behave when there is not enough space in the buffer to > accommodate > > *member? > > The buffer is allocated with the size of all string copies, so my understanding > here is this won't happen. Ah ok, that makes sense. Looks good on my end.
On 2016/08/10 04:47:21, mattloring wrote: > On 2016/08/10 00:48:35, lpy wrote: > > On 2016/08/10 00:41:54, mattloring wrote: > > > > > > https://codereview.chromium.org/2232683002/diff/1/src/libplatform/tracing/tra... > > > File src/libplatform/tracing/trace-object.cc (right): > > > > > > > > > https://codereview.chromium.org/2232683002/diff/1/src/libplatform/tracing/tra... > > > src/libplatform/tracing/trace-object.cc:28: strncpy(*buffer, *member, > length); > > > How does this behave when there is not enough space in the buffer to > > accommodate > > > *member? > > > > The buffer is allocated with the size of all string copies, so my > understanding > > here is this won't happen. Ah ok, that makes sense. Looks good on my end. (with test updates)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lpy@chromium.org changed reviewers: + yangguo@chromium.org
+ Yang@ for review.
lgtm https://codereview.chromium.org/2232683002/diff/20001/src/libplatform/tracing... File src/libplatform/tracing/trace-object.cc (right): https://codereview.chromium.org/2232683002/diff/20001/src/libplatform/tracing... src/libplatform/tracing/trace-object.cc:25: const char* end) { is the last parameter still necessary?
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmeawad@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2232683002/#ps40001 (title: "remove end")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Minor bugs fix. 1. The third parameter of strncpy should be the length of source string. 2. Value contains " should be valid. BUG=v8:4561 ========== to ========== [Tracing] Minor bugs fix. 1. The third parameter of strncpy should be the length of source string. 2. Value contains " should be valid. BUG=v8:4561 Committed: https://crrev.com/375bf0eb19a31c8d084f199fe66b273ec2885d28 Cr-Commit-Position: refs/heads/master@{#38563} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/375bf0eb19a31c8d084f199fe66b273ec2885d28 Cr-Commit-Position: refs/heads/master@{#38563} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
