Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 2168173002: Fixup integer conversion logic. (Closed)

Created:
4 years, 2 months ago by dsinclair
Modified:
4 years, 2 months ago
Reviewers:
Lei Zhang
CC:
pdfium-reviews_googlegroups.com, npm
Base URL:
https://pdfium.googlesource.com/pdfium.git@master
Target Ref:
refs/heads/master
Project:
pdfium
Visibility:
Public.

Description

Fixup integer conversion logic. In bc8a64029f898286c3dcad3a6cecdc98ef30b139 we updated the FX_atonum logic to correctly handle integer overflow. This causes issues when parsing the Permissions flag of encrypted documents as that flag isn't encoded like other numbers. The Permissions flag is a unsigned value, and has to be treated as such since the sign bit is always set. The current logic will detect an overflow of the int value and return 0. The old logic would have detected the overflow and returned the negative result regardless. This CL updates the logic to do the string to int conversion as a uint32_t and then verifies the uint32_t value, if a sign was provided, fits within the int range, otherwise it converts it to an int and lets it be positive or negative as needed. BUG=pdfium:539 Committed: https://pdfium.googlesource.com/pdfium/+/6f1025492801aaa93fca2c0ed7c40a3389ad8cd1

Patch Set 1 #

Patch Set 2 : Fixup test value #

Total comments: 4

Patch Set 3 : Rebase to master #

Patch Set 4 : Fixup boundary cases #

Patch Set 5 : Rebase to master #

Patch Set 6 : Fixup cast to int and negation #

Patch Set 7 : Better min int value #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -4 lines) Patch
M core/fxcrt/fx_basic_util.cpp View 1 2 3 4 5 2 chunks +37 lines, -4 lines 0 comments Download
M core/fxcrt/fx_basic_util_unittest.cpp View 1 2 3 4 5 6 2 chunks +47 lines, -0 lines 0 comments Download

Messages

Total messages: 21 (13 generated)
dsinclair
PTAL.
4 years, 2 months ago (2016-07-21 17:34:36 UTC) #2
Lei Zhang
https://codereview.chromium.org/2168173002/diff/20001/core/fxcrt/fx_basic_util.cpp File core/fxcrt/fx_basic_util.cpp (right): https://codereview.chromium.org/2168173002/diff/20001/core/fxcrt/fx_basic_util.cpp#newcode37 core/fxcrt/fx_basic_util.cpp:37: // actually a unsigned value. We treat use uint32_t ...
4 years, 2 months ago (2016-07-21 20:39:31 UTC) #4
dsinclair
https://codereview.chromium.org/2168173002/diff/20001/core/fxcrt/fx_basic_util.cpp File core/fxcrt/fx_basic_util.cpp (right): https://codereview.chromium.org/2168173002/diff/20001/core/fxcrt/fx_basic_util.cpp#newcode37 core/fxcrt/fx_basic_util.cpp:37: // actually a unsigned value. We treat use uint32_t ...
4 years, 2 months ago (2016-07-25 13:47:59 UTC) #6
Lei Zhang
Windows bots are red. :\
4 years, 2 months ago (2016-07-25 17:40:17 UTC) #7
dsinclair
win bots happy, ptal.
4 years, 2 months ago (2016-07-27 15:03:55 UTC) #16
Lei Zhang
lgtm
4 years, 2 months ago (2016-07-28 04:43:57 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2168173002/120001
4 years, 2 months ago (2016-07-28 04:44:08 UTC) #19
commit-bot: I haz the power
4 years, 2 months ago (2016-07-28 04:44:29 UTC) #21
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://pdfium.googlesource.com/pdfium/+/6f1025492801aaa93fca2c0ed7c40a3389ad...

Powered by Google App Engine
This is Rietveld 408576698