Strengthen bounds check in CWeightTable::Calc * part II

Strengthen bounds check in CWeightTable::Calc * part II

This CL implemented a better version of CWeightTable::GetPixelWeightSize(), which will calculate the size of array PixelWeight.m_Weights correctly to prevent potential heap buffer overflow conditions.

BUG=chromium:654183
R=ochang@chromium.org, thestig@chromium.org, dsinclair@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/05923132ae08d45fbe957219775a48c55ee57aef

[Ke Liu, 2016-10-08 07:51:21]

The beginning address of array |PixelWeight.m_Weights| can be changed
dynamically. So we must calculate the maximum available array size dynamically
too.

This CL strengthens bounds check for accessing array PixelWeight.m_Weights.

BTW, although the heap buffer overflow issue was triggered under XFA, but the
code was shared in non XFA mode too. In other words, pdfium may be vulnerable
even if XFA was disabled.

[Lei Zhang, 2016-10-10 18:30:50]

thestig@chromium.org changed reviewers:
+ tsepez@chromium.org

[Lei Zhang, 2016-10-10 18:30:51]

https://codereview.chromium.org/2404453003/diff/1/core/fxge/dib/fx_dib_engine...
File core/fxge/dib/fx_dib_engine.cpp (left):

https://codereview.chromium.org/2404453003/diff/1/core/fxge/dib/fx_dib_engine...
core/fxge/dib/fx_dib_engine.cpp:46: return m_dwWeightTablesSize / sizeof(int);
Should we instead just check for:

return m_ItemSize / sizeof(int);

??

[Ke Liu, 2016-10-11 03:31:17]

On 2016/10/10 18:30:51, Lei Zhang wrote:
>
https://codereview.chromium.org/2404453003/diff/1/core/fxge/dib/fx_dib_engine...
> File core/fxge/dib/fx_dib_engine.cpp (left):
> 
>
https://codereview.chromium.org/2404453003/diff/1/core/fxge/dib/fx_dib_engine...
> core/fxge/dib/fx_dib_engine.cpp:46: return m_dwWeightTablesSize / sizeof(int);
> Should we instead just check for:
> 
> return m_ItemSize / sizeof(int);
> 
> ??

Thanks, it sounds good. And is the following style more preferable?

return (m_ItemSize - sizeof(int) * 2) / sizeof(int);

Reasons:
1. According to the definition of PixelWeight, there are two int type members
before the |m_Weights| array.

// core/fxge/dib/dib_int.h
38 struct PixelWeight {
39   int m_SrcStart;
40   int m_SrcEnd;
41   int m_Weights[1];
42 };

2. Accordingly, |m_ItemSize| contains the size of these two int type members.

// core/fxge/dib/fx_dib_engine.cpp
62  m_ItemSize =
63      sizeof(int) * 2 +
64      (int)(sizeof(int) * (FXSYS_ceil(FXSYS_fabs((FX_FLOAT)scale)) +
ext_size));

[Ke Liu, 2016-10-11 03:42:22]

Description was changed from

==========
Strengthen bounds check in CWeightTable::Calc * part II

The beginning address of array |PixelWeight.m_Weights| can be changed
dynamically. So we must calculate the maximum available array size dynamically
too.

This CL strengthens bounds check for accessing array PixelWeight.m_Weights.

BUG=chromium:654183
R=ochang@chromium.org, thestig@chromium.org, dsinclair@chromium.org
==========

to

==========
Strengthen bounds check in CWeightTable::Calc * part II

This CL implemented a better version of CWeightTable::GetPixelWeightSize(),
which will calculate the size of array PixelWeight.m_Weights correctly to
prevent potential heap buffer overflow conditions.

BUG=chromium:654183
R=ochang@chromium.org, thestig@chromium.org, dsinclair@chromium.org
==========

[Ke Liu, 2016-10-14 01:53:24]

On 2016/10/11 03:31:17, Ke Liu wrote:
> On 2016/10/10 18:30:51, Lei Zhang wrote:
> >
>
https://codereview.chromium.org/2404453003/diff/1/core/fxge/dib/fx_dib_engine...
> > File core/fxge/dib/fx_dib_engine.cpp (left):
> > 
> >
>
https://codereview.chromium.org/2404453003/diff/1/core/fxge/dib/fx_dib_engine...
> > core/fxge/dib/fx_dib_engine.cpp:46: return m_dwWeightTablesSize /
sizeof(int);
> > Should we instead just check for:
> > 
> > return m_ItemSize / sizeof(int);
> > 
> > ??
> 
> Thanks, it sounds good. And is the following style more preferable?
> 
> return (m_ItemSize - sizeof(int) * 2) / sizeof(int);
> 
> Reasons:
> 1. According to the definition of PixelWeight, there are two int type members
> before the |m_Weights| array.
> 
> // core/fxge/dib/dib_int.h
> 38 struct PixelWeight {
> 39   int m_SrcStart;
> 40   int m_SrcEnd;
> 41   int m_Weights[1];
> 42 };
> 
> 2. Accordingly, |m_ItemSize| contains the size of these two int type members.
> 
> // core/fxge/dib/fx_dib_engine.cpp
> 62  m_ItemSize =
> 63      sizeof(int) * 2 +
> 64      (int)(sizeof(int) * (FXSYS_ceil(FXSYS_fabs((FX_FLOAT)scale)) +
> ext_size));

ping

[Lei Zhang, 2016-10-14 23:51:54]

https://codereview.chromium.org/2404453003/diff/20001/core/fxge/dib/fx_dib_en...
File core/fxge/dib/fx_dib_engine.cpp (right):

https://codereview.chromium.org/2404453003/diff/20001/core/fxge/dib/fx_dib_en...
core/fxge/dib/fx_dib_engine.cpp:46: return (m_ItemSize - sizeof(int) * 2) /
sizeof(int);
How about: (m_ItemSize / sizeof(int)) - 2;

[Ke Liu, 2016-10-17 01:54:25]

On 2016/10/14 23:51:54, Lei Zhang wrote:
>
https://codereview.chromium.org/2404453003/diff/20001/core/fxge/dib/fx_dib_en...
> File core/fxge/dib/fx_dib_engine.cpp (right):
> 
>
https://codereview.chromium.org/2404453003/diff/20001/core/fxge/dib/fx_dib_en...
> core/fxge/dib/fx_dib_engine.cpp:46: return (m_ItemSize - sizeof(int) * 2) /
> sizeof(int);
> How about: (m_ItemSize / sizeof(int)) - 2;

Thank you Lei. I've updated the code. Please review it.

[Lei Zhang, 2016-10-17 06:29:57]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-10-17 06:29:57]

lgtm

[commit-bot: I haz the power, 2016-10-17 06:30:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 07:16:25]

Description was changed from

==========
Strengthen bounds check in CWeightTable::Calc * part II

This CL implemented a better version of CWeightTable::GetPixelWeightSize(),
which will calculate the size of array PixelWeight.m_Weights correctly to
prevent potential heap buffer overflow conditions.

BUG=chromium:654183
R=ochang@chromium.org, thestig@chromium.org, dsinclair@chromium.org
==========

to

==========
Strengthen bounds check in CWeightTable::Calc * part II

This CL implemented a better version of CWeightTable::GetPixelWeightSize(),
which will calculate the size of array PixelWeight.m_Weights correctly to
prevent potential heap buffer overflow conditions.

BUG=chromium:654183
R=ochang@chromium.org, thestig@chromium.org, dsinclair@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/05923132ae08d45fbe957219775a48c55ee5...
==========

[commit-bot: I haz the power, 2016-10-17 07:16:26]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/05923132ae08d45fbe957219775a48c55ee5...