Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1523523002:
Fix hint table loading issues. (Closed)
|
Created:
5 years ago by Oliver Chang Modified:
5 years ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix hint table loading issues.
BUG=566179
R=jun_fang@foxitsoftware.com, thestig@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/be8408f43bcfd69a74007a340a4c034004146c60
Patch Set 1 : #Patch Set 2 : nit #Patch Set 3 : nit #2 #Patch Set 4 : mistake #
Total comments: 4
Patch Set 5 : address comments #
Total comments: 8
Patch Set 6 : address more comments #Patch Set 7 : im dumb #
Total comments: 6
Messages
Total messages: 26 (6 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Fix hint table loading issues. BUG=566179 ========== to ========== Fix hint table loading issues. BUG=566179 R=jun_fang@foxitsoftware.com,thestig@chromium.org ==========
ochang@chromium.org changed reviewers: + jun_fang@foxitsoftware.com, thestig@chromium.org
PTAL.
oops, made a mistake. please see #4
https://codereview.chromium.org/1523523002/diff/80001/core/include/fxcrt/fx_b... File core/include/fxcrt/fx_basic.h (right): https://codereview.chromium.org/1523523002/diff/80001/core/include/fxcrt/fx_b... core/include/fxcrt/fx_basic.h:814: FX_DWORD GetPos() { return m_BitPos; } Both methods can be const? https://codereview.chromium.org/1523523002/diff/80001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1523523002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4702: hStream->BitsRemaining() < required_bits.ValueOrDie()) braces, ditto below. Maybe make a helper function? I know some consider them optional in this case, but it does get harder to read when the if statement spans multiple lines.
Patchset #5 (id:100001) has been deleted
Thanks for the review https://codereview.chromium.org/1523523002/diff/80001/core/include/fxcrt/fx_b... File core/include/fxcrt/fx_basic.h (right): https://codereview.chromium.org/1523523002/diff/80001/core/include/fxcrt/fx_b... core/include/fxcrt/fx_basic.h:814: FX_DWORD GetPos() { return m_BitPos; } On 2015/12/12 01:38:12, Lei Zhang wrote: > Both methods can be const? good point, done. https://codereview.chromium.org/1523523002/diff/80001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1523523002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4702: hStream->BitsRemaining() < required_bits.ValueOrDie()) On 2015/12/12 01:38:12, Lei Zhang wrote: > braces, ditto below. Maybe make a helper function? > > I know some consider them optional in this case, but it does get harder to read > when the if statement spans multiple lines. Made a helper. A lot cleaner now.
https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:78: bool CanReadFromBitStream(CFX_BitStream* hStream, |hStream| can now be a const ptr. https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4705: FX_SAFE_DWORD required_bits = dwDeltaObjectsBits; Can you add a line break above, so it's clear the 288 |kHeaderSize| is for the above section? Ditto below. https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4978: size < shared_hint_table_offset || !shared_hint_table_offset) { Can't you merge "!shared_hint_table_offset" with "shared_hint_table_offset < 0" above to form "shared_hint_table_offset <= 0" ?
Patchset #6 (id:140001) has been deleted
https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:78: bool CanReadFromBitStream(CFX_BitStream* hStream, On 2015/12/12 02:51:18, Lei Zhang wrote: > |hStream| can now be a const ptr. Done. https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4705: FX_SAFE_DWORD required_bits = dwDeltaObjectsBits; On 2015/12/12 02:51:18, Lei Zhang wrote: > Can you add a line break above, so it's clear the 288 |kHeaderSize| is for the > above section? Ditto below. Done. https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4978: size < shared_hint_table_offset || !shared_hint_table_offset) { On 2015/12/12 02:51:18, Lei Zhang wrote: > Can't you merge "!shared_hint_table_offset" with "shared_hint_table_offset < 0" > above to form "shared_hint_table_offset <= 0" ? Done.
lgtm https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:78: bool CanReadFromBitStream(CFX_BitStream* hStream, On 2015/12/12 03:02:07, Oliver Chang wrote: > On 2015/12/12 02:51:18, Lei Zhang wrote: > > |hStream| can now be a const ptr. > > Done. Sorry, I meant 'const CFX_BitStream*'
thanks, I'll wait for Jun to take a look before landing. https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1523523002/diff/120001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:78: bool CanReadFromBitStream(CFX_BitStream* hStream, On 2015/12/12 03:11:59, Lei Zhang wrote: > On 2015/12/12 03:02:07, Oliver Chang wrote: > > On 2015/12/12 02:51:18, Lei Zhang wrote: > > > |hStream| can now be a const ptr. > > > > Done. > > Sorry, I meant 'const CFX_BitStream*' Oops, mixed up my understanding of const * vs * const. I am not good at programming.
https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:78: bool CanReadFromBitStream(const CFX_BitStream* hStream, I don't think that we need to add an extra function to do checking. We can check the return value of CFX_BitStream::GetBits(). https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4708: if (!CanReadFromBitStream(hStream, required_bits)) It means a failure if the return of CFX_BitStream::GetBits() is 0. I don't think that we need to add an extra function to do checking. https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4798: hStream->SkipBits(safeTotalPageLen.ValueOrDie()); We should add a return value for SkipBits() like: FX_DWORD SkipBits(FX_DWORD nBits); 0: failed to skip nBits: skipped nBits https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4849: if (!CanReadFromBitStream(hStream, required_bits)) To check the return value of hStream->GetBits(). https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4891: hStream->SkipBits(dwSharedObjTotal); To check the return value of SkipBits() after we add a return value for it.
https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4708: if (!CanReadFromBitStream(hStream, required_bits)) On 2015/12/12 09:48:10, jun_fang wrote: > It means a failure if the return of CFX_BitStream::GetBits() is 0. > I don't think that we need to add an extra function to do checking. How do we distinguish between reading a value of 0 vs failure?
On 2015/12/12 15:07:26, Oliver Chang wrote: > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4708: if > (!CanReadFromBitStream(hStream, required_bits)) > On 2015/12/12 09:48:10, jun_fang wrote: > > It means a failure if the return of CFX_BitStream::GetBits() is 0. > > I don't think that we need to add an extra function to do checking. > > How do we distinguish between reading a value of 0 vs failure? If the input para is 0, it can be treated as another error. It doesn't mean anything to read 0 bytes from stream. Below is definition of GetBits(): FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { if (nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { return 0; }
On 2015/12/12 16:03:13, jun_fang wrote: > On 2015/12/12 15:07:26, Oliver Chang wrote: > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4708: if > > (!CanReadFromBitStream(hStream, required_bits)) > > On 2015/12/12 09:48:10, jun_fang wrote: > > > It means a failure if the return of CFX_BitStream::GetBits() is 0. > > > I don't think that we need to add an extra function to do checking. > > > > How do we distinguish between reading a value of 0 vs failure? > > If the input para is 0, it can be treated as another error. It doesn't mean > anything to read 0 bytes > from stream. > > Below is definition of GetBits(): > FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { > if (nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { > return 0; > } Maybe you can add a checker on whether nBits is 0. It can be: FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { if (nBits == 0 || nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { return 0; }
On 2015/12/12 16:05:00, jun_fang wrote: > On 2015/12/12 16:03:13, jun_fang wrote: > > On 2015/12/12 15:07:26, Oliver Chang wrote: > > > > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > > > > > > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4708: if > > > (!CanReadFromBitStream(hStream, required_bits)) > > > On 2015/12/12 09:48:10, jun_fang wrote: > > > > It means a failure if the return of CFX_BitStream::GetBits() is 0. > > > > I don't think that we need to add an extra function to do checking. > > > > > > How do we distinguish between reading a value of 0 vs failure? > > > > If the input para is 0, it can be treated as another error. It doesn't mean > > anything to read 0 bytes > > from stream. > > > > Below is definition of GetBits(): > > FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { > > if (nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { > > return 0; > > } > > Maybe you can add a checker on whether nBits is 0. It can be: > FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { > if (nBits == 0 || nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { > return 0; > } Sorry, what I meant is how do we distinguish between say GetBits(4) == 0 (where there is no error, the stream is just all 0s) vs GetBits(4) == 0 (where there is no data left in the stream.)
On 2015/12/12 16:06:58, Oliver Chang wrote: > On 2015/12/12 16:05:00, jun_fang wrote: > > On 2015/12/12 16:03:13, jun_fang wrote: > > > On 2015/12/12 15:07:26, Oliver Chang wrote: > > > > > > > > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > > > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > > > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4708: if > > > > (!CanReadFromBitStream(hStream, required_bits)) > > > > On 2015/12/12 09:48:10, jun_fang wrote: > > > > > It means a failure if the return of CFX_BitStream::GetBits() is 0. > > > > > I don't think that we need to add an extra function to do checking. > > > > > > > > How do we distinguish between reading a value of 0 vs failure? > > > > > > If the input para is 0, it can be treated as another error. It doesn't mean > > > anything to read 0 bytes > > > from stream. > > > > > > Below is definition of GetBits(): > > > FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { > > > if (nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { > > > return 0; > > > } > > > > Maybe you can add a checker on whether nBits is 0. It can be: > > FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { > > if (nBits == 0 || nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { > > return 0; > > } > > Sorry, what I meant is how do we distinguish between say > > GetBits(4) == 0 (where there is no error, the stream is just all 0s) > > vs > > GetBits(4) == 0 (where there is no data left in the stream.) Good question. In theory, 0 can be an error code and returned in CFX_BitStream::GetBits(), 0 shouldn't be in the valid data. Otherwise, we should change 0 to -1 as an error code. Let me double check whether 0 can be returned as data.
On 2015/12/12 16:25:19, jun_fang wrote: > On 2015/12/12 16:06:58, Oliver Chang wrote: > > On 2015/12/12 16:05:00, jun_fang wrote: > > > On 2015/12/12 16:03:13, jun_fang wrote: > > > > On 2015/12/12 15:07:26, Oliver Chang wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > > > > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > > > > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4708: if > > > > > (!CanReadFromBitStream(hStream, required_bits)) > > > > > On 2015/12/12 09:48:10, jun_fang wrote: > > > > > > It means a failure if the return of CFX_BitStream::GetBits() is 0. > > > > > > I don't think that we need to add an extra function to do checking. > > > > > > > > > > How do we distinguish between reading a value of 0 vs failure? > > > > > > > > If the input para is 0, it can be treated as another error. It doesn't > mean > > > > anything to read 0 bytes > > > > from stream. > > > > > > > > Below is definition of GetBits(): > > > > FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { > > > > if (nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { > > > > return 0; > > > > } > > > > > > Maybe you can add a checker on whether nBits is 0. It can be: > > > FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { > > > if (nBits == 0 || nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { > > > return 0; > > > } > > > > Sorry, what I meant is how do we distinguish between say > > > > GetBits(4) == 0 (where there is no error, the stream is just all 0s) > > > > vs > > > > GetBits(4) == 0 (where there is no data left in the stream.) > > Good question. In theory, 0 can be an error code and returned in > CFX_BitStream::GetBits(), > 0 shouldn't be in the valid data. Otherwise, we should change 0 to -1 as an > error code. > Let me double check whether 0 can be returned as data. In general I don't think it's a good idea to use a sentinel value to represent failure for something like reading a value from a bitstream. -1 (32 bits of 1s) could very well be a valid value too, no? The changes I was more concerned about were: 4952 if (dwIndex >= m_dwSharedObjNumArray.GetSize()) 4953 return IPDF_DataAvail::DataNotAvailable; and the addition of |shared_hint_table_offset|.
On 2015/12/12 16:33:27, Oliver Chang wrote: > On 2015/12/12 16:25:19, jun_fang wrote: > > On 2015/12/12 16:06:58, Oliver Chang wrote: > > > On 2015/12/12 16:05:00, jun_fang wrote: > > > > On 2015/12/12 16:03:13, jun_fang wrote: > > > > > On 2015/12/12 15:07:26, Oliver Chang wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > > > > > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1523523002/diff/180001/core/src/fpdfapi/fpdf_... > > > > > > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:4708: if > > > > > > (!CanReadFromBitStream(hStream, required_bits)) > > > > > > On 2015/12/12 09:48:10, jun_fang wrote: > > > > > > > It means a failure if the return of CFX_BitStream::GetBits() is 0. > > > > > > > I don't think that we need to add an extra function to do checking. > > > > > > > > > > > > How do we distinguish between reading a value of 0 vs failure? > > > > > > > > > > If the input para is 0, it can be treated as another error. It doesn't > > mean > > > > > anything to read 0 bytes > > > > > from stream. > > > > > > > > > > Below is definition of GetBits(): > > > > > FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { > > > > > if (nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { > > > > > return 0; > > > > > } > > > > > > > > Maybe you can add a checker on whether nBits is 0. It can be: > > > > FX_DWORD CFX_BitStream::GetBits(FX_DWORD nBits) { > > > > if (nBits == 0 || nBits > m_BitSize || m_BitPos + nBits > m_BitSize) { > > > > return 0; > > > > } > > > > > > Sorry, what I meant is how do we distinguish between say > > > > > > GetBits(4) == 0 (where there is no error, the stream is just all 0s) > > > > > > vs > > > > > > GetBits(4) == 0 (where there is no data left in the stream.) > > > > Good question. In theory, 0 can be an error code and returned in > > CFX_BitStream::GetBits(), > > 0 shouldn't be in the valid data. Otherwise, we should change 0 to -1 as an > > error code. > > Let me double check whether 0 can be returned as data. > > In general I don't think it's a good idea to use a sentinel value to represent > failure for something like reading a value from a bitstream. -1 (32 bits of 1s) > could very well be a valid value too, no? > > The changes I was more concerned about were: > > 4952 if (dwIndex >= m_dwSharedObjNumArray.GetSize()) > 4953 return IPDF_DataAvail::DataNotAvailable; > > and the addition of |shared_hint_table_offset|. You are right. Currently, we can't distinguish valid data (0) from error code (0). There is a potential issue in CFX_BitStream::GetBits(). It can't use 0 to indicate an error in reading bits. I don't have further comments on this CL. LGTM. Thanks!
Thanks Jun.
Description was changed from ========== Fix hint table loading issues. BUG=566179 R=jun_fang@foxitsoftware.com,thestig@chromium.org ========== to ========== Fix hint table loading issues. BUG=566179 R=jun_fang@foxitsoftware.com, thestig@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/be8408f43bcfd69a74007a340a4c03400414... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) manually as be8408f43bcfd69a74007a340a4c034004146c60 (presubmit successful).
Message was sent while issue was closed.
On 2015/12/14 17:13:31, Oliver Chang wrote: > Committed patchset #7 (id:180001) manually as > be8408f43bcfd69a74007a340a4c034004146c60 (presubmit successful). What actually landed does not match patch set 7.
Message was sent while issue was closed.
Huh? How did that happen? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
