check pointer overflow in t2.c

check pointer overflow in t2.c

BUG=413375
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/e93d5341d87c54713a9632c8823288fa901a3b78

[Bo Xu, 2014-10-03 18:35:59]

bo_xu@foxitsoftware.com changed reviewers:
+ tsepez@chromium.org

[Bo Xu, 2014-10-03 18:35:59]

Hi Tom, some discussion of this fix is here
https://code.google.com/p/openjpeg/issues/detail?id=390.

Seems the original check was optimized out by the compiler? There are some
similar checks haven't been changed in openjpeg. I will merge them once they are
done.

[Tom Sepez, 2014-10-03 18:38:26]

Heh.  I don't have permission to see the upstream issue.  I can review this on
its own, though.  Stay tuned.

[Tom Sepez, 2014-10-03 18:41:00]

LGTM.  This is similar to something I suggested on one of the other code
reivews, right?

[Tom Sepez, 2014-10-03 18:41:10]

https://codereview.chromium.org/624023003/diff/20001/core/src/fxcodec/fx_libo...
File core/src/fxcodec/fx_libopenjpeg/libopenjpeg20/jp2.c (right):

https://codereview.chromium.org/624023003/diff/20001/core/src/fxcodec/fx_libo...
core/src/fxcodec/fx_libopenjpeg/libopenjpeg20/jp2.c:1283: /*	ISO/IEC
15444-1:2004 (E), Table I.9  Legal METH values:
nit: looks to be an unintended whitespace only change.

[Bo Xu, 2014-10-03 18:41:30]

On 2014/10/03 18:38:26, Tom Sepez wrote:
> Heh.  I don't have permission to see the upstream issue.  I can review this on
> its own, though.  Stay tuned.

Basically they referred to this article: http://lwn.net/Articles/278137/
I am curious what would be the best way for this situation?

[Tom Sepez, 2014-10-03 18:49:41]

> Basically they referred to this article: http://lwn.net/Articles/278137/
> I am curious what would be the best way for this situation?
Generally, you want to represent sizes as size_t's and compare sizes against
each other, rather than making pointer comparisions ...

[Tom Sepez, 2014-10-03 18:56:21]

> Generally, you want to represent sizes as size_t's and compare sizes against
> each other, rather than making pointer comparisions ...
For example, for the loop in question, If I were coding this, I would keep track
of the space remaining in the buffer as its own size_t variable, and update it
as its used.  Then your test becomes something along the lines of 
if (lseg->newlen > remaining) { ...

[Bo Xu, 2014-10-03 19:08:22]

On 2014/10/03 18:56:21, Tom Sepez wrote:
> > Generally, you want to represent sizes as size_t's and compare sizes against
> > each other, rather than making pointer comparisions ...
> For example, for the loop in question, If I were coding this, I would keep
track
> of the space remaining in the buffer as its own size_t variable, and update it
> as its used.  Then your test becomes something along the lines of 
> if (lseg->newlen > remaining) { ...

That sounds a much better way to keep it in bound :)

[Bo Xu, 2014-10-03 19:08:38]

https://codereview.chromium.org/624023003/diff/20001/core/src/fxcodec/fx_libo...
File core/src/fxcodec/fx_libopenjpeg/libopenjpeg20/jp2.c (right):

https://codereview.chromium.org/624023003/diff/20001/core/src/fxcodec/fx_libo...
core/src/fxcodec/fx_libopenjpeg/libopenjpeg20/jp2.c:1283: /*	ISO/IEC
15444-1:2004 (E), Table I.9  Legal METH values:
On 2014/10/03 18:41:10, Tom Sepez wrote:
> nit: looks to be an unintended whitespace only change.

I got the warning of "the file contains a character that cannot be represented
in the current code page (936)" when I compiled in VS2013. Seems there is the
char that brought in from the openjpeg update.

[Tom Sepez, 2014-10-03 19:13:21]

> I got the warning of "the file contains a character that cannot be represented
> in the current code page (936)" when I compiled in VS2013. Seems there is the
> char that brought in from the openjpeg update.
That would explain it.  Thanks for fixing this.  Can you also ping them upstream
to correct this?

[Bo Xu, 2014-10-03 19:19:11]

On 2014/10/03 19:13:21, Tom Sepez wrote:
> > I got the warning of "the file contains a character that cannot be
represented
> > in the current code page (936)" when I compiled in VS2013. Seems there is
the
> > char that brought in from the openjpeg update.
> That would explain it.  Thanks for fixing this.  Can you also ping them
upstream
> to correct this?

Sure. I have reported to them.

[Bo Xu, 2014-10-03 19:29:55]

Committed patchset #2 (id:20001) manually as
e93d5341d87c54713a9632c8823288fa901a3b78 (presubmit successful).