openjpeg: Prevent an integer overflow in opj_jp2_apply_pclr.

openjpeg: Prevent an integer overflow in opj_jp2_apply_pclr.

This patch also prevent a null pointer access problem.

BUG=chromium:638829
R=ochang@chromium.org

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

[Ke Liu, 2016-08-24 03:10:04]

Description was changed from

==========
openjpeg: Prevent an integer overflow in opj_jp2_apply_pclr.

This patch also prevent a null pointer access problem.

BUG=chromium:638829
R=ochang@chromium.org
==========

to

==========
openjpeg: Prevent an integer overflow in opj_jp2_apply_pclr.

This patch also prevent a null pointer access problem.

BUG=chromium:638829
R=ochang@chromium.org
==========

[Ke Liu, 2016-08-24 03:10:04]

stackexploit@gmail.com changed reviewers:
+ thestig@chromium.org

[Ke Liu, 2016-08-24 03:17:07]

On 2016/08/24 03:10:04, Ke Liu wrote:
> mailto:stackexploit@gmail.com changed reviewers:
> + mailto:thestig@chromium.org

Thanks for Oliver's suggestion at https://codereview.chromium.org/2253423002/

1. The integer overflow check code has been moved to the front of function
opj_jp2_apply_pclr so that we can avoid redundant code which was used to free
memory blocks.
2. The code dst = new_comps[i].data; was moved to avoid redundant code.
3. Replace assert macro with if statement to avoid null pointer deference.

Thanks.

[Ke Liu, 2016-08-24 03:19:53]

Updated based on the previous cl ( https://codereview.chromium.org/2253423002/
).

[Oliver Chang, 2016-08-25 01:02:20]

https://codereview.chromium.org/2270343002/diff/1/third_party/libopenjpeg20/j...
File third_party/libopenjpeg20/jp2.c (right):

https://codereview.chromium.org/2270343002/diff/1/third_party/libopenjpeg20/j...
third_party/libopenjpeg20/jp2.c:976: for (i = 0; i < nr_channels; ++i) {
I know indentation is all over the place in this file, but could you make this
match the indentation in the previous lines?

https://codereview.chromium.org/2270343002/diff/1/third_party/libopenjpeg20/j...
third_party/libopenjpeg20/jp2.c:978: if (old_comps[cmp].h == 0 ||
old_comps[cmp].w > UINT_MAX / sizeof(OPJ_INT32) / old_comps[cmp].h) {
let's be explicit here and use ((OPJ_UINT32)-1) instead of UINT_MAX, since
"unsigned int" isn't actually defined to be 32 bits.

https://codereview.chromium.org/2270343002/diff/1/third_party/libopenjpeg20/j...
third_party/libopenjpeg20/jp2.c:1028: if (new_comps[j].data) {
you don't need to check if the pointer is null before passing it to free()

[Ke Liu, 2016-08-25 03:34:43]

On 2016/08/25 01:02:20, Oliver Chang (slow) wrote:
>
https://codereview.chromium.org/2270343002/diff/1/third_party/libopenjpeg20/j...
> File third_party/libopenjpeg20/jp2.c (right):
> 
>
https://codereview.chromium.org/2270343002/diff/1/third_party/libopenjpeg20/j...
> third_party/libopenjpeg20/jp2.c:976: for (i = 0; i < nr_channels; ++i) {
> I know indentation is all over the place in this file, but could you make this
> match the indentation in the previous lines?
> 
>
https://codereview.chromium.org/2270343002/diff/1/third_party/libopenjpeg20/j...
> third_party/libopenjpeg20/jp2.c:978: if (old_comps[cmp].h == 0 ||
> old_comps[cmp].w > UINT_MAX / sizeof(OPJ_INT32) / old_comps[cmp].h) {
> let's be explicit here and use ((OPJ_UINT32)-1) instead of UINT_MAX, since
> "unsigned int" isn't actually defined to be 32 bits.
> 
>
https://codereview.chromium.org/2270343002/diff/1/third_party/libopenjpeg20/j...
> third_party/libopenjpeg20/jp2.c:1028: if (new_comps[j].data) {
> you don't need to check if the pointer is null before passing it to free()

Thanks Oliver.
This cl has been updated according to your suggestion.

[Oliver Chang, 2016-08-25 16:07:58]

lgtm, thanks!

[Oliver Chang, 2016-08-29 18:47:17]

The CQ bit was checked by ochang@chromium.org

[commit-bot: I haz the power, 2016-08-29 18:47:27]

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-08-29 19:04:53]

Description was changed from

==========
openjpeg: Prevent an integer overflow in opj_jp2_apply_pclr.

This patch also prevent a null pointer access problem.

BUG=chromium:638829
R=ochang@chromium.org
==========

to

==========
openjpeg: Prevent an integer overflow in opj_jp2_apply_pclr.

This patch also prevent a null pointer access problem.

BUG=chromium:638829
R=ochang@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/c37d7d452d6a37c997c8709576dd71406ecf...
==========

[commit-bot: I haz the power, 2016-08-29 19:04:54]

Committed patchset #2 (id:20001) as
https://pdfium.googlesource.com/pdfium/+/c37d7d452d6a37c997c8709576dd71406ecf...