Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1828873002:
Fix FXSYS_StrToInt() (Closed)
DescriptionFix FXSYS_StrToInt()
Correctly handle sign and out of range values.
R=dsinclair@chromium.org, tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/6d18bd3b8ec82ae3c24a439f5c7925786a0e2d8b
Patch Set 1 : #
Total comments: 10
Patch Set 2 : address comments #
Messages
Total messages: 16 (7 generated)
Patchset #1 (id:1) has been deleted
weili@chromium.org changed reviewers: + tsepez@chromium.org
Stumbled upon this while trying to clear a warning. Fix this first. PTAL.
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... File core/fxcrt/fx_basic_gcc.cpp (right): https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc.cpp:28: if (std::numeric_limits<IntType>::is_signed) { I didn't make this change originally because I wasn't sure if the code currently depended on the weird truncation behaviour.
https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... File core/fxcrt/fx_basic_gcc.cpp (right): https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc.cpp:28: if (std::numeric_limits<IntType>::is_signed) { On 2016/03/24 01:40:44, dsinclair wrote: > I didn't make this change originally because I wasn't sure if the code currently > depended on the weird truncation behaviour. I checked callers which mainly use this to process fields or offsets of pdf file. That is why this has not causing any problem so far.
https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... File core/fxcrt/fx_basic_gcc.cpp (right): https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc.cpp:20: bool neg = *str == '-'; This loses the std::numeric_limits<IntType>::is_signed which we need below to determine if we can negate or not. Otherwise, calling with a uint8_t and passing -20 would do the wrong thing. https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc.cpp:35: return std::numeric_limits<IntType>::max(); If you put the is_signed check back at line 20 this if {} else can go away and just have the return on 31. https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... File core/fxcrt/fx_basic_gcc_unittest.cpp (right): https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc_unittest.cpp:96: EXPECT_EQ(4294967295, FXSYS_atoui("-1")); I think this is wrong, it should return 0 because - isn't valid. https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc_unittest.cpp:99: EXPECT_EQ(4294964951, FXSYS_atoui("-2345")); This should also be 0 I think. https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc_unittest.cpp:108: EXPECT_EQ(4294967295, FXSYS_atoui("-4294967345")); Also 0.
https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... File core/fxcrt/fx_basic_gcc.cpp (right): https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc.cpp:20: bool neg = *str == '-'; On 2016/03/24 02:41:06, dsinclair wrote: > This loses the std::numeric_limits<IntType>::is_signed which we need below to > determine if we can negate or not. Otherwise, calling with a uint8_t and passing > -20 would do the wrong thing. I think eventually we may want to use std:strto? to replace all these functions. It is ok if we would want to use a custom function to treat all negative values for unsigned values as 0, but the common way is to preserve the binary representation so that it would the same value after converting to an integer. For pdfium, str to unsigned are exclusively used for reading object nums and generation numbers, both of them are non-negative. Ditto for all the following comments.
lgtm https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... File core/fxcrt/fx_basic_gcc.cpp (right): https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc.cpp:20: bool neg = *str == '-'; On 2016/03/24 17:31:25, Wei Li wrote: > On 2016/03/24 02:41:06, dsinclair wrote: > > This loses the std::numeric_limits<IntType>::is_signed which we need below to > > determine if we can negate or not. Otherwise, calling with a uint8_t and > passing > > -20 would do the wrong thing. > > I think eventually we may want to use std:strto? to replace all these functions. > It is ok if we would want to use a custom function to treat all negative values > for unsigned values as 0, but the common way is to preserve the binary > representation so that it would the same value after converting to an integer. > > For pdfium, str to unsigned are exclusively used for reading object nums and > generation numbers, both of them are non-negative. > > Ditto for all the following comments. Fair enough, think the conditional on 28 can still be simplifed a bit: if (neg && std::numeric_limits<IntType>::is_signed) { return std::numeric_limits<IntType>::min(); else return std::numeric_limits<IntType>::max();
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
thanks https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... File core/fxcrt/fx_basic_gcc.cpp (right): https://codereview.chromium.org/1828873002/diff/20001/core/fxcrt/fx_basic_gcc... core/fxcrt/fx_basic_gcc.cpp:20: bool neg = *str == '-'; On 2016/03/24 18:10:11, dsinclair wrote: > On 2016/03/24 17:31:25, Wei Li wrote: > > On 2016/03/24 02:41:06, dsinclair wrote: > > > This loses the std::numeric_limits<IntType>::is_signed which we need below > to > > > determine if we can negate or not. Otherwise, calling with a uint8_t and > > passing > > > -20 would do the wrong thing. > > > > I think eventually we may want to use std:strto? to replace all these > functions. > > It is ok if we would want to use a custom function to treat all negative > values > > for unsigned values as 0, but the common way is to preserve the binary > > representation so that it would the same value after converting to an integer. > > > > For pdfium, str to unsigned are exclusively used for reading object nums and > > generation numbers, both of them are non-negative. > > > > Ditto for all the following comments. > > > Fair enough, think the conditional on 28 can still be simplifed a bit: > > if (neg && std::numeric_limits<IntType>::is_signed) { > return std::numeric_limits<IntType>::min(); > else > return std::numeric_limits<IntType>::max(); Done.
lgtm
Description was changed from ========== Fix FXSYS_StrToInt() Correctly handle sign and out of range values. ========== to ========== Fix FXSYS_StrToInt() Correctly handle sign and out of range values. R=dsinclair@chromium.org, tsepez@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/6d18bd3b8ec82ae3c24a439f5c7925786a0e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001) manually as 6d18bd3b8ec82ae3c24a439f5c7925786a0e2d8b (presubmit successful).
Message was sent while issue was closed.
Patchset #3 (id:100001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
