Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1585533002:
Fix infinite loops caused by calling circular indirect objects (Closed)
|
Created:
4 years, 11 months ago by Wei Li Modified:
4 years, 11 months ago CC:
pdfium-reviews_googlegroups.com, kai_jing Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix infinite loops caused by calling circular indirect objects
There are multiple functions in CPDF_Object class which can cause infinite loop due to recursively calling circular indirect objects. Fix them by deference indirect object first.
BUG=pdfium:355
R=jun_fang@foxitsoftware.com, thestig@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/90853cb1dfd1bf3803ec21cfae3e93948137be61
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Total comments: 8
Patch Set 3 : fix nits and rebase #
Total comments: 2
Patch Set 4 : address comments and rebase #
Total comments: 1
Messages
Total messages: 20 (4 generated)
Description was changed from ========== Fix infinite loops caused by calling circular indirect objects There are multiple functions in CPDF_Object class which can cause infinite loop due to recursively calling circular indirect objects. Fix them by dereference indirect object first. BUG=pdfium:355 R=thestig@chromium.org ========== to ========== Fix infinite loops caused by calling circular indirect objects There are multiple functions in CPDF_Object class which can cause infinite loop due to recursively calling circular indirect objects. Fix them by deference indirect object first. BUG=pdfium:355 R=thestig@chromium.org ==========
PTAL. thanks
Are references to references legal? If no, then this is fine. Otherwise, a reference to a reference to a string will no longer work with this CL, right? https://codereview.chromium.org/1585533002/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1585533002/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:56: } else no else, or you can write the function as: if (m_Type != PDFOBJ_REFERENCE) return this; const CPDF_Reference* pRef = AsReference(); ... https://codereview.chromium.org/1585533002/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:83: return CFX_ByteStringC((const uint8_t*)str, str.GetLength()); Can we switch to the "CFX_ByteStringC(const CFX_ByteString& src);" ctor?
https://codereview.chromium.org/1585533002/diff/1/core/src/fpdfapi/fpdf_parse... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1585533002/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:56: } else On 2016/01/13 02:33:16, Lei Zhang wrote: > no else, or you can write the function as: > > if (m_Type != PDFOBJ_REFERENCE) > return this; > > const CPDF_Reference* pRef = AsReference(); > ... Done. https://codereview.chromium.org/1585533002/diff/1/core/src/fpdfapi/fpdf_parse... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:83: return CFX_ByteStringC((const uint8_t*)str, str.GetLength()); On 2016/01/13 02:33:16, Lei Zhang wrote: > Can we switch to the "CFX_ByteStringC(const CFX_ByteString& src);" ctor? Done.
weili@chromium.org changed reviewers: + jun_fang@foxitsoftware.com
+Jun as reviewer Could an indirect object refer to another indirect object? I read the pdf spec. It seems to suggest indirect objects are indirect references to other objects. I did some tests, and looks like Acrobat thinks the same. There are other places in pdfium that assume only one level indirection. Is it safe to assume that indirect object only refers to a basic object? If not, I can change the code to detect circular references.
On 2016/01/13 06:28:54, Wei Li wrote: > +Jun as reviewer > > Could an indirect object refer to another indirect object? I read the pdf spec. > It seems to suggest indirect objects are indirect references to other objects. I > did some tests, and looks like Acrobat thinks the same. There are other places > in pdfium that assume only one level indirection. Is it safe to assume that > indirect object only refers to a basic object? If not, I can change the code to > detect circular references. Hi Wei, please give me one more day. I will review this CL tomorrow.
On 2016/01/13 16:15:18, jun_fang wrote: > On 2016/01/13 06:28:54, Wei Li wrote: > > +Jun as reviewer > > > > Could an indirect object refer to another indirect object? I read the pdf > spec. > > It seems to suggest indirect objects are indirect references to other objects. > I > > did some tests, and looks like Acrobat thinks the same. There are other places > > in pdfium that assume only one level indirection. Is it safe to assume that > > indirect object only refers to a basic object? If not, I can change the code > to > > detect circular references. > > Hi Wei, please give me one more day. I will review this CL tomorrow. A indirect reference can only be a reference to an indirect object rather than a reference. LGTM with nits.
https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:50: if (m_Type == PDFOBJ_REFERENCE) { nit: prefer like if (m_Type != PDFOBJ_REFERENCE) return this; https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:80: switch (obj->GetType()) { nit: if we only have two conditions, I'd like to use 'if' rather than 'case'. https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:108: switch (obj->GetType()) { nit: use 'if'? https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:121: switch (obj->GetType()) { nit: the same as above
Jun, thanks for the review. I assume you meant "An indirect reference can only be a reference to a DIRECT object rather than a reference". Lei, pls take a look, thanks! https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:50: if (m_Type == PDFOBJ_REFERENCE) { On 2016/01/14 14:58:40, jun_fang wrote: > nit: prefer like > > if (m_Type != PDFOBJ_REFERENCE) > return this; Done. https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:80: switch (obj->GetType()) { On 2016/01/14 14:58:40, jun_fang wrote: > nit: if we only have two conditions, I'd like to use 'if' rather than 'case'. Done. https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:108: switch (obj->GetType()) { On 2016/01/14 14:58:40, jun_fang wrote: > nit: use 'if'? Done. https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:121: switch (obj->GetType()) { On 2016/01/14 14:58:40, jun_fang wrote: > nit: the same as above Done.
On 2016/01/14 19:07:22, Wei Li wrote: > Jun, thanks for the review. I assume you meant "An indirect reference can only > be a reference to a DIRECT object rather than a reference". > > Lei, pls take a look, thanks! > > https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... > File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): > > https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:50: if (m_Type == > PDFOBJ_REFERENCE) { > On 2016/01/14 14:58:40, jun_fang wrote: > > nit: prefer like > > > > if (m_Type != PDFOBJ_REFERENCE) > > return this; > > Done. > > https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:80: switch (obj->GetType()) > { > On 2016/01/14 14:58:40, jun_fang wrote: > > nit: if we only have two conditions, I'd like to use 'if' rather than 'case'. > > Done. > > https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:108: switch > (obj->GetType()) { > On 2016/01/14 14:58:40, jun_fang wrote: > > nit: use 'if'? > > Done. > > https://codereview.chromium.org/1585533002/diff/20001/core/src/fpdfapi/fpdf_p... > core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:121: switch > (obj->GetType()) { > On 2016/01/14 14:58:40, jun_fang wrote: > > nit: the same as above > > Done. Below is the definition of 'indirect object': an object that is labeled with a positive integer object number followed by a non-negative integer generation number followed by obj and having endobj after it. So an indirect reference should be a reference to an indirect object not a direct object.
> Below is the definition of 'indirect object':
>
> an object that is labeled with a positive integer object number followed by a
> non-negative integer generation number followed by obj and having endobj after
> it.
>
> So an indirect reference should be a reference to an indirect object not a
> direct object.
Yes, you are right. I mixed up the terms.
My original question is whether an indirect object can refers
to another indirect object. For example:
2 0 obj
3 0 obj
(hello)
endobj
endobj
I think we can safely assume this won't happen, right?
On 2016/01/15 01:17:08, Wei Li wrote: > > Below is the definition of 'indirect object': > > > > an object that is labeled with a positive integer object number followed by a > > non-negative integer generation number followed by obj and having endobj after > > it. > > > > So an indirect reference should be a reference to an indirect object not a > > direct object. > > Yes, you are right. I mixed up the terms. > My original question is whether an indirect object can refers > to another indirect object. For example: > 2 0 obj > 3 0 obj > (hello) > endobj > endobj > > I think we can safely assume this won't happen, right? According to the following description, this case is illegal. Beginning with PDF 1.5, indirect objects may reside in object streams (see 7.5.7, "Object Streams"). They are referred to in the same way; however, their definition shall not include the keywords obj and endobj, and their generation number shall be zero.
lgtm https://codereview.chromium.org/1585533002/diff/40001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1585533002/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:50: if (m_Type != PDFOBJ_REFERENCE) You can just call AsReference() and omit this check. If it's not a reference, it'll return nullptr. Check for that instead.
Thanks, will submit https://codereview.chromium.org/1585533002/diff/40001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp (right): https://codereview.chromium.org/1585533002/diff/40001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_parser/fpdf_parser_objects.cpp:50: if (m_Type != PDFOBJ_REFERENCE) On 2016/01/16 00:51:57, Lei Zhang (OOO) wrote: > You can just call AsReference() and omit this check. If it's not a reference, > it'll return nullptr. Check for that instead. Done.
Description was changed from ========== Fix infinite loops caused by calling circular indirect objects There are multiple functions in CPDF_Object class which can cause infinite loop due to recursively calling circular indirect objects. Fix them by deference indirect object first. BUG=pdfium:355 R=thestig@chromium.org ========== to ========== Fix infinite loops caused by calling circular indirect objects There are multiple functions in CPDF_Object class which can cause infinite loop due to recursively calling circular indirect objects. Fix them by deference indirect object first. BUG=pdfium:355 R=jun_fang@foxitsoftware.com, thestig@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/90853cb1dfd1bf3803ec21cfae3e93948137... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 90853cb1dfd1bf3803ec21cfae3e93948137be61 (presubmit successful).
Message was sent while issue was closed.
ochang@chromium.org changed reviewers: + ochang@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1585533002/diff/60001/core/include/fpdfapi/fp... File core/include/fpdfapi/fpdf_objects.h (right): https://codereview.chromium.org/1585533002/diff/60001/core/include/fpdfapi/fp... core/include/fpdfapi/fpdf_objects.h:105: const CPDF_Object* const GetBasicObject() const; this extra const prevented a roll :( ../../third_party/pdfium/core/include/fpdfapi/fpdf_objects.h:105:22: error: 'const' type qualifier on return type has no effect [-Werror,-Wignored-qualifiers] const CPDF_Object* const GetBasicObject() const; Not sure why we didn't catch this on our bots.
Message was sent while issue was closed.
On 2016/01/19 23:09:35, Oliver Chang wrote: > https://codereview.chromium.org/1585533002/diff/60001/core/include/fpdfapi/fp... > File core/include/fpdfapi/fpdf_objects.h (right): > > https://codereview.chromium.org/1585533002/diff/60001/core/include/fpdfapi/fp... > core/include/fpdfapi/fpdf_objects.h:105: const CPDF_Object* const > GetBasicObject() const; > this extra const prevented a roll :( > > ../../third_party/pdfium/core/include/fpdfapi/fpdf_objects.h:105:22: error: > 'const' type qualifier on return type has no effect > [-Werror,-Wignored-qualifiers] > const CPDF_Object* const GetBasicObject() const; > > Not sure why we didn't catch this on our bots. Because we don't have Werror turned on. Some day... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
