Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2358023002:
Check for overflow in CMap_GetCode. (Closed)
DescriptionCheck for overflow in CMap_GetCode.
Given a large enough value for the character code it's possible to overflow
the conversion to an int. This Cl updates the code to guard against overflow.
BUG=chromium:648739
Committed: https://pdfium.googlesource.com/pdfium/+/c0f60dc29db66262bbc0082fcd51170a570b0d1f
Patch Set 1 #
Total comments: 11
Patch Set 2 : Review feedback #
Total comments: 4
Patch Set 3 : Review fixes #
Messages
Total messages: 19 (12 generated)
The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dsinclair@chromium.org changed reviewers: + tsepez@chromium.org, weili@chromium.org
PTAL. https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... File core/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right): https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:444: pdfium::base::CheckedNumeric<uint32_t> num = 0; The method returns a uint32_t, so I updated the internal value to a uint32_t to match the return type.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... File core/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right): https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:444: pdfium::base::CheckedNumeric<uint32_t> num = 0; On 2016/09/21 14:56:33, dsinclair wrote: > The method returns a uint32_t, so I updated the internal value to a uint32_t to > match the return type. I'm guessing this is UBSAN noise? Or do we really care and want to return 0 if it overflows? If its the former, then making num be uint32_t means it just overflows without triggering undefined behaviour. If its the later, ok. https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:449: break; just return 0 here. https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:451: return num.ValueOrDefault(0); then valueordie here. https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:457: break; just return 0 here. https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:459: return num.ValueOrDefault(0); then valueordie here.
https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... File core/fpdfapi/fpdf_font/fpdf_font_cid.cpp (right): https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:444: pdfium::base::CheckedNumeric<uint32_t> num = 0; On 2016/09/21 16:48:51, Tom Sepez wrote: > On 2016/09/21 14:56:33, dsinclair wrote: > > The method returns a uint32_t, so I updated the internal value to a uint32_t > to > > match the return type. > > I'm guessing this is UBSAN noise? Or do we really care and want to return 0 if > it overflows? > > If its the former, then making num be uint32_t means it just overflows without > triggering undefined behaviour. > > If its the later, ok. > I think we want the latter as, I believe, returning 0 would give us the null char and not display anything vs display something random. https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:449: break; On 2016/09/21 16:48:51, Tom Sepez wrote: > just return 0 here. Done. https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:451: return num.ValueOrDefault(0); On 2016/09/21 16:48:51, Tom Sepez wrote: > then valueordie here. Done. https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:457: break; On 2016/09/21 16:48:51, Tom Sepez wrote: > just return 0 here. Done. https://codereview.chromium.org/2358023002/diff/1/core/fpdfapi/fpdf_font/fpdf... core/fpdfapi/fpdf_font/fpdf_font_cid.cpp:459: return num.ValueOrDefault(0); On 2016/09/21 16:48:51, Tom Sepez wrote: > then valueordie here. Done.
The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2358023002/diff/20001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/fpdf_font_cid_unittest.cpp (right): https://codereview.chromium.org/2358023002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/fpdf_font_cid_unittest.cpp:33: EXPECT_EQ(0u, CPDF_CMapParser::CMap_GetCode("<FFFFFFFFa")); nit: how about 100000000, the first thing that overflows. https://codereview.chromium.org/2358023002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/fpdf_font_cid_unittest.cpp:34: EXPECT_EQ(4294967295u, CPDF_CMapParser::CMap_GetCode("<FFFFFFFF")); nit: this should come before the case at line 33 since its smaller.
https://codereview.chromium.org/2358023002/diff/20001/core/fpdfapi/fpdf_font/... File core/fpdfapi/fpdf_font/fpdf_font_cid_unittest.cpp (right): https://codereview.chromium.org/2358023002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/fpdf_font_cid_unittest.cpp:33: EXPECT_EQ(0u, CPDF_CMapParser::CMap_GetCode("<FFFFFFFFa")); On 2016/09/21 18:58:13, Tom Sepez wrote: > nit: how about 100000000, the first thing that overflows. Done. https://codereview.chromium.org/2358023002/diff/20001/core/fpdfapi/fpdf_font/... core/fpdfapi/fpdf_font/fpdf_font_cid_unittest.cpp:34: EXPECT_EQ(4294967295u, CPDF_CMapParser::CMap_GetCode("<FFFFFFFF")); On 2016/09/21 18:58:13, Tom Sepez wrote: > nit: this should come before the case at line 33 since its smaller. Done.
The CQ bit was checked by dsinclair@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2358023002/#ps40001 (title: "Review fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Check for overflow in CMap_GetCode. Given a large enough value for the character code it's possible to overflow the conversion to an int. This Cl updates the code to guard against overflow. BUG=chromium:648739 ========== to ========== Check for overflow in CMap_GetCode. Given a large enough value for the character code it's possible to overflow the conversion to an int. This Cl updates the code to guard against overflow. BUG=chromium:648739 Committed: https://pdfium.googlesource.com/pdfium/+/c0f60dc29db66262bbc0082fcd51170a570b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://pdfium.googlesource.com/pdfium/+/c0f60dc29db66262bbc0082fcd51170a570b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
