in the attempt to fix 627393, changed IFX_FileRead's readBlock to return the length it reads

in the attempt to fix 627393, changed IFX_FileRead's readBlock to return the length it reads

BUG=627393

[hong_zhang, 2016-10-19 03:01:40]

The CQ bit was checked by hong_zhang@foxitsoftware.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 03:01:46]

Dry run: 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-10-19 03:01:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 03:01:48]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-pdfium-committers".
Note that this has nothing to do with OWNERS files.

[hong_zhang, 2016-10-19 03:01:57]

The CQ bit was unchecked by hong_zhang@foxitsoftware.com

[hong_zhang, 2016-10-19 03:06:10]

hong_zhang@foxitsoftware.com changed reviewers:
+ ochang@chromium.org, thestig@chromium.org, tracy_jiang@foxitsoftware.com,
tsepez@chromium.org

[hong_zhang, 2016-10-19 03:06:11]

Hi all,

As mentioned in the email, I changes the IFX_FileRead's ReadBlock to return a
length it reads. PTAL.

Best Regards,
-Hong

[Tom Sepez, 2016-10-19 23:12:48]

This is all good, and should solve a lot of potential problems.  All that
remains is to think carefully about signed/unsigned values, due to the
restrictions C++ inflicts on us.

https://codereview.chromium.org/2430743003/diff/20001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_data_avail.cpp (right):

https://codereview.chromium.org/2430743003/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_data_avail.cpp:1192: if
(m_pFileRead->ReadBlock(pBuf, m_dwTrailerOffset, iSize) != iSize &&
this may trigger a signed/unsigned comparision warning on some compilers.  iSize
may need to be uint32_t, along with a check for underflow at line 1184.

https://codereview.chromium.org/2430743003/diff/20001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_parser_unittest.cpp (right):

https://codereview.chromium.org/2430743003/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_parser_unittest.cpp:25: FX_BOOL IsEOF() override {
return m_nCurPos >= total_size_; }
We may get a signed vs unsigned comparison error on some compilers.

https://codereview.chromium.org/2430743003/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_parser_unittest.cpp:25: FX_BOOL IsEOF() override {
return m_nCurPos >= total_size_; }
nit: can we make this just a |bool|, we've been trying to reduce dependence of
FX_BOOL.

https://codereview.chromium.org/2430743003/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_parser_unittest.cpp:39: size_t total_size_;
|total_size_| probably needs to be a FX_FILESIZE as well, to skirt the
signed/unsigned comparison above.

https://codereview.chromium.org/2430743003/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_parser_unittest.cpp:40: FX_FILESIZE m_nCurPos;
nit: use pre-existing convention, eg. current_pos_

https://codereview.chromium.org/2430743003/diff/20001/testing/libfuzzer/xfa_c...
File testing/libfuzzer/xfa_codec_fuzzer.h (right):

https://codereview.chromium.org/2430743003/diff/20001/testing/libfuzzer/xfa_c...
testing/libfuzzer/xfa_codec_fuzzer.h:71: size_t m_size;
this may be cleaner if it were FX_FILESIZE as well.

[hong_zhang, 2016-11-12 00:41:22]

Dear all, I made a couple changes addressing Tom's previous comments, and also
merged newer code base. PTAL.

[Tom Sepez, 2016-11-14 20:27:40]

Pls take another look at this, I suspect any place we're not capturing the
return value for later use is going to be wrong.  Thanks.

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/edit/fpdf_...
File core/fpdfapi/edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/edit/fpdf_...
core/fpdfapi/edit/fpdf_edit_create.cpp:1502: if (m_File.AppendBlock(buffer,
block_size) < 0) {
we could have a short read here, yet we appended the full block_size.

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_data_avail.cpp (right):

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_data_avail.cpp:1080: ScopedFileStream
file(FX_CreateMemoryStream(pBuf, (size_t)iSize, false));
same issue here with reads < isize.

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_parser.cpp (right):

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_parser.cpp:606: uint8_t byte = buffer[i];
and here.

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_parser_unittest.cpp (right):

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_parser_unittest.cpp:24: // IFX_FileRead
nit: I renamed IFX_FileRead to IFX_SeekableReadStream a few weeks ago because it
was bugging me that something called a "File" Read might be a memory-only
stream.

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_parser_unittest.cpp:29: offset +
pdfium::base::checked_cast<FX_FILESIZE>(size) > total_size_) {
note: this addition can overflow, but its only a test so its probably fine.

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_syntax_parser.cpp (right):

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_syntax_parser.cpp:115: m_Pos += size;
and here.

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/fpd...
File core/fpdfapi/parser/fpdf_parser_utility.cpp (right):

https://codereview.chromium.org/2430743003/diff/60001/core/fpdfapi/parser/fpd...
core/fpdfapi/parser/fpdf_parser_utility.cpp:78: if (memcmp(buf, "%PDF", 4) == 0)
might not have read 4 bytes if EOF.

[hong_zhang, 2016-11-15 02:01:32]

Patchset #6 (id:100001) has been deleted

[hong_zhang, 2016-11-15 02:13:22]

We took care of the short read issue. PTAL.

[Tom Sepez, 2016-11-16 18:32:01]

Getting closer, but still some bugs I think.

https://codereview.chromium.org/2430743003/diff/120001/core/fpdfapi/parser/cp...
File core/fpdfapi/parser/cpdf_stream.cpp (right):

https://codereview.chromium.org/2430743003/diff/120001/core/fpdfapi/parser/cp...
core/fpdfapi/parser/cpdf_stream.cpp:108: return m_pFile->ReadBlock(buf, offset,
size) == size || m_pFile->IsEOF();
This will return success if a short read at EOF.  Its caller at
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/parser/c...
will continue but will use the full length at
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/parser/c...
for example.

https://codereview.chromium.org/2430743003/diff/120001/core/fxcodec/codec/fx_...
File core/fxcodec/codec/fx_codec_progress.cpp (right):

https://codereview.chromium.org/2430743003/diff/120001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:1207: pPngModule->Input(m_pPngContext,
m_pSrcBuf, readSize, pAttribute);
I think we want to issue reads for the full size even if we previously got a
short read here.

This is the last statement of the loop, and when changed to a full read, can be
combined with line 1182:

while (pPngModule->Input(m_pPngContext, m_pSrcBuf, size, pAttribute)) {

https://codereview.chromium.org/2430743003/diff/120001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:1209: ASSERT(!bResult);
This is the only use of bResult outside the loop, and there aren't any breaks in
the loop, so we can remove this line and bResult entirely.

https://codereview.chromium.org/2430743003/diff/120001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:1246: pGifModule->Input(m_pGifContext,
m_pSrcBuf, size);
Again, do we want a full block read here?

https://codereview.chromium.org/2430743003/diff/120001/core/fxcodec/codec/fx_...
core/fxcodec/codec/fx_codec_progress.cpp:2156: bool bResult =
nit: local not needed.

https://codereview.chromium.org/2430743003/diff/120001/core/fxge/android/cfpf...
File core/fxge/android/cfpf_skiafontmgr.cpp (right):

https://codereview.chromium.org/2430743003/diff/120001/core/fxge/android/cfpf...
core/fxge/android/cfpf_skiafontmgr.cpp:41: return readSize;
readSize has gone out of scope. Did this compile?

https://codereview.chromium.org/2430743003/diff/120001/fpdfsdk/fpdf_dataavail...
File fpdfsdk/fpdf_dataavail.cpp (right):

https://codereview.chromium.org/2430743003/diff/120001/fpdfsdk/fpdf_dataavail...
fpdfsdk/fpdf_dataavail.cpp:77: // IFX_FileRead
nit: now called IFX_SeekableReadStream

https://codereview.chromium.org/2430743003/diff/120001/fpdfsdk/fpdfview.cpp
File fpdfsdk/fpdfview.cpp (right):

https://codereview.chromium.org/2430743003/diff/120001/fpdfsdk/fpdfview.cpp#n...
fpdfsdk/fpdfview.cpp:206: m_FileAccess.m_GetBlock(m_FileAccess.m_Param, offset,
(uint8_t*)buffer, size);
We probably still need to check the error status of this call?

[Tom Sepez, 2016-11-18 21:55:55]

Also, I was concerned that we didn't have any unit tests for this, so I'm in the
process of adding at least one at 
https://codereview.chromium.org/2517513003

[hong_zhang, 2016-11-18 23:46:29]

On 2016/11/18 21:55:55, Tom Sepez wrote:
> Also, I was concerned that we didn't have any unit tests for this, so I'm in
the
> process of adding at least one at 
> https://codereview.chromium.org/2517513003

Sure we will make sure to test with the unit test.