Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1476163002:
Add basic checking for RebuildCrossRef (Closed)
|
Created:
5 years ago by Wei Li Modified:
5 years ago CC:
pdfium-reviews_googlegroups.com, kai_jing Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd basic checking for RebuildCrossRef
RebuildCrossRef function returns false when we can not find file trailer
or any indirect object. This serves as a basic file format checking.
BUG=pdfium:215
R=jun_fang@foxitsoftware.com
Committed: https://pdfium.googlesource.com/pdfium/+/f14da1d58e8e12633c7a47e6efd5ffe43bb37b4b
Patch Set 1 : #
Total comments: 4
Patch Set 2 : change to explict checking #Patch Set 3 : remove unnecessary local variables #
Total comments: 2
Patch Set 4 : add root object checking #
Total comments: 8
Patch Set 5 : not checking root obj #
Messages
Total messages: 16 (4 generated)
Patchset #1 (id:1) has been deleted
weili@chromium.org changed reviewers: + jun_fang@foxitsoftware.com, thestig@chromium.org
PTAL, thanks
Description was changed from ========== Add basic checking for RebuildCrossRef RebuildCrossRef function returns false when we can not find file trailer or any indirect object. This serves as a basic file format checking. BUG=PDFium:215 ========== to ========== Add basic checking for RebuildCrossRef RebuildCrossRef function returns false when we can not find file trailer or any indirect object. This serves as a basic file format checking. BUG=pdfium:215 ==========
Looks good, but Jun should sign off on it as well. https://codereview.chromium.org/1476163002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1476163002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:253: RebuildCrossRef(); Should we check the result, now that it can actually return false? https://codereview.chromium.org/1476163002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1614: RebuildCrossRef(); Ditto. In fact, this block looks a lot like the one above...
thanks, I will wait for Jun's comments as well. https://codereview.chromium.org/1476163002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1476163002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:253: RebuildCrossRef(); On 2015/11/26 00:42:41, Lei Zhang wrote: > Should we check the result, now that it can actually return false? Done. https://codereview.chromium.org/1476163002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1614: RebuildCrossRef(); On 2015/11/26 00:42:41, Lei Zhang wrote: > Ditto. In fact, this block looks a lot like the one above... Done. Yes, some might be put into common functions.
LGTM with a nit. https://codereview.chromium.org/1476163002/diff/60001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1476163002/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:252: if (!RebuildCrossRef() || GetRootObjNum() == 0) nit: No need to check the return value of GetRootObjNum(). If RebuildCrossRef() returns true, GetRootObjNum() must be larger than 0. https://codereview.chromium.org/1476163002/diff/60001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1610: if (!RebuildCrossRef() || GetRootObjNum() == 0) Nit: no need to check the return value of GetRootObjNum().
Hi, Jun: The previous change didn't check for root object. So it still returns true even if the root obj num is 0. Now I moved root object checking inside RebuildCrossRef function so there is no need to check every time. PTAL, thanks.
On 2015/11/30 18:23:11, Wei wrote: > Hi, Jun: > > The previous change didn't check for root object. So it still returns true > even if the root obj num is 0. Now I moved root object checking inside > RebuildCrossRef function so there is no need to check every time. > PTAL, thanks. Hi Wei, When I double checked RebuildCrossRef, I found that it was no need to check the return of RebuildCrossRef() if we can make sure the object number of the root is not 0. I would like to give you some comments at the related code to clarify it.
https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (left): https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1612: if (RootObjNum == 0) { It's a trigger to rebuild cross reference table when RootObjNum is 0. After the cross reference is rebuilt, only checking RootObjNum is enough to verify whether RebuildCrossRef succeeded. If RootObjNum is larger than 0, m_pTrailer should not be nullptr. Please check the details in CPDF_Parser::GetRootObjNum(). Also, we have at least one object, the root object, in m_CrossRef. FX_DWORD CPDF_Parser::GetRootObjNum() { CPDF_Reference* pRef = ToReference( m_pTrailer ? m_pTrailer->GetElement(FX_BSTRC("Root")) : nullptr); return pRef ? pRef->GetRefObjNum() : 0; }
https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:975: return m_pTrailer && GetRootObjNum() > 0 && m_CrossRef.GetSize() > 0; The newly added condition ' GetRootObjNum() > 0' will change the existing behavior. Please restore back to "m_pTrailer && m_CrossRef.GetSize() > 0". Thanks! https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1610: if (!RebuildCrossRef()) Based on the analysis, I prefer to keep left code unchanged. It's better to add a comment to explain why it's no need to check the return of RebuildCrossRef().
https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:252: if (!RebuildCrossRef()) please check the comment at line 1610.
https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (left): https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1612: if (RootObjNum == 0) { On 2015/12/01 14:56:20, jun_fang wrote: > It's a trigger to rebuild cross reference table when RootObjNum is 0. > After the cross reference is rebuilt, only checking RootObjNum is enough to > verify whether RebuildCrossRef succeeded. If RootObjNum is larger than 0, > m_pTrailer should not be nullptr. Please check the details in > CPDF_Parser::GetRootObjNum(). Also, we have at least one > object, the root object, in m_CrossRef. > > > FX_DWORD CPDF_Parser::GetRootObjNum() { > CPDF_Reference* pRef = ToReference( > m_pTrailer ? m_pTrailer->GetElement(FX_BSTRC("Root")) : nullptr); > return pRef ? pRef->GetRefObjNum() : 0; > } I saw this. But RebuildCrossRef function is used at eight different places. Checking the results should not rely on some implicit conditions of other functions although it was ok to not checking on these two specific places. BTW, file trailer's root element has an obj number doesn't mean m_CrossRef has that object (for some malformed pdf). So pls notice that we are still checking a bit more than before. https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:252: if (!RebuildCrossRef()) On 2015/12/01 15:25:18, jun_fang wrote: > please check the comment at line 1610. Acknowledged. https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:975: return m_pTrailer && GetRootObjNum() > 0 && m_CrossRef.GetSize() > 0; On 2015/12/01 15:20:58, jun_fang wrote: > The newly added condition ' GetRootObjNum() > 0' will change the existing > behavior. Please restore back to "m_pTrailer && m_CrossRef.GetSize() > 0". > Thanks! I reverted to my original change. I think more thorough format checking may help discover problems early, but at this moment I would opt for not changing current caller behavior. :) https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1610: if (!RebuildCrossRef()) On 2015/12/01 15:20:58, jun_fang wrote: > Based on the analysis, I prefer to keep left code unchanged. It's better to add > a comment to explain why it's no need to check the return of RebuildCrossRef(). I would prefer keeping the check here as the comments above stated. thanks.
On 2015/12/01 18:45:04, Wei wrote: > https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (left): > > https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1612: if (RootObjNum == 0) { > On 2015/12/01 14:56:20, jun_fang wrote: > > It's a trigger to rebuild cross reference table when RootObjNum is 0. > > After the cross reference is rebuilt, only checking RootObjNum is enough to > > verify whether RebuildCrossRef succeeded. If RootObjNum is larger than 0, > > m_pTrailer should not be nullptr. Please check the details in > > CPDF_Parser::GetRootObjNum(). Also, we have at least one > > object, the root object, in m_CrossRef. > > > > > > FX_DWORD CPDF_Parser::GetRootObjNum() { > > CPDF_Reference* pRef = ToReference( > > m_pTrailer ? m_pTrailer->GetElement(FX_BSTRC("Root")) : nullptr); > > return pRef ? pRef->GetRefObjNum() : 0; > > } > > I saw this. But RebuildCrossRef function is used at eight different places. > Checking the results should not rely on some implicit conditions of other > functions although it was ok to not checking on these two specific places. BTW, > file trailer's root element has an obj number doesn't mean m_CrossRef has that > object (for some malformed pdf). So pls notice that we are still checking a bit > more than before. > > https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right): > > https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:252: if (!RebuildCrossRef()) > On 2015/12/01 15:25:18, jun_fang wrote: > > please check the comment at line 1610. > > Acknowledged. > > https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:975: return m_pTrailer && > GetRootObjNum() > 0 && m_CrossRef.GetSize() > 0; > On 2015/12/01 15:20:58, jun_fang wrote: > > The newly added condition ' GetRootObjNum() > 0' will change the existing > > behavior. Please restore back to "m_pTrailer && m_CrossRef.GetSize() > 0". > > Thanks! > > I reverted to my original change. I think more thorough format checking may help > discover problems early, but at this moment I would opt for not changing current > caller behavior. :) > > https://codereview.chromium.org/1476163002/diff/80001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1610: if > (!RebuildCrossRef()) > On 2015/12/01 15:20:58, jun_fang wrote: > > Based on the analysis, I prefer to keep left code unchanged. It's better to > add > > a comment to explain why it's no need to check the return of > RebuildCrossRef(). > > I would prefer keeping the check here as the comments above stated. thanks. OK. LGTM for patch 5. Thanks!
Description was changed from ========== Add basic checking for RebuildCrossRef RebuildCrossRef function returns false when we can not find file trailer or any indirect object. This serves as a basic file format checking. BUG=pdfium:215 ========== to ========== Add basic checking for RebuildCrossRef RebuildCrossRef function returns false when we can not find file trailer or any indirect object. This serves as a basic file format checking. BUG=pdfium:215 R=jun_fang@foxitsoftware.com Committed: https://pdfium.googlesource.com/pdfium/+/f14da1d58e8e12633c7a47e6efd5ffe43bb3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) manually as f14da1d58e8e12633c7a47e6efd5ffe43bb37b4b (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
