Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2072473002:
Update the current RTP parser fuzzer to handle header extensions. (Closed)
DescriptionUpdate the current RTP parser fuzzer to handle header extensions.
This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help.
BUG=webrtc:4771
NOTRY=true
Patch Set 1 #Patch Set 2 : Use enum value for # extensions #
Total comments: 17
Patch Set 3 : Code review changes #
Total comments: 3
Patch Set 4 : Fix nits #
Total comments: 1
Patch Set 5 : sort includes #Messages
Total messages: 33 (18 generated)
Description was changed from ========== Update the current RTP parser fuzzer to handle header extensions. Since this changes the corpus semantics (it reads one extra byte at the front and uses it as a bitmask to decide which extensions to enable), it would be good to wait until we support seed corpora for the WebRTC fuzzers. BUG=chromium:620242 ========== to ========== Update the current RTP parser fuzzer to handle header extensions. Since this changes the corpus semantics (it reads one extra byte at the front and uses it as a bitmask to decide which extensions to enable), it would be good to wait until we support seed corpora for the WebRTC fuzzers. Also waiting for a change to land to rtp_rtcp_defines.h to specify the number of header extensions. BUG=chromium:620242 ==========
Description was changed from ========== Update the current RTP parser fuzzer to handle header extensions. Since this changes the corpus semantics (it reads one extra byte at the front and uses it as a bitmask to decide which extensions to enable), it would be good to wait until we support seed corpora for the WebRTC fuzzers. Also waiting for a change to land to rtp_rtcp_defines.h to specify the number of header extensions. BUG=chromium:620242 ========== to ========== Update the current RTP parser fuzzer to handle header extensions. This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help. BUG=chromium:4771 ==========
katrielc@chromium.org changed reviewers: + pbos@webrtc.org
The fuzzer is not finding much new at the moment (https://cluster-fuzz.appspot.com/fuzzerstats?fuzzer_name=libfuzzer_rtp_packet...), so I think it makes sense to land this without a seed corpus at least for now, and if it gets stuck then we can help it along however.
pbos@webrtc.org changed reviewers: + danilchap@webrtc.org
+danilchap to verify that this is used properly https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... File webrtc/test/fuzzers/rtp_packet_fuzzer.cc (right): https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:28: RtpPacketReceived::ExtensionManager extensions; Make a compile-time assert on kRtpExtensionNumberOfExtensions being <= size of extensionMask, so that we have to update this fuzzer if we add more extensions. https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:30: // Skip i=0 which is kRtpExtensionNone i.e. not an actual extension. From zero, but skip if it's kRtpExtensionNone explicitly, instead of relying on declaration order. e.g. RTPExtensionType extension_type = static_cast<RTPExtensionType>(i); if (extension_type == kRtpExtensionNone) continue; ... https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:48: // Each extension has its own getter. Call all the ones we registered above. Should this have defined behavior on non-registered extensions? https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:51: switch (i) { cast to RTPExtensionType here instead of to int below. https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:74: // TODO(danilchap) Add this once it's written. TODO(katrielc): https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:77: break; Remove default, or assert false on it.
https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... File webrtc/test/fuzzers/rtp_packet_fuzzer.cc (right): https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:20: // We decide which header extensions to register by reading one byte The way parsing is written, registering all extensions shouldn't leave any codepath unchecked as long as we support less than 14 extensions. https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:48: // Each extension has its own getter. Call all the ones we registered above. On 2016/06/26 21:49:59, pbos-webrtc wrote: > Should this have defined behavior on non-registered extensions? Yes. both for non-registered and non-present extensions GetExtension should return false. Which mean it might be better to replace loop with set of GetExtension, ignoring 'registered' status.
katrielc@chromium.org changed reviewers: + katrielc@chromium.org - pbos@webrtc.org
https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... File webrtc/test/fuzzers/rtp_packet_fuzzer.cc (right): https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:20: // We decide which header extensions to register by reading one byte On 2016/06/27 11:06:17, danilchap wrote: > The way parsing is written, registering all extensions shouldn't leave any > codepath unchecked as long as we support less than 14 extensions. What about an out-of-bounds read that reads too far into the buffer? That's legal unless no other headers and data are present, right? https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:28: RtpPacketReceived::ExtensionManager extensions; On 2016/06/26 21:49:59, pbos-webrtc wrote: > Make a compile-time assert on kRtpExtensionNumberOfExtensions being <= size of > extensionMask, so that we have to update this fuzzer if we add more extensions. Done. https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:30: // Skip i=0 which is kRtpExtensionNone i.e. not an actual extension. On 2016/06/26 21:49:59, pbos-webrtc wrote: > From zero, but skip if it's kRtpExtensionNone explicitly, instead of relying on > declaration order. > > e.g. RTPExtensionType extension_type = static_cast<RTPExtensionType>(i); > > if (extension_type == kRtpExtensionNone) > continue; > > ... Done. https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:48: // Each extension has its own getter. Call all the ones we registered above. On 2016/06/27 11:06:17, danilchap wrote: > On 2016/06/26 21:49:59, pbos-webrtc wrote: > > Should this have defined behavior on non-registered extensions? > > Yes. both for non-registered and non-present extensions GetExtension should > return false. > Which mean it might be better to replace loop with set of GetExtension, ignoring > 'registered' status. Acknowledged. https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:51: switch (i) { On 2016/06/26 21:49:59, pbos-webrtc wrote: > cast to RTPExtensionType here instead of to int below. Done. https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:74: // TODO(danilchap) Add this once it's written. On 2016/06/26 21:49:59, pbos-webrtc wrote: > TODO(katrielc): Done, though I'm leaving in two weeks... https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:77: break; On 2016/06/26 21:49:59, pbos-webrtc wrote: > Remove default, or assert false on it. Done.
Description was changed from ========== Update the current RTP parser fuzzer to handle header extensions. This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help. BUG=chromium:4771 ========== to ========== Update the current RTP parser fuzzer to handle header extensions. This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help. BUG=webrtc:4771 ==========
lgtm % nit https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... File webrtc/test/fuzzers/rtp_packet_fuzzer.cc (right): https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:20: // We decide which header extensions to register by reading one byte On 2016/07/04 07:15:31, katrielc wrote: > On 2016/06/27 11:06:17, danilchap wrote: > > The way parsing is written, registering all extensions shouldn't leave any > > codepath unchecked as long as we support less than 14 extensions. > > What about an out-of-bounds read that reads too far into the buffer? That's > legal unless no other headers and data are present, right? I'm not sure how not registering extension can help this case: not registering extension @id=1 is same as registering same extension at @id=2 (and registering no other extension at @id=1), so with 6 extensions some ids will still be free and parser still should behave when they appear: should skip them when length provided correctly, should fail the parse when not. They only code path that is not covering is behavior of GetExtension function when extension is not registered, but that should be covered by unit test, not fuzzer. On the other side.... that's about current implementation. Future implementation might be different and may fail differently with non-registered extensions. https://codereview.webrtc.org/2072473002/diff/20001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:74: // TODO(danilchap) Add this once it's written. On 2016/07/04 07:15:31, katrielc wrote: > On 2016/06/26 21:49:59, pbos-webrtc wrote: > > TODO(katrielc): > > Done, though I'm leaving in two weeks... That doesn't matter here: TODO name the person who wrote it, not the person who would do it. https://codereview.webrtc.org/2072473002/diff/40001/webrtc/test/fuzzers/rtp_h... File webrtc/test/fuzzers/rtp_header_fuzzer.cc (right): https://codereview.webrtc.org/2072473002/diff/40001/webrtc/test/fuzzers/rtp_h... webrtc/test/fuzzers/rtp_header_fuzzer.cc:21: static_assert(kRtpExtensionNumberOfExtensions <= 1 * 8, I guess '8' is as clear as '1 * 8' here. If you want it to be more explicit, you may use constexpr: constexpr size_t kBitsInByte = 8; static_assert(... <= 1 * kBitsInByte, though even then personally I find "static_assert(... <= kBitsInByte," more clear. https://codereview.webrtc.org/2072473002/diff/40001/webrtc/test/fuzzers/rtp_p... File webrtc/test/fuzzers/rtp_packet_fuzzer.cc (right): https://codereview.webrtc.org/2072473002/diff/40001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:55: // Each extension has its own getter. Call all the ones we registered above. Update comment too since you do not check getting just registered extensions.
Description was changed from ========== Update the current RTP parser fuzzer to handle header extensions. This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help. BUG=webrtc:4771 ========== to ========== Update the current RTP parser fuzzer to handle header extensions. This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help. BUG=webrtc:4771 NOTRY=true ==========
katrielc@chromium.org changed reviewers: - katrielc@chromium.org
The CQ bit was checked by katrielc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2072473002/#ps60001 (title: "Fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6666)
katrielc@chromium.org changed reviewers: + pbos@webrtc.org
Sending out to pbos@ as sole OWNER :) https://codereview.webrtc.org/2072473002/diff/40001/webrtc/test/fuzzers/rtp_h... File webrtc/test/fuzzers/rtp_header_fuzzer.cc (right): https://codereview.webrtc.org/2072473002/diff/40001/webrtc/test/fuzzers/rtp_h... webrtc/test/fuzzers/rtp_header_fuzzer.cc:21: static_assert(kRtpExtensionNumberOfExtensions <= 1 * 8, On 2016/07/04 08:43:55, danilchap wrote: > I guess '8' is as clear as '1 * 8' here. > If you want it to be more explicit, you may use constexpr: > constexpr size_t kBitsInByte = 8; > static_assert(... <= 1 * kBitsInByte, > though even then personally I find > "static_assert(... <= kBitsInByte," more clear. Eh, I'll just write 8 :)
danilchap@webrtc.org changed reviewers: + stefan@webrtc.org - pbos@webrtc.org
noticed one more nit. https://codereview.webrtc.org/2072473002/diff/60001/webrtc/test/fuzzers/rtp_p... File webrtc/test/fuzzers/rtp_packet_fuzzer.cc (right): https://codereview.webrtc.org/2072473002/diff/60001/webrtc/test/fuzzers/rtp_p... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:12: #include "webrtc/modules/rtp_rtcp/source/rtp_header_extension.h" just noticed this includes are not in alphabetical order. They better be.
katrielc@chromium.org changed reviewers: + phoglund@webrtc.org - stefan@webrtc.org
Adding phoglund@ since you've reviewed a couple of these fuzzers already :)
lgtm
The CQ bit was checked by katrielc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2072473002/#ps80001 (title: "sort includes")
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 ========== Update the current RTP parser fuzzer to handle header extensions. This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help. BUG=webrtc:4771 NOTRY=true ========== to ========== Update the current RTP parser fuzzer to handle header extensions. This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help. BUG=webrtc:4771 NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Update the current RTP parser fuzzer to handle header extensions. This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help. BUG=webrtc:4771 NOTRY=true ========== to ========== Update the current RTP parser fuzzer to handle header extensions. This changes the corpus semantics, but libfuzzer should be smart enough to figure it out, and if not then we can add a seed_corpus to help. BUG=webrtc:4771 NOTRY=true Committed: https://crrev.com/36a321d2e32bd9d823329958a71d4eba8356522a Cr-Commit-Position: refs/heads/master@{#13384} ==========
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/36a321d2e32bd9d823329958a71d4eba8356522a Cr-Commit-Position: refs/heads/master@{#13384} |
