Fix subtle issues in opj_skip_from_memory and add unit tests.

Fix subtle issues in opj_skip_from_memory and add unit tests.

Follow on to https://codereview.chromium.org/990683002/.
This more closely mimics what fseek() actually does, so as
to avoid subtle bugs down the road.

Move the DecodeData struct into a header so the test can
use it, and provide a constructor for it.

Along the way, I added include guards, removed the p_
prefix from some non-pointer vars, fixed some IWYU, and
resolved some signed/unsigned comparison warnings with
careful casting.

BUG=452671
R=jun_fang@foxitsoftware.com, thestig@chromium.org

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

[Tom Sepez, 2015-03-18 23:43:37]

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

[Tom Sepez, 2015-03-18 23:44:05]

Please review.  Jun, in particular when I suggested the prior patch needed a
unit test, this is what I had in mind.

[jun_fang, 2015-03-19 00:28:46]

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

https://codereview.chromium.org/1016203002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:84: srcData->offset =
std::min(srcData->offset + checkedNbBytes, srcData->src_size);
In this case, we should return the skipped bytes rather than the requested
bytes.

https://codereview.chromium.org/1016203002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:104: srcData->offset =
srcData->src_size;
Before the code change, it returns false when the requested position beyonds the
size of source data. Are you sure that you want to change   it?

[Tom Sepez, 2015-03-19 00:35:16]

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

https://codereview.chromium.org/1016203002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:84: srcData->offset =
std::min(srcData->offset + checkedNbBytes, srcData->src_size);
On 2015/03/19 00:28:46, jun_fang wrote:
> In this case, we should return the skipped bytes rather than the requested
> bytes.

Looking at the example code you pointed at before,

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;
}
This never returns a fractional result.  So I did the same.

https://codereview.chromium.org/1016203002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:104: srcData->offset =
srcData->src_size;
On 2015/03/19 00:28:46, jun_fang wrote:
> Before the code change, it returns false when the requested position beyonds
the
> size of source data. Are you sure that you want to change   it?

Looking at the example code again,
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;
}

and realizing that fseek() always allows seek beyond EOF, I'd say yes (it
returns in error only when things like a non-seekable fd are passed in).

[jun_fang, 2015-03-19 00:48:35]

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

https://codereview.chromium.org/1016203002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:84: srcData->offset =
std::min(srcData->offset + checkedNbBytes, srcData->src_size);
On 2015/03/19 00:35:16, Tom Sepez wrote:
> On 2015/03/19 00:28:46, jun_fang wrote:
> > In this case, we should return the skipped bytes rather than the requested
> > bytes.
> 
> Looking at the example code you pointed at before,
> 
> 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;
> }
> This never returns a fractional result.  So I did the same.
> 

I checked the code before. I suspect that there may be a problem in open jpeg
code if the skipped bytes are different with the requested ones. Anyway, we can
follow them this time.

https://codereview.chromium.org/1016203002/diff/20001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:104: srcData->offset =
srcData->src_size;
On 2015/03/19 00:35:16, Tom Sepez wrote:
> On 2015/03/19 00:28:46, jun_fang wrote:
> > Before the code change, it returns false when the requested position beyonds
> the
> > size of source data. Are you sure that you want to change   it?
> 
> Looking at the example code again,
> 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;
> }
> 
> and realizing that fseek() always allows seek beyond EOF, I'd say yes (it
> returns in error only when things like a non-seekable fd are passed in).

OK. let's follow the implemention in open jpeg.

[jun_fang, 2015-03-19 00:48:54]

On 2015/03/19 00:48:35, jun_fang wrote:
>
https://codereview.chromium.org/1016203002/diff/20001/core/src/fxcodec/codec/...
> File core/src/fxcodec/codec/fx_codec_jpx_opj.cpp (right):
> 
>
https://codereview.chromium.org/1016203002/diff/20001/core/src/fxcodec/codec/...
> core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:84: srcData->offset =
> std::min(srcData->offset + checkedNbBytes, srcData->src_size);
> On 2015/03/19 00:35:16, Tom Sepez wrote:
> > On 2015/03/19 00:28:46, jun_fang wrote:
> > > In this case, we should return the skipped bytes rather than the requested
> > > bytes.
> > 
> > Looking at the example code you pointed at before,
> > 
> > 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;
> > }
> > This never returns a fractional result.  So I did the same.
> > 
> 
> I checked the code before. I suspect that there may be a problem in open jpeg
> code if the skipped bytes are different with the requested ones. Anyway, we
can
> follow them this time.
> 
>
https://codereview.chromium.org/1016203002/diff/20001/core/src/fxcodec/codec/...
> core/src/fxcodec/codec/fx_codec_jpx_opj.cpp:104: srcData->offset =
> srcData->src_size;
> On 2015/03/19 00:35:16, Tom Sepez wrote:
> > On 2015/03/19 00:28:46, jun_fang wrote:
> > > Before the code change, it returns false when the requested position
beyonds
> > the
> > > size of source data. Are you sure that you want to change   it?
> > 
> > Looking at the example code again,
> > 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;
> > }
> > 
> > and realizing that fseek() always allows seek beyond EOF, I'd say yes (it
> > returns in error only when things like a non-seekable fd are passed in).
> 
> OK. let's follow the implemention in open jpeg.

LGTM now. Thanks!

[Lei Zhang, 2015-03-19 01:35:50]

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

[Lei Zhang, 2015-03-19 01:35:51]

lgtm

https://codereview.chromium.org/1016203002/diff/60001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpx_unittest.cpp (right):

https://codereview.chromium.org/1016203002/diff/60001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_unittest.cpp:17: static unsigned char
stream_data[] = {
can this be const?

[Tom Sepez, 2015-03-19 18:02:10]

https://codereview.chromium.org/1016203002/diff/60001/core/src/fxcodec/codec/...
File core/src/fxcodec/codec/fx_codec_jpx_unittest.cpp (right):

https://codereview.chromium.org/1016203002/diff/60001/core/src/fxcodec/codec/...
core/src/fxcodec/codec/fx_codec_jpx_unittest.cpp:17: static unsigned char
stream_data[] = {
On 2015/03/19 01:35:51, Lei Zhang wrote:
> can this be const?
Not without casting away const later due to the lack const in the interface
provided by DecodeData (its data member may be used for either input or output
depending on read vs. write)

[Tom Sepez, 2015-03-19 19:51:09]

Committed patchset #4 (id:60001) manually as
d1f792ac1ba25494b44e664d4a427127299d05e6 (presubmit successful).