Fix memory leak in opj_j2k_merge_ppt.

Fix memory leak in opj_j2k_merge_ppt.

Replace the assert macro with if statement.

BUG=chromium:614691
R=dsinclair@chromium.org, ochang@chromium.org

[Ke Liu, 2016-09-19 06:18:50]

Please review my patch.

I think there are two ways to fix this issue. Currently I used the first method.

1. Replace the assert macro with if statement. Just return OPJ_FALSE once
p_tcp->ppt_buffer if not null.

2. If p_tcp->ppt_buffer if not null, call opj_free(p_tcp->ppt_buffer) before
malloc memory again.

[dsinclair, 2016-09-19 12:59:01]

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

https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
third_party/libopenjpeg20/j2k.c:3811: if(p_tcp->ppt_buffer != NULL) {
Have we verified this can ever happen? It's a precondition that ppt_buffer is
NULL coming into this method, so the callers should be verifying that the buffer
is NULL before making the call.

Can we take a look at the callers to determine if they are doing this check,
instead of changing the assert here.

[Ke Liu, 2016-09-19 13:46:01]

On 2016/09/19 12:59:01, dsinclair wrote:
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> File third_party/libopenjpeg20/j2k.c (right):
> 
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> third_party/libopenjpeg20/j2k.c:3811: if(p_tcp->ppt_buffer != NULL) {
> Have we verified this can ever happen? It's a precondition that ppt_buffer is
> NULL coming into this method, so the callers should be verifying that the
buffer
> is NULL before making the call.
> 
> Can we take a look at the callers to determine if they are doing this check,
> instead of changing the assert here.

Yes, the issue can be truly happen. The caller did not check whether the buffer
is NULL or not. Please take a look at the original post of this issue (
https://bugs.chromium.org/p/chromium/issues/detail?id=614691 ), I uploaded a
test pdf file just before the #1 comment.

[dsinclair, 2016-09-19 14:02:03]

On 2016/09/19 13:46:01, Ke Liu wrote:
> On 2016/09/19 12:59:01, dsinclair wrote:
> >
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> > File third_party/libopenjpeg20/j2k.c (right):
> > 
> >
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> > third_party/libopenjpeg20/j2k.c:3811: if(p_tcp->ppt_buffer != NULL) {
> > Have we verified this can ever happen? It's a precondition that ppt_buffer
is
> > NULL coming into this method, so the callers should be verifying that the
> buffer
> > is NULL before making the call.
> > 
> > Can we take a look at the callers to determine if they are doing this check,
> > instead of changing the assert here.
> 
> Yes, the issue can be truly happen. The caller did not check whether the
buffer
> is NULL or not. Please take a look at the original post of this issue (
> https://bugs.chromium.org/p/chromium/issues/detail?id=614691 ), I uploaded a
> test pdf file just before the #1 comment.


So, can we update the call sites to perform the correct checks? Is it just the
one site which got this wrong are are there several?

[Ke Liu, 2016-09-19 14:15:37]

On 2016/09/19 14:02:03, dsinclair wrote:
> On 2016/09/19 13:46:01, Ke Liu wrote:
> > On 2016/09/19 12:59:01, dsinclair wrote:
> > >
> >
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> > > File third_party/libopenjpeg20/j2k.c (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> > > third_party/libopenjpeg20/j2k.c:3811: if(p_tcp->ppt_buffer != NULL) {
> > > Have we verified this can ever happen? It's a precondition that ppt_buffer
> is
> > > NULL coming into this method, so the callers should be verifying that the
> > buffer
> > > is NULL before making the call.
> > > 
> > > Can we take a look at the callers to determine if they are doing this
check,
> > > instead of changing the assert here.
> > 
> > Yes, the issue can be truly happen. The caller did not check whether the
> buffer
> > is NULL or not. Please take a look at the original post of this issue (
> > https://bugs.chromium.org/p/chromium/issues/detail?id=614691 ), I uploaded a
> > test pdf file just before the #1 comment.
> 
> 
> So, can we update the call sites to perform the correct checks? Is it just the
> one site which got this wrong are are there several?

I've checked that there is only one line of code like assert(p_tcp->ppt_buffer
== NULL). I think the possible reason is that function opj_j2k_merge_ppt was
called more than one time, p_tcp->ppt_buffer was not NULL when the function was
called twice.

I will check the details tomorrow since it's midnight in my country. Of course,
you can take a look at it if you are available since a proof-of-concept file was
supplied.

[dsinclair, 2016-09-19 14:36:38]

On 2016/09/19 14:15:37, Ke Liu wrote:
> On 2016/09/19 14:02:03, dsinclair wrote:
> > On 2016/09/19 13:46:01, Ke Liu wrote:
> > > On 2016/09/19 12:59:01, dsinclair wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> > > > File third_party/libopenjpeg20/j2k.c (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> > > > third_party/libopenjpeg20/j2k.c:3811: if(p_tcp->ppt_buffer != NULL) {
> > > > Have we verified this can ever happen? It's a precondition that
ppt_buffer
> > is
> > > > NULL coming into this method, so the callers should be verifying that
the
> > > buffer
> > > > is NULL before making the call.
> > > > 
> > > > Can we take a look at the callers to determine if they are doing this
> check,
> > > > instead of changing the assert here.
> > > 
> > > Yes, the issue can be truly happen. The caller did not check whether the
> > buffer
> > > is NULL or not. Please take a look at the original post of this issue (
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=614691 ), I uploaded
a
> > > test pdf file just before the #1 comment.
> > 
> > 
> > So, can we update the call sites to perform the correct checks? Is it just
the
> > one site which got this wrong are are there several?
> 
> I've checked that there is only one line of code like assert(p_tcp->ppt_buffer
> == NULL). I think the possible reason is that function opj_j2k_merge_ppt was
> called more than one time, p_tcp->ppt_buffer was not NULL when the function
was
> called twice.
> 
> I will check the details tomorrow since it's midnight in my country. Of
course,
> you can take a look at it if you are available since a proof-of-concept file
was
> supplied.



Right, I don't expect the caller to assert, but I'd expect them to do the if()
check and take appropriate action. For some reason the caller is calling with
the buffer != NULL when it shouldn't be so it seems like the bug is further up
the stack then opj_j2k_merge_ppt.

[Ke Liu, 2016-09-20 13:21:58]

On 2016/09/19 14:36:38, dsinclair wrote:
> On 2016/09/19 14:15:37, Ke Liu wrote:
> > On 2016/09/19 14:02:03, dsinclair wrote:
> > > On 2016/09/19 13:46:01, Ke Liu wrote:
> > > > On 2016/09/19 12:59:01, dsinclair wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> > > > > File third_party/libopenjpeg20/j2k.c (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2353463002/diff/1/third_party/libopenjpeg20/j...
> > > > > third_party/libopenjpeg20/j2k.c:3811: if(p_tcp->ppt_buffer != NULL) {
> > > > > Have we verified this can ever happen? It's a precondition that
> ppt_buffer
> > > is
> > > > > NULL coming into this method, so the callers should be verifying that
> the
> > > > buffer
> > > > > is NULL before making the call.
> > > > > 
> > > > > Can we take a look at the callers to determine if they are doing this
> > check,
> > > > > instead of changing the assert here.
> > > > 
> > > > Yes, the issue can be truly happen. The caller did not check whether the
> > > buffer
> > > > is NULL or not. Please take a look at the original post of this issue (
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=614691 ), I
uploaded
> a
> > > > test pdf file just before the #1 comment.
> > > 
> > > 
> > > So, can we update the call sites to perform the correct checks? Is it just
> the
> > > one site which got this wrong are are there several?
> > 
> > I've checked that there is only one line of code like
assert(p_tcp->ppt_buffer
> > == NULL). I think the possible reason is that function opj_j2k_merge_ppt was
> > called more than one time, p_tcp->ppt_buffer was not NULL when the function
> was
> > called twice.
> > 
> > I will check the details tomorrow since it's midnight in my country. Of
> course,
> > you can take a look at it if you are available since a proof-of-concept file
> was
> > supplied.
> 
> 
> 
> Right, I don't expect the caller to assert, but I'd expect them to do the if()
> check and take appropriate action. For some reason the caller is calling with
> the buffer != NULL when it shouldn't be so it seems like the bug is further up
> the stack then opj_j2k_merge_ppt.

Hi dsinclair, I'm not sure how to fix it if we should patch other functions
other than opj_j2k_merge_ppt. You can figure out what's going on by
investigating the proof-of-concept file uploaded by me. So if you have better
ideas, please close this CL and open another one. Thanks.

[dsinclair, 2017-05-10 14:19:28]

Description was changed from

==========
Fix memory leak in opj_j2k_merge_ppt.

Replace the assert macro with if statement.

BUG=chromium:614691
R=dsinclair@chromium.org, ochang@chromium.org
==========

to

==========
Fix memory leak in opj_j2k_merge_ppt.

Replace the assert macro with if statement.

BUG=chromium:614691
R=dsinclair@chromium.org, ochang@chromium.org
==========

[dsinclair, 2017-05-10 14:19:28]

dsinclair@chromium.org changed reviewers:
- dsinclair@chromium.org