Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
384593002:
Fix uninitialized coords in _DrawCoonPatchMeshes (Closed)
|
Created:
6 years, 5 months ago by jun_fang Modified:
6 years, 5 months ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Visibility:
Public. |
DescriptionFix uninitialized coords in _DrawCoonPatchMeshes
BUG=391470
R=palmer@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/f86d7d6
Patch Set 1 #
Total comments: 11
Messages
Total messages: 8 (0 generated)
https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... File core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp (right): https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:691: for (int i = 0; i < 16; i ++) This is fine. But a more elegant solution is to declare that the default constructor of |CFX_FloatPoint| sets its member fields to 0 (or whatever). The default constructor will be called for each element of the array. https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:698: FX_DWORD flag = stream.GetFlag(); Because this value is untrustworthy (comes from the PDF itself, right?) and is unchecked, ... https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:709: tempColors[0] = patch.patch_colors[flag]; ... this could be an out-of-bounds read, I think.
https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... File core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp (right): https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:691: for (int i = 0; i < 16; i ++) On 2014/07/10 23:11:01, Chromium Palmer wrote: > This is fine. > > But a more elegant solution is to declare that the default constructor of > |CFX_FloatPoint| sets its member fields to 0 (or whatever). The default > constructor will be called for each element of the array. CFX_FloatPoint is created by CFX_PSVTTemplate<FX_FLOAT>. This template supports couple types. https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:698: FX_DWORD flag = stream.GetFlag(); On 2014/07/10 23:11:01, Chromium Palmer wrote: > Because this value is untrustworthy (comes from the PDF itself, right?) and is > unchecked, ... The value from GetFlag() is limited to 0 to 3. The definition of GetFlag is shown as below: FX_DWORD CPDF_MeshStream::GetFlag() { return m_BitStream.GetBits(m_nFlagBits) & 0x03; } https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:709: tempColors[0] = patch.patch_colors[flag]; On 2014/07/10 23:11:01, Chromium Palmer wrote: > ... this could be an out-of-bounds read, I think. flag should be 0 to 3 as explained before. So it's safe.
https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... File core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp (right): https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:691: for (int i = 0; i < 16; i ++) > CFX_FloatPoint is created by CFX_PSVTTemplate<FX_FLOAT>. This template supports > couple types. Yes. I am saying, you could give that template a default constructor. Currently it does not have one. https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:698: FX_DWORD flag = stream.GetFlag(); On 2014/07/15 01:13:41, jun_fang wrote: > On 2014/07/10 23:11:01, Chromium Palmer wrote: > > Because this value is untrustworthy (comes from the PDF itself, right?) and is > > unchecked, ... > > The value from GetFlag() is limited to 0 to 3. The definition of GetFlag is > shown as below: > > FX_DWORD CPDF_MeshStream::GetFlag() > { > return m_BitStream.GetBits(m_nFlagBits) & 0x03; > } Acknowledged. https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:709: tempColors[0] = patch.patch_colors[flag]; On 2014/07/15 01:13:41, jun_fang wrote: > On 2014/07/10 23:11:01, Chromium Palmer wrote: > > ... this could be an out-of-bounds read, I think. > > flag should be 0 to 3 as explained before. So it's safe. Acknowledged.
LGTM
https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... File core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp (right): https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:691: for (int i = 0; i < 16; i ++) On 2014/07/15 01:49:17, Chromium Palmer wrote: > > CFX_FloatPoint is created by CFX_PSVTTemplate<FX_FLOAT>. This template > supports > > couple types. > > Yes. I am saying, you could give that template a default constructor. Currently > it does not have one. :-). I know. I tried to add a default constructor. However there is a minor issue that we may use 0 to initialize a float/double variable because we don't know which type will be created. We can only use one type to initialize.
Message was sent while issue was closed.
Committed patchset #1 manually as rf86d7d6 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... File core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp (right): https://codereview.chromium.org/384593002/diff/1/core/src/fpdfapi/fpdf_render... core/src/fpdfapi/fpdf_render/fpdf_render_pattern.cpp:691: for (int i = 0; i < 16; i ++) On 2014/07/15 03:56:17, jun_fang wrote: > On 2014/07/15 01:49:17, Chromium Palmer wrote: > > > CFX_FloatPoint is created by CFX_PSVTTemplate<FX_FLOAT>. This template > > supports > > > couple types. > > > > Yes. I am saying, you could give that template a default constructor. > Currently > > it does not have one. > > :-). I know. I tried to add a default constructor. However there is a minor > issue that we may use 0 to initialize a float/double variable because we don't > know which type will be created. We can only use one type to initialize. Just happened to see this CL go by. The nice thing with templates is you can use the "typename T" and T() (default initializer) to zero-initialize numeric types. In C++ language, this is known as the "Default Constructible" requirement (20.1.5 of C++03), which defines the requirement that T() must be a well-formed expression. Among the STL, you have many types require default-constructible; among them, vectors and maps (which also zero-initialize). This behaviour (for primitives) is defined in Section 8.5 of C++03. In particular, the language of 8.5 subclause 5 and 7. That is, the initializer for all scalar types (which are a subset of the object types clause) have a default initializer (), which is a value-initializer (subclause 7). The default value-initializer for a scalar type is to zero-initialize (subclause 5). It's an extremely portable way to zero-initialize all types T, where T may be a scalar type. You're guaranteed that the CFX_FloatPoint constructor (really, CFX_PSVVTTemplate<FX_FLOAT> constructor) will be run for every member in the array (subclause 5 dictates that array types have every member value-initialized), ergo if your constructor value-initialized the coords using T() [Or, in the case of CFX_PSVTemplate, "baseType()"], this would be entirely unnecessary. To express this in an initializer list, you could do CFX_PSVTemplate::CFX_PSVTemplate() : x(), y() {} which would appropriately value-initialize (ergo zero-initialize) |
||||||||||||||||||||||||||||||||||||||||||||||||||
