Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
419693003:
Fix potential integer overflow in fpdf_render_loadimage.cpp (Closed)
|
Created:
6 years, 5 months ago by jun_fang Modified:
5 years, 9 months ago Base URL:
https://pdfium.googlesource.com/pdfium.git@master Visibility:
Public. |
DescriptionFix potential integer overflow in fpdf_render_loadimage.cpp
BUG=391929
R=tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/f04bd76
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : #Messages
Total messages: 10 (0 generated)
Hi Tom and Chris, please help to review it. Thanks!
https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right): https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:16: typedef base::CheckedNumeric<FX_DWORD> FxDword; nit: despite what palmer says about typedefs, these seem confusingly named to me. Sorry. How about FX_SAFE_DWORD or FX_SAFE_INT or similar? What do you think about moving these to the same place as those for FX_WORD? It would be a shame to have 17 different versions of FX_Something that all mean base::CheckedNumeric<FX_DWORD> sprinkled throughout the code.
> What do you think about moving these to the same place as those for FX_WORD? It > would be a shame to have 17 different versions of FX_Something that all mean > base::CheckedNumeric<FX_DWORD> sprinkled throughout the code. Good idea. I'm neutral about the particular names of the typedefs; I'll go along with whatever.
On 2014/07/25 17:29:02, Chromium Palmer wrote: > > What do you think about moving these to the same place as those for FX_WORD? > It > > would be a shame to have 17 different versions of FX_Something that all mean > > base::CheckedNumeric<FX_DWORD> sprinkled throughout the code. > > Good idea. > > I'm neutral about the particular names of the typedefs; I'll go along with > whatever. I agree with Tom too. I plan to define these safe types in the same file as FX_DWORD. It's pdfium/core/include/fxcrt/fx_system.h. Please tell me if you have any other concerns.
I'll leave this one to Tom and/or Jorge. I've got several more PDFium reviews on my plate. :)
https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right): https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:183: if (m_bpc == 0 || m_nComponents == 0) { We continue execution here in the old code in this case. Why do we want to short-circuit the calls below? Is this the comparison from line 221 of the old file moved up here? https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:226: if (!pitch.IsValid()) return FALSE; nit: braces, return on its own line, to match existing style. https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:226: if (!pitch.IsValid()) return FALSE; We can return here without setting m_pitch as in the old code. Does this matter? https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:240: if (!pitch.IsValid()) return FALSE; same two comments as above: nit for {} and do we need to set m_pitch? https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:321: return 0; same question as above for early return. https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1192: pitch *= (line+1); do you mean src_pitch *= (line + 1) https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1193: if (!pitch.IsValid()) { ditto https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1197: if (m_pStreamAcc->GetSize() >= (line + 1) * src_pitch) { didn't we just do the (line +1) multiplication above?
https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... File core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp (right): https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:16: typedef base::CheckedNumeric<FX_DWORD> FxDword; On 2014/07/25 16:15:23, Tom Sepez wrote: > nit: despite what palmer says about typedefs, these seem confusingly named to > me. Sorry. How about FX_SAFE_DWORD or FX_SAFE_INT or similar? > > What do you think about moving these to the same place as those for FX_WORD? It > would be a shame to have 17 different versions of FX_Something that all mean > base::CheckedNumeric<FX_DWORD> sprinkled throughout the code. will update it in the next patch. https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:183: if (m_bpc == 0 || m_nComponents == 0) { On 2014/07/25 21:13:51, Tom Sepez wrote: > We continue execution here in the old code in this case. Why do we want to > short-circuit the calls below? Is this the comparison from line 221 of the old > file moved up here? You can find |FX_DWORD src_pitch = m_bpc;| at line 178 of original code. if |m_bpc| is 0, it can't load any data at line 198 in the original code because |m_Height * src_pitch| is 0. bpc: bits per component components are used in color space. For example, RGB color space has 3 components. They must be larger than 0 (>0). https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:226: if (!pitch.IsValid()) return FALSE; On 2014/07/25 21:13:51, Tom Sepez wrote: > nit: braces, return on its own line, to match existing style. m_pitch will be updated if there is no problem during calculation.If there is an error in calculating it, we don't have a choice but keep old value.It's incorrect to assign width to pitch. https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:240: if (!pitch.IsValid()) return FALSE; On 2014/07/25 21:13:51, Tom Sepez wrote: > same two comments as above: nit for {} and do we need to set m_pitch? Will add {} in the next patch. https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:321: return 0; On 2014/07/25 21:13:51, Tom Sepez wrote: > same question as above for early return. at line 353 of the original code, m_bpc will be used indirectly. If it's 0, it's meaningless to call LoadAllData(). bpc and components are used to calculate pitch.pitch is 0 when one of them is 0. https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1192: pitch *= (line+1); On 2014/07/25 21:13:51, Tom Sepez wrote: > do you mean src_pitch *= (line + 1) at line 1196 of left code (OLD), there is (line + 1) * src_pitch. at line 1197 of left code (OLD), there is line * src_pitch. I added a check here to avoid to check two times. https://codereview.chromium.org/419693003/diff/20001/core/src/fpdfapi/fpdf_re... core/src/fpdfapi/fpdf_render/fpdf_render_loadimage.cpp:1197: if (m_pStreamAcc->GetSize() >= (line + 1) * src_pitch) { On 2014/07/25 21:13:51, Tom Sepez wrote: > didn't we just do the (line +1) multiplication above? Will update it in the next patch.
Hi Tom,please see my answers and review code change again.
LGTM thanks.
Message was sent while issue was closed.
Committed patchset #3 manually as rf04bd76 (presubmit successful). |
