Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2365373002:
Add autothread to pseudo-tcp fuzzer. (Closed)
DescriptionAdd autothread to pseudo-tcp fuzzer.
I think this will make a rtc::Thread object exist for the lifetime of
the environment, which will remove some uninteresting crashes.
BUG=chrome:648075
Committed: https://crrev.com/590cf281fb3def59d6f639813691def18ab55370
Cr-Commit-Position: refs/heads/master@{#14438}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rewrite to use init list for struct #
Total comments: 4
Patch Set 3 : Added const, comments for thread. #Messages
Total messages: 16 (7 generated)
Description was changed from ========== Add autothread to pseudo-tcp fuzzer. I think this will make a rtc::Thread object exist for the lifetime of the environment, which will remove some uninteresting crashes. BUG=648075 ========== to ========== Add autothread to pseudo-tcp fuzzer. I think this will make a rtc::Thread object exist for the lifetime of the environment, which will remove some uninteresting crashes. BUG=chrome:648075 ==========
phoglund@webrtc.org changed reviewers: + sergeyu@chromium.org
lgtm
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/2365373002/diff/1/webrtc/test/fuzzers/pseudotcp... File webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc (right): https://codereview.webrtc.org/2365373002/diff/1/webrtc/test/fuzzers/pseudotcp... webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc:33: cricket::PseudoTcp* ptcp; Move struct members down to after the constructor, and use init lists if possible. https://codereview.webrtc.org/2365373002/diff/1/webrtc/test/fuzzers/pseudotcp... webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc:44: env->ptcp->NotifyPacket(reinterpret_cast<const char*>(data), size); Could the environment here including FakeIPseudo.. just be stack allocated?
pbos PTAL https://codereview.webrtc.org/2365373002/diff/1/webrtc/test/fuzzers/pseudotcp... File webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc (right): https://codereview.webrtc.org/2365373002/diff/1/webrtc/test/fuzzers/pseudotcp... webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc:33: cricket::PseudoTcp* ptcp; On 2016/09/27 19:43:49, pbos-webrtc wrote: > Move struct members down to after the constructor, and use init lists if > possible. Done. https://codereview.webrtc.org/2365373002/diff/1/webrtc/test/fuzzers/pseudotcp... webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc:44: env->ptcp->NotifyPacket(reinterpret_cast<const char*>(data), size); On 2016/09/27 19:43:49, pbos-webrtc wrote: > Could the environment here including FakeIPseudo.. just be stack allocated? No, it core dumps. I didn't investigate further why it does that. It's apparently fine to leak some memory in a fuzzer program, so I think we should just leave it as is. Also, if there's any state in ptcp, we'll be accumulating state over many calls to FuzzOneInput, and I think that's a good thing since we can reach more code. With stack alloc ptcp gets constructed and destructed once per call.
lgtm w/ nits https://codereview.webrtc.org/2365373002/diff/20001/webrtc/test/fuzzers/pseud... File webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc (right): https://codereview.webrtc.org/2365373002/diff/20001/webrtc/test/fuzzers/pseud... webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc:37: cricket::PseudoTcp* ptcp; * const ptcp; https://codereview.webrtc.org/2365373002/diff/20001/webrtc/test/fuzzers/pseud... webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc:38: rtc::AutoThread thread; Please put a comment on why the thread is necessary here.
https://codereview.webrtc.org/2365373002/diff/20001/webrtc/test/fuzzers/pseud... File webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc (right): https://codereview.webrtc.org/2365373002/diff/20001/webrtc/test/fuzzers/pseud... webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc:37: cricket::PseudoTcp* ptcp; On 2016/09/28 16:01:41, pbos-webrtc wrote: > * const ptcp; Done. https://codereview.webrtc.org/2365373002/diff/20001/webrtc/test/fuzzers/pseud... webrtc/test/fuzzers/pseudotcp_parser_fuzzer.cc:38: rtc::AutoThread thread; On 2016/09/28 16:01:41, pbos-webrtc wrote: > Please put a comment on why the thread is necessary here. Done.
The CQ bit was checked by phoglund@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2365373002/#ps40001 (title: "Added const, comments for thread.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Add autothread to pseudo-tcp fuzzer. I think this will make a rtc::Thread object exist for the lifetime of the environment, which will remove some uninteresting crashes. BUG=chrome:648075 ========== to ========== Add autothread to pseudo-tcp fuzzer. I think this will make a rtc::Thread object exist for the lifetime of the environment, which will remove some uninteresting crashes. BUG=chrome:648075 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add autothread to pseudo-tcp fuzzer. I think this will make a rtc::Thread object exist for the lifetime of the environment, which will remove some uninteresting crashes. BUG=chrome:648075 ========== to ========== Add autothread to pseudo-tcp fuzzer. I think this will make a rtc::Thread object exist for the lifetime of the environment, which will remove some uninteresting crashes. BUG=chrome:648075 Committed: https://crrev.com/590cf281fb3def59d6f639813691def18ab55370 Cr-Commit-Position: refs/heads/master@{#14438} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/590cf281fb3def59d6f639813691def18ab55370 Cr-Commit-Position: refs/heads/master@{#14438} |
