Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1888313002:
Fix the issue of undefined-shift in VP8GetBit. (Closed)
|
Created:
4 years, 8 months ago by jackychen_ Modified:
4 years, 8 months ago Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix the issue of undefined-shift in VP8GetBit.
BUG=chromium:603497
Committed: https://crrev.com/0a2c054f426104ed5364424d3ec88d6cffe203eb
Cr-Commit-Position: refs/heads/master@{#12450}
Patch Set 1 #
Total comments: 6
Patch Set 2 : use RTC_CHECK_GE #
Total comments: 4
Patch Set 3 : Use error_ to save bitReader status. #
Total comments: 6
Patch Set 4 : Remove error_ #
Total comments: 2
Patch Set 5 : Check eof_ status. #Messages
Total messages: 30 (9 generated)
Description was changed from ========== Fix the issue of undefined-shift in VP8GetBit. BUG=chromium:603497 ========== to ========== Fix the issue of undefined-shift in VP8GetBit. BUG=chromium:603497 ==========
jackychen@google.com changed reviewers: + marpan@webrtc.org
jackychen@google.com changed reviewers: + mflodman@webrtc.org, pbos@webrtc.org
I think you should move the error to where the packet is incorrect. https://codereview.webrtc.org/1888313002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/vp8_header_parser.cc:83: pos = 0; Also log an error in here, but I think you should do that when bits_ is read instead and not here. This is not where the error occurs, br->bits_ shouldn't be able to be negative right? https://codereview.webrtc.org/1888313002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/vp8_header_parser.cc:84: kParseError = 1; Does this even compile? You shouldn't modify global state in this code. https://codereview.webrtc.org/1888313002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/vp8_header_parser.cc:178: kParseError = 0; Should this not be a property of the bitreader?
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/1888313002/diff/1/webrtc/modules/video_coding/u... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/vp8_header_parser.cc:83: pos = 0; On 2016/04/15 08:58:58, pbos-webrtc wrote: > Also log an error in here, but I think you should do that when bits_ is read > instead and not here. This is not where the error occurs, br->bits_ shouldn't be > able to be negative right? Right, it must be an invalid bitstream. https://codereview.webrtc.org/1888313002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/vp8_header_parser.cc:84: kParseError = 1; On 2016/04/15 08:58:58, pbos-webrtc wrote: > Does this even compile? You shouldn't modify global state in this code. Done. https://codereview.webrtc.org/1888313002/diff/1/webrtc/modules/video_coding/u... webrtc/modules/video_coding/utility/vp8_header_parser.cc:178: kParseError = 0; On 2016/04/15 08:58:58, pbos-webrtc wrote: > Should this not be a property of the bitreader? Done.
Patchset #2 (id:40001) has been deleted
https://codereview.webrtc.org/1888313002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:12: #include "webrtc/base/logging.h" These should go below where they were, vp8_header_parser.h should be included first since it belongs to this .cc file. https://codereview.webrtc.org/1888313002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:79: RTC_CHECK_GE(br->bits_, 0); You can't use this for network data. We cannot crash here (which RTC_CHECK makes us do). We need to be able to abort gracefully here. Perhaps this function should be: static bool VP8GetBit(VP8BitReader* br, int* out_bit, int prob), and return true/false based on success. Then VP8LoadNewBytes should be able to pass/fail.
https://codereview.webrtc.org/1888313002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:12: #include "webrtc/base/logging.h" On 2016/04/18 15:10:14, pbos-webrtc wrote: > These should go below where they were, vp8_header_parser.h should be included > first since it belongs to this .cc file. Done. https://codereview.webrtc.org/1888313002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:79: RTC_CHECK_GE(br->bits_, 0); On 2016/04/18 15:10:14, pbos-webrtc wrote: > You can't use this for network data. We cannot crash here (which RTC_CHECK makes > us do). We need to be able to abort gracefully here. > > Perhaps this function should be: > > static bool VP8GetBit(VP8BitReader* br, int* out_bit, int prob), and return > true/false based on success. Then VP8LoadNewBytes should be able to pass/fail. When VP8GetBit read invalid bitstream, it should pass the failure status all the way up to GetQp to indicate the QP read for this frame is not usable. I think your comment in patch #1 is right, the status should be a property of bitReader. I misunderstood your comment about "abort", I thought you mean "crash", my bad!
https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:69: br->error_ = false; Is this already covered by the eof_ flag? If so we don't need error_. https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:79: if (br->bits_ < 0) { Shouldn't this be set inside VP8LoadNewBytes when it fails?
https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:69: br->error_ = false; On 2016/04/18 19:25:33, pbos-webrtc wrote: > Is this already covered by the eof_ flag? If so we don't need error_. Not really, the error here might not be the same with what eof_ covers, but do you want me to reuse eof_ to also cover this error? https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:79: if (br->bits_ < 0) { On 2016/04/18 19:25:33, pbos-webrtc wrote: > Shouldn't this be set inside VP8LoadNewBytes when it fails? I want to stop the operation on br->bits here since otherwise I need to change the return value of VP8LoadNewBytes and also add the judge here. Besides, the error may also happens in VP8LoadFinalBytes. So I thought it might be better to put it here.
https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:69: br->error_ = false; On 2016/04/18 20:49:47, jacky.chen wrote: > On 2016/04/18 19:25:33, pbos-webrtc wrote: > > Is this already covered by the eof_ flag? If so we don't need error_. > > Not really, the error here might not be the same with what eof_ covers, > but do you want me to reuse eof_ to also cover this error? Have you diagnosed the actual error that happens?
https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/utility/vp8_header_parser.cc:203: LOG(LS_WARNING) << "Failed to get QP, invalid bitstream."; Are you sure that this isn't because the stream is cut off (e.g. should go under eof_?)?
Patchset #4 (id:100001) has been deleted
lgtm
https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp8_header_parser.cc:59: br->eof_ = 1; Actually, does this happen from VP8LoadFinalBytes? or how does this happen? is eof_ already set and we should just check eof_ from VP8GetBit?
On 2016/04/19 23:11:50, pbos-webrtc wrote: > https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... > File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): > > https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... > webrtc/modules/video_coding/utility/vp8_header_parser.cc:59: br->eof_ = 1; > Actually, does this happen from VP8LoadFinalBytes? or how does this happen? is > eof_ already set and we should just check eof_ from VP8GetBit? Probably! Can you check it locally to see whether just checking eof_ can fix the crash issue?
https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/utility/vp8_header_parser.cc:59: br->eof_ = 1; On 2016/04/19 23:11:50, pbos-webrtc wrote: > Actually, does this happen from VP8LoadFinalBytes? or how does this happen? is > eof_ already set and we should just check eof_ from VP8GetBit? I'm not sure. So to be safe, I put the check here.
On 2016/04/20 00:27:18, jacky.chen wrote: > https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... > File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): > > https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... > webrtc/modules/video_coding/utility/vp8_header_parser.cc:59: br->eof_ = 1; > On 2016/04/19 23:11:50, pbos-webrtc wrote: > > Actually, does this happen from VP8LoadFinalBytes? or how does this happen? is > > eof_ already set and we should just check eof_ from VP8GetBit? > > I'm not sure. So to be safe, I put the check here. The following fix seems to work for me: diff --git a/webrtc/modules/video_coding/utility/vp8_header_parser.cc b/webrtc/modules/video_coding/utility/vp8_header_parser.cc index 631385d..cfa4e90 100644 --- a/webrtc/modules/video_coding/utility/vp8_header_parser.cc +++ b/webrtc/modules/video_coding/utility/vp8_header_parser.cc @@ -74,6 +74,8 @@ static int VP8GetBit(VP8BitReader* const br, int prob) { uint8_t range = br->range_; if (br->bits_ < 0) { VP8LoadNewBytes(br); + if (br->eof_) + return 0; } const int pos = br->bits_; So just check eof_ after VP8LoadNewBytes and we're probably fine. Then you don't need to add status codes to VP8LoadNewBytes either. I'll keep running the fuzzer a bit longer.
On 2016/04/20 12:27:04, pbos-webrtc wrote: > On 2016/04/20 00:27:18, jacky.chen wrote: > > > https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... > > File webrtc/modules/video_coding/utility/vp8_header_parser.cc (right): > > > > > https://codereview.webrtc.org/1888313002/diff/120001/webrtc/modules/video_cod... > > webrtc/modules/video_coding/utility/vp8_header_parser.cc:59: br->eof_ = 1; > > On 2016/04/19 23:11:50, pbos-webrtc wrote: > > > Actually, does this happen from VP8LoadFinalBytes? or how does this happen? > is > > > eof_ already set and we should just check eof_ from VP8GetBit? > > > > I'm not sure. So to be safe, I put the check here. > > The following fix seems to work for me: > > diff --git a/webrtc/modules/video_coding/utility/vp8_header_parser.cc > b/webrtc/modules/video_coding/utility/vp8_header_parser.cc > index 631385d..cfa4e90 100644 > --- a/webrtc/modules/video_coding/utility/vp8_header_parser.cc > +++ b/webrtc/modules/video_coding/utility/vp8_header_parser.cc > @@ -74,6 +74,8 @@ static int VP8GetBit(VP8BitReader* const br, int prob) { > uint8_t range = br->range_; > if (br->bits_ < 0) { > VP8LoadNewBytes(br); > + if (br->eof_) > + return 0; > } > > const int pos = br->bits_; > > So just check eof_ after VP8LoadNewBytes and we're probably fine. Then you don't > need to add status codes to VP8LoadNewBytes either. I'll keep running the fuzzer > a bit longer. Cool! I have make the change you recommended. Do you need to lgtm again?
lgtm woo
The CQ bit was checked by jackychen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1888313002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1888313002/140001
Message was sent while issue was closed.
Description was changed from ========== Fix the issue of undefined-shift in VP8GetBit. BUG=chromium:603497 ========== to ========== Fix the issue of undefined-shift in VP8GetBit. BUG=chromium:603497 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Fix the issue of undefined-shift in VP8GetBit. BUG=chromium:603497 ========== to ========== Fix the issue of undefined-shift in VP8GetBit. BUG=chromium:603497 Committed: https://crrev.com/0a2c054f426104ed5364424d3ec88d6cffe203eb Cr-Commit-Position: refs/heads/master@{#12450} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0a2c054f426104ed5364424d3ec88d6cffe203eb Cr-Commit-Position: refs/heads/master@{#12450} |
