Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1865203002:
Avoid integer overflow errors when parsing Exp-Golomb codes (Closed)
DescriptionAvoid integer overflow errors when parsing Exp-Golomb codes
Maximum value is 2^31 - 1.
BUG=600963
TEST=media_unittests pass
Committed: https://crrev.com/9373fc3e3743eaf0a3a835033bc89ad7b22df9d7
Cr-Commit-Position: refs/heads/master@{#385918}
Patch Set 1 #
Total comments: 5
Patch Set 2 : static_assert #
Total comments: 6
Patch Set 3 : 1u #
Total comments: 4
Patch Set 4 : more 1u #
Total comments: 2
Messages
Total messages: 21 (6 generated)
jrummell@chromium.org changed reviewers: + dalecurtis@chromium.org, sandersd@chromium.org
PTAL. https://codereview.chromium.org/1865203002/diff/1/media/filters/h264_parser.cc File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/1/media/filters/h264_parser.c... media/filters/h264_parser.cc:325: *val = std::numeric_limits<int>::max(); Since we're checking for 2^31-1 explicitly, I debated using 0x7fffffff or numeric_limits<int32_t> instead. Can int be larger than 32 bits on some platforms?
https://codereview.chromium.org/1865203002/diff/1/media/filters/h264_parser.cc File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/1/media/filters/h264_parser.c... media/filters/h264_parser.cc:324: if (num_bits == 31) { Add a comment to the first line explaining that we are limiting to &rest == 0, so the value must be 2 ^ 31 - 1. https://codereview.chromium.org/1865203002/diff/1/media/filters/h264_parser.c... media/filters/h264_parser.cc:325: *val = std::numeric_limits<int>::max(); On 2016/04/07 01:09:32, jrummell wrote: > Since we're checking for 2^31-1 explicitly, I debated using 0x7fffffff or > numeric_limits<int32_t> instead. Can int be larger than 32 bits on some > platforms? What we should actually be doing is rewriting H264Parser to use int32_t. There is no reason for this code to vary by platform (and in fact this code assumes that int == int32_t). A static assert here about the size of an int, along with a TODO, would be a good start.
Updated. https://codereview.chromium.org/1865203002/diff/1/media/filters/h264_parser.cc File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/1/media/filters/h264_parser.c... media/filters/h264_parser.cc:324: if (num_bits == 31) { On 2016/04/07 01:26:09, sandersd wrote: > Add a comment to the first line explaining that we are limiting to &rest == 0, > so the value must be 2 ^ 31 - 1. Done. https://codereview.chromium.org/1865203002/diff/1/media/filters/h264_parser.c... media/filters/h264_parser.cc:325: *val = std::numeric_limits<int>::max(); On 2016/04/07 01:26:09, sandersd wrote: > On 2016/04/07 01:09:32, jrummell wrote: > > Since we're checking for 2^31-1 explicitly, I debated using 0x7fffffff or > > numeric_limits<int32_t> instead. Can int be larger than 32 bits on some > > platforms? > > What we should actually be doing is rewriting H264Parser to use int32_t. There > is no reason for this code to vary by platform (and in fact this code assumes > that int == int32_t). > > A static assert here about the size of an int, along with a TODO, would be a > good start. Done.
https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... media/filters/h264_parser.cc:336: *val = (1 << num_bits) - 1; Is it sufficient to just have (1u << num_bits)
lgtm https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... media/filters/h264_parser.cc:336: *val = (1 << num_bits) - 1; On 2016/04/07 02:08:23, DaleCurtis wrote: > Is it sufficient to just have (1u << num_bits) It is correct to restrict to num_bits < 32, use entirely unsigned math, and then verify at the end that the result is <= INT_MAX. It doesn't seem to make anything simpler.
lgtm % up to you. https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... media/filters/h264_parser.cc:327: // TODO(jrummell): This file should be converted to int32_t as a lot of I don't think this is necessary, but up to you. We also use int as int32_t in chrome. https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... media/filters/h264_parser.cc:336: *val = (1 << num_bits) - 1; On 2016/04/07 at 17:56:16, sandersd wrote: > On 2016/04/07 02:08:23, DaleCurtis wrote: > > Is it sufficient to just have (1u << num_bits) > > It is correct to restrict to num_bits < 32, use entirely unsigned math, and then verify at the end that the result is <= INT_MAX. It doesn't seem to make anything simpler. dalecurtis@xorax /tmp $ cat test.cc #include <stdint.h> int main(int argc, char* argv[]) { int32_t a = (1 << 31) - 1; return a; } dalecurtis@xorax /tmp $ clang -Wall test.cc test.cc:4:25: warning: overflow in expression; result is 2147483647 with type 'int' [-Winteger-overflow] int32_t a = (1 << 31) - 1; ^ 1 warning generated. Switching to 1u seems sufficient. I'd just move this calculation to l.323, and delete lines l.327..l.331
Updated to use 1u. Tried compilation on Windows and no complaints. Also add h264_bit_reader which has the same issue, so one of you please review again. https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... media/filters/h264_parser.cc:327: // TODO(jrummell): This file should be converted to int32_t as a lot of On 2016/04/07 19:02:17, DaleCurtis wrote: > I don't think this is necessary, but up to you. We also use int as int32_t in > chrome. happy to remove my name from a TODO. https://codereview.chromium.org/1865203002/diff/20001/media/filters/h264_pars... media/filters/h264_parser.cc:336: *val = (1 << num_bits) - 1; On 2016/04/07 19:02:17, DaleCurtis wrote: > On 2016/04/07 at 17:56:16, sandersd wrote: > > On 2016/04/07 02:08:23, DaleCurtis wrote: > > > Is it sufficient to just have (1u << num_bits) > > > > It is correct to restrict to num_bits < 32, use entirely unsigned math, and > then verify at the end that the result is <= INT_MAX. It doesn't seem to make > anything simpler. > > dalecurtis@xorax /tmp $ cat test.cc > #include <stdint.h> > int main(int argc, char* argv[]) { > int32_t a = (1 << 31) - 1; > return a; > } > > dalecurtis@xorax /tmp $ clang -Wall test.cc > test.cc:4:25: warning: overflow in expression; result is 2147483647 with type > 'int' [-Winteger-overflow] > int32_t a = (1 << 31) - 1; > ^ > 1 warning generated. > > Switching to 1u seems sufficient. I'd just move this calculation to l.323, and > delete lines l.327..l.331 Done.
LGTM % small change. https://codereview.chromium.org/1865203002/diff/40001/media/filters/h264_bit_... File media/filters/h264_bit_reader.cc (right): https://codereview.chromium.org/1865203002/diff/40001/media/filters/h264_bit_... media/filters/h264_bit_reader.cc:82: *out &= ((1u << num_bits) - 1); Both should be 1u. https://codereview.chromium.org/1865203002/diff/40001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/40001/media/filters/h264_pars... media/filters/h264_parser.cc:326: *val = (1u << num_bits) - 1; Both ones should be 1u.
Thanks for the reviews. https://codereview.chromium.org/1865203002/diff/40001/media/filters/h264_bit_... File media/filters/h264_bit_reader.cc (right): https://codereview.chromium.org/1865203002/diff/40001/media/filters/h264_bit_... media/filters/h264_bit_reader.cc:82: *out &= ((1u << num_bits) - 1); On 2016/04/07 20:48:09, sandersd wrote: > Both should be 1u. Done. https://codereview.chromium.org/1865203002/diff/40001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/40001/media/filters/h264_pars... media/filters/h264_parser.cc:326: *val = (1u << num_bits) - 1; On 2016/04/07 20:48:09, sandersd wrote: > Both ones should be 1u. Done.
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/1865203002/#ps60001 (title: "more 1u")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1865203002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1865203002/60001
Again lgtm as is, but optional nits if you want. https://codereview.chromium.org/1865203002/diff/60001/media/filters/h264_pars... File media/filters/h264_parser.cc (right): https://codereview.chromium.org/1865203002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:323: // Special case for |num_bits| == 31 to avoid integer overflow. The only Comment goes with l.328, https://codereview.chromium.org/1865203002/diff/60001/media/filters/h264_pars... media/filters/h264_parser.cc:330: return (rest == 0) ? kOk : kInvalidStream; Could just be "return rest ? kInvalidStream : kOk"
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jrummell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1865203002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1865203002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Avoid integer overflow errors when parsing Exp-Golomb codes Maximum value is 2^31 - 1. BUG=600963 TEST=media_unittests pass ========== to ========== Avoid integer overflow errors when parsing Exp-Golomb codes Maximum value is 2^31 - 1. BUG=600963 TEST=media_unittests pass Committed: https://crrev.com/9373fc3e3743eaf0a3a835033bc89ad7b22df9d7 Cr-Commit-Position: refs/heads/master@{#385918} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9373fc3e3743eaf0a3a835033bc89ad7b22df9d7 Cr-Commit-Position: refs/heads/master@{#385918} |
