Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2062103002:
Add a libfuzzer for RtpHeaderParser. (Closed)
|
Created:
4 years, 6 months ago by katrielc1 Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd a libfuzzer for RtpHeaderParser.
NOTRY=true
Committed: https://crrev.com/d4bcdad26349ab9e03c065442ab7d5d13e166800
Cr-Commit-Position: refs/heads/master@{#13271}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Update per code review #
Total comments: 1
Patch Set 3 : update as per code review #
Total comments: 2
Patch Set 4 : don't use id 0 #Patch Set 5 : Use an enum value to count number of extensions #
Total comments: 1
Patch Set 6 : Remove blank line #
Messages
Total messages: 43 (20 generated)
katrielc@chromium.org changed reviewers: + katrielc@chromium.org, phoglund@webrtc.org
Not sure if adding the enum length like this is considered acceptable?
Some minor comments. https://codereview.webrtc.org/2062103002/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2062103002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:63: const int kNumberOfRtpHeaderExtensions = 7; Maybe just add this as the last entry in the enum? It will get the value 7 and automatically update when new entries are added. Have a blank line between it and the others to signfify it's a special entry. You'll have to bring in an OWNER for rtp_rtcp anyway, and they probably know better than me. https://codereview.webrtc.org/2062103002/diff/1/webrtc/test/fuzzers/rtp_heade... File webrtc/test/fuzzers/rtp_header_fuzzer.cc (right): https://codereview.webrtc.org/2062103002/diff/1/webrtc/test/fuzzers/rtp_heade... webrtc/test/fuzzers/rtp_header_fuzzer.cc:24: size--; Why is size decreased here? Because you're stepping one byte into data? It took me a fair amount of thinking to puzzle out what this code does, do maybe it can be made a bit more readable. What about if (size <= 1) return; // We decide which header extensions to register by reading one byte // from the beginning of |data| and interpreting it as a bitmask over // the RTPExtensionType enum. std::bitset<8> extensionMask(data[0]); data++; size--; ... It's one line more of code but it's a lot easier to grok we're reading the first byte and then discarding it. I don't have to reason about the difference between prefix and suffix ++. Also the comment makes more sense over that operation rather than the guard clause at the top. https://codereview.webrtc.org/2062103002/diff/1/webrtc/test/fuzzers/rtp_heade... webrtc/test/fuzzers/rtp_header_fuzzer.cc:29: if (extensionMask[i]) Nit: use { } since body is more than one line (even if one is a comment)
https://codereview.webrtc.org/2062103002/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2062103002/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:63: const int kNumberOfRtpHeaderExtensions = 7; The problem with adding it to the enum is that it's then a valid enum value, which you don't want. (For example, you'd have to change all the switches in the rest of the code to handle it.) https://codereview.webrtc.org/2062103002/diff/1/webrtc/test/fuzzers/rtp_heade... File webrtc/test/fuzzers/rtp_header_fuzzer.cc (right): https://codereview.webrtc.org/2062103002/diff/1/webrtc/test/fuzzers/rtp_heade... webrtc/test/fuzzers/rtp_header_fuzzer.cc:24: size--; On 2016/06/15 08:20:25, phoglund wrote: > Why is size decreased here? Because you're stepping one byte into data? > > It took me a fair amount of thinking to puzzle out what this code does, do maybe > it can be made a bit more readable. What about > > if (size <= 1) > return; > > // We decide which header extensions to register by reading one byte > // from the beginning of |data| and interpreting it as a bitmask over > // the RTPExtensionType enum. > std::bitset<8> extensionMask(data[0]); > data++; > size--; > > ... > > It's one line more of code but it's a lot easier to grok we're reading > the first byte and then discarding it. I don't have to reason about the > difference between prefix and suffix ++. Also the comment makes more > sense over that operation rather than the guard clause at the top. Done. https://codereview.webrtc.org/2062103002/diff/1/webrtc/test/fuzzers/rtp_heade... webrtc/test/fuzzers/rtp_header_fuzzer.cc:29: if (extensionMask[i]) On 2016/06/15 08:20:25, phoglund wrote: > Nit: use { } since body is more than one line (even if one is a comment) Done.
Description was changed from ========== Add a libfuzzer for RtpHeaderParser. ========== to ========== Add a libfuzzer for RtpHeaderParser. stefan: I changed rtp_rtcp_defines.h to add a count, is that OK? ==========
katrielc@chromium.org changed reviewers: + stefan@webrtc.org - katrielc@chromium.org
danilchap@webrtc.org changed reviewers: + katrielc@chromium.org - stefan@webrtc.org
https://codereview.webrtc.org/2062103002/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2062103002/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:63: const int kNumberOfRtpHeaderExtensions = 7; make it constexpr, and it should be 6 (there are 6 supported extensions now: None is not an extension)
On 2016/06/15 09:18:14, danilchap wrote: > make it constexpr, and it should be 6 (there are 6 supported extensions now: None is not an extension) If kRtpExtensionNone is not an extension, should it be valid to register it in the parser? (It currently is.)
Description was changed from ========== Add a libfuzzer for RtpHeaderParser. stefan: I changed rtp_rtcp_defines.h to add a count, is that OK? ========== to ========== Add a libfuzzer for RtpHeaderParser. henrik.lundin: I changed rtp_rtcp_defines.h to add a count, is that OK? ==========
katrielc@webrtc.org changed reviewers: + henrik.lundin@webrtc.org - katrielc@chromium.org
On 2016/06/15 09:22:23, katrielc wrote: > On 2016/06/15 09:18:14, danilchap wrote: > If kRtpExtensionNone is not an extension, should it be valid to register it in > the parser? (It currently is.) no, it is not really valid even if code doesn't crash when you do. Probably there should be a DCHECK to discourage that use.
https://codereview.webrtc.org/2062103002/diff/40001/webrtc/test/fuzzers/rtp_h... File webrtc/test/fuzzers/rtp_header_fuzzer.cc (right): https://codereview.webrtc.org/2062103002/diff/40001/webrtc/test/fuzzers/rtp_h... webrtc/test/fuzzers/rtp_header_fuzzer.cc:35: extensions.Register(static_cast<RTPExtensionType>(i), i); valid ids for an extension is in range 1-14, i.e. i = 0 is not a valid id. (may be you want int i = 1; i <= kNumberofRtpHeaderExtensions loop instead)
henrik.lundin@webrtc.org changed reviewers: - henrik.lundin@webrtc.org
This is unfortunately not my area. Check with the video guys (mflodman, sprang, etc). Either way, you should remove the question to me from the CL description; otherwise it will make it into eternity in the WebRTC repository.
Description was changed from ========== Add a libfuzzer for RtpHeaderParser. henrik.lundin: I changed rtp_rtcp_defines.h to add a count, is that OK? ========== to ========== Add a libfuzzer for RtpHeaderParser. NOTRY=true ==========
katrielc@webrtc.org changed reviewers: + mflodman@webrtc.org
mflodman: is the change to rtp_header_defines OK? https://codereview.webrtc.org/2062103002/diff/40001/webrtc/test/fuzzers/rtp_h... File webrtc/test/fuzzers/rtp_header_fuzzer.cc (right): https://codereview.webrtc.org/2062103002/diff/40001/webrtc/test/fuzzers/rtp_h... webrtc/test/fuzzers/rtp_header_fuzzer.cc:35: extensions.Register(static_cast<RTPExtensionType>(i), i); On 2016/06/15 10:16:33, danilchap wrote: > valid ids for an extension is in range 1-14, i.e. i = 0 is not a valid id. (may > be you want int i = 1; i <= kNumberofRtpHeaderExtensions loop instead) Done.
ping mflodman -- is the new constexpr in rtp_rtcp_defines.h OK? On 2016/06/15 13:22:48, katrielc1 wrote: > mflodman: is the change to rtp_header_defines OK?
On 2016/06/20 10:51:37, katrielc wrote: > ping mflodman -- is the new constexpr in rtp_rtcp_defines.h OK? > > On 2016/06/15 13:22:48, katrielc1 wrote: > > mflodman: is the change to rtp_header_defines OK? I'd prefer what Patrik suggests, add this to the enum itself instead. There is a very high risk och forgetting to update the constant when adding a new supported extension.
Description was changed from ========== Add a libfuzzer for RtpHeaderParser. NOTRY=true ========== to ========== Add a libfuzzer for RtpHeaderParser. ==========
On 2016/06/20 15:59:50, mflodman wrote: > On 2016/06/20 10:51:37, katrielc wrote: > > ping mflodman -- is the new constexpr in rtp_rtcp_defines.h OK? > > > > On 2016/06/15 13:22:48, katrielc1 wrote: > > > mflodman: is the change to rtp_header_defines OK? > > I'd prefer what Patrik suggests, add this to the enum itself instead. There is a > very high risk och forgetting to update the constant when adding a new supported > extension. OK, done -- that also requires changing a switch(enum) to add the missing case.
lgtm
The CQ bit was checked by katrielc@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062103002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6453)
ping mflodman (sorry to pester!)
Sorry for the slow review, LGTM for rtp_rtcp_defines.h with one nit comment. https://codereview.webrtc.org/2062103002/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2062103002/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:71: nit: I'd prefer to remove the empty line.
The CQ bit was checked by katrielc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/2062103002/#ps100001 (title: "Remove blank line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062103002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14411)
Description was changed from ========== Add a libfuzzer for RtpHeaderParser. ========== to ========== Add a libfuzzer for RtpHeaderParser. NOTRY=true ==========
katrielc@chromium.org changed reviewers: - katrielc@chromium.org
The CQ bit was checked by katrielc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2062103002/100001
Message was sent while issue was closed.
Description was changed from ========== Add a libfuzzer for RtpHeaderParser. NOTRY=true ========== to ========== Add a libfuzzer for RtpHeaderParser. NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add a libfuzzer for RtpHeaderParser. NOTRY=true ========== to ========== Add a libfuzzer for RtpHeaderParser. NOTRY=true Committed: https://crrev.com/d4bcdad26349ab9e03c065442ab7d5d13e166800 Cr-Commit-Position: refs/heads/master@{#13271} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d4bcdad26349ab9e03c065442ab7d5d13e166800 Cr-Commit-Position: refs/heads/master@{#13271} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
