Fix a bug that JPX images can't be shown

Fix a bug that JPX images can't be shown

In the process of opj_end_decompress, it will return fail when the end of coding stream is reached. However it returns true in the same scenario implemented in openJPEG. So the final solution is from openJPEG. Return true when the end of coding stream is reached.

BUG=452671
R=tsepez@chromium.org

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

[jun_fang, 2015-03-08 01:54:09]

jun_fang@foxitsoftware.com changed reviewers:
+ kai_jing@foxitsoftware.com, thestig@chromium.org, tsepez@chromium.org

[jun_fang, 2015-03-08 01:54:09]

Hi Tom, 

Please start to review this change.

[jun_fang, 2015-03-09 00:02:06]

On 2015/03/08 01:54:09, jun_fang wrote:
> Hi Tom, 
> 
> Please start to review this change.

I find more clues on this issue. Please hold it on and wait for my new patch.
Thanks!

[jun_fang, 2015-03-10 22:40:39]

On 2015/03/09 00:02:06, jun_fang wrote:
> On 2015/03/08 01:54:09, jun_fang wrote:
> > Hi Tom, 
> > 
> > Please start to review this change.
> 
> I find more clues on this issue. Please hold it on and wait for my new patch.
> Thanks!

In the process of opj_end_decompress, opj_skip_from_memory is called in
pdfium/chrome to skip some data.
It always return -1 when it reaches the end of stream. In the same process in
openJPEG, the similar 
function opj_skip_from_file is called and it returns p_nb_bytes even when it
detects the end of file. 
So this issue reproduces in pdfium/chrome but not in openjpeg.

in openjpeg\src\lib\openjp2\openjpeg.c

static OPJ_OFF_T opj_skip_from_file (OPJ_OFF_T p_nb_bytes, FILE * p_user_data)
{
	int ret = OPJ_FSEEK(p_user_data,p_nb_bytes,SEEK_CUR);
	if (ret) {
		return -1;
	}

	return p_nb_bytes;
}

static OPJ_BOOL opj_seek_from_file (OPJ_OFF_T p_nb_bytes, FILE * p_user_data)
{
	if (OPJ_FSEEK(p_user_data,p_nb_bytes,SEEK_SET)) {
		return OPJ_FALSE;
	}

	return OPJ_TRUE;
}

[jun_fang, 2015-03-10 22:43:31]

On 2015/03/10 22:40:39, jun_fang wrote:
> On 2015/03/09 00:02:06, jun_fang wrote:
> > On 2015/03/08 01:54:09, jun_fang wrote:
> > > Hi Tom, 
> > > 
> > > Please start to review this change.
> > 
> > I find more clues on this issue. Please hold it on and wait for my new
patch.
> > Thanks!
> 
> In the process of opj_end_decompress, opj_skip_from_memory is called in
> pdfium/chrome to skip some data.
> It always return -1 when it reaches the end of stream. In the same process in
> openJPEG, the similar 
> function opj_skip_from_file is called and it returns p_nb_bytes even when it
> detects the end of file. 
> So this issue reproduces in pdfium/chrome but not in openjpeg.
> 
> in openjpeg\src\lib\openjp2\openjpeg.c
> 
> static OPJ_OFF_T opj_skip_from_file (OPJ_OFF_T p_nb_bytes, FILE * p_user_data)
> {
> 	int ret = OPJ_FSEEK(p_user_data,p_nb_bytes,SEEK_CUR);
> 	if (ret) {
> 		return -1;
> 	}
> 
> 	return p_nb_bytes;
> }
> 
> static OPJ_BOOL opj_seek_from_file (OPJ_OFF_T p_nb_bytes, FILE * p_user_data)
> {
> 	if (OPJ_FSEEK(p_user_data,p_nb_bytes,SEEK_SET)) {
> 		return OPJ_FALSE;
> 	}
> 
> 	return OPJ_TRUE;
> }

The opj_skip_from_file in openjpeg shall be shown as below. I added "ret" in
this function for debugging.

static OPJ_OFF_T opj_skip_from_file (OPJ_OFF_T p_nb_bytes, FILE * p_user_data)
{
	if (OPJ_FSEEK(p_user_data,p_nb_bytes,SEEK_CUR)) {
		return -1;
	}

	return p_nb_bytes;
}

[Tom Sepez, 2015-03-11 18:15:23]

Makes sense.  Posix fseek() says that fseeking beyond EOF is ok:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fseek.html

[Tom Sepez, 2015-03-11 18:25:41]

https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:59: return p_nb_bytes;
I think we have to adjust the offset here, for example imagine a "file"
of size 1024 with a current offset of 1000, and we do a skip of 100. A
subsequent read should then return 0 bytes, but we'd give the remaining 24.

It gets worse should we allow negative seeks (is OPJ_OFF_T signed?), in which
case we'd have to handle seeking back into a valid portion of the "file" at the
correct offset even after seeking past EOF. This kind of implies that the
invariant that srcData->offset < srcData->src_size can't hold, which is
unfortunate because it feels safer that way. 

Let's assume that we don't have to handle negative seeks, and just set
srcData->offset to srcData->size here.

[Tom Sepez, 2015-03-11 18:29:13]

https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:43: if (srcData == NULL ||
srcData->src_size == 0 || srcData->src_data == NULL || srcData->offset >=
srcData->src_size) {
Also, per the spec, a fseek() beyond EOF followed by a write produces a region
read back as 0s from the "file".  I'm not sure we care.

[Tom Sepez, 2015-03-11 19:00:13]

This is also the kind of thing that is well-suited for unit tests.
If I were working on this, the first thing I would do is to add
fx_codec_jpx_opj_unittest.cpp, putting tests in it that show that
the current implementation doesn't match the posix semantics, and
then correct the code to match.

[jun_fang, 2015-03-11 21:14:42]

https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:43: if (srcData == NULL ||
srcData->src_size == 0 || srcData->src_data == NULL || srcData->offset >=
srcData->src_size) {
On 2015/03/11 18:29:12, Tom Sepez wrote:
> Also, per the spec, a fseek() beyond EOF followed by a write produces a region
> read back as 0s from the "file".  I'm not sure we care.

I think that the checking |srcData->offset >= srcData->src_size| is used to
ensure |p_buffer + writeLength| not to overflow.

https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:59: return p_nb_bytes;
On 2015/03/11 18:25:41, Tom Sepez wrote:
> I think we have to adjust the offset here, for example imagine a "file"
> of size 1024 with a current offset of 1000, and we do a skip of 100. A
> subsequent read should then return 0 bytes, but we'd give the remaining 24.
> 
> It gets worse should we allow negative seeks (is OPJ_OFF_T signed?), in which
> case we'd have to handle seeking back into a valid portion of the "file" at
the
> correct offset even after seeking past EOF. This kind of implies that the
> invariant that srcData->offset < srcData->src_size can't hold, which is
> unfortunate because it feels safer that way. 
> 
> Let's assume that we don't have to handle negative seeks, and just set
> srcData->offset to srcData->size here. 

Acknowledged.

[jun_fang, 2015-03-13 00:15:44]

On 2015/03/11 21:14:42, jun_fang wrote:
>
https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
> File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):
> 
>
https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
> core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:43: if (srcData == NULL ||
> srcData->src_size == 0 || srcData->src_data == NULL || srcData->offset >=
> srcData->src_size) {
> On 2015/03/11 18:29:12, Tom Sepez wrote:
> > Also, per the spec, a fseek() beyond EOF followed by a write produces a
region
> > read back as 0s from the "file".  I'm not sure we care.
> 
> I think that the checking |srcData->offset >= srcData->src_size| is used to
> ensure |p_buffer + writeLength| not to overflow.
> 
>
https://codereview.chromium.org/990683002/diff/20001/core/src/fxcodec/codec/f...
> core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:59: return p_nb_bytes;
> On 2015/03/11 18:25:41, Tom Sepez wrote:
> > I think we have to adjust the offset here, for example imagine a "file"
> > of size 1024 with a current offset of 1000, and we do a skip of 100. A
> > subsequent read should then return 0 bytes, but we'd give the remaining 24.
> > 
> > It gets worse should we allow negative seeks (is OPJ_OFF_T signed?), in
which
> > case we'd have to handle seeking back into a valid portion of the "file" at
> the
> > correct offset even after seeking past EOF. This kind of implies that the
> > invariant that srcData->offset < srcData->src_size can't hold, which is
> > unfortunate because it feels safer that way. 
> > 
> > Let's assume that we don't have to handle negative seeks, and just set
> > srcData->offset to srcData->size here. 
> 
> Acknowledged.

Hi Tom,

Does it look good to you?

[Tom Sepez, 2015-03-16 17:16:09]

> Hi Tom,
> 
> Does it look good to you?

Code looks good, but really needs its own unit test.  Thanks.

[Tom Sepez, 2015-03-16 17:24:52]

https://codereview.chromium.org/990683002/diff/40001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/990683002/diff/40001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:598: l_stream =
fx_opj_stream_create_memory_stream(&srcData, OPJ_J2K_STREAM_CHUNK_SIZE, 1);
Doesn't srcData go out of scope and get destroyed while l_stream might still be
active?  Guess not based on line 649, but we can return with l_stream non-NULL
under error conditions?

https://codereview.chromium.org/990683002/diff/40001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:661: //if(image->icc_profile_buf &&
!image->useColorSpace) {
nit: kill dead code while we're at it.

[Tom Sepez, 2015-03-16 17:51:21]

If I were making a unit test, I'd call it fx_codec_jpx_unittest.cpp, and it
would live in this directory.

https://codereview.chromium.org/990683002/diff/40001/core/src/fxcodec/codec/f...
File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):

https://codereview.chromium.org/990683002/diff/40001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:23: struct DecodeData {
So if I'm making a unit test, this declaration moves to codec_int.h

https://codereview.chromium.org/990683002/diff/40001/core/src/fxcodec/codec/f...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:28: static OPJ_SIZE_T
opj_read_from_memory (void * p_buffer, OPJ_SIZE_T p_nb_bytes,  void*
p_user_data)
So if I'm making a unit test, these are not longer static functions, but
externals, and are prototpyed in codec_int.h

[Tom Sepez, 2015-03-16 17:51:28]

lgtm

[Tom Sepez, 2015-03-16 17:58:12]

On 2015/03/16 17:51:28, Tom Sepez wrote:
> lgtm

Argh, premature LGTM.  No matter, If you land this, we can add the unit test
later.

[jun_fang, 2015-03-16 18:11:21]

On 2015/03/16 17:58:12, Tom Sepez wrote:
> On 2015/03/16 17:51:28, Tom Sepez wrote:
> > lgtm
> 
> Argh, premature LGTM.  No matter, If you land this, we can add the unit test
> later.

OK.Thanks!

[jun_fang, 2015-03-16 18:23:02]

Committed patchset #3 (id:40001) manually as
cd3c4764b87323ec7b712e537d18292d849611b9 (presubmit successful).