Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2375343004:
Assert that only 0-numbered objects are Released() (Closed)
DescriptionAssert that only 0-numbered objects are Released()
This condition holds because numbered objects are brute-force
deleted by the indirect object holder, rather than being
released.
Be careful about recursive deletion, check before advancing,
since we no longer count on Release() doing this for us.
Fix a few tests where the test was violating ownership rules.
This should be the last step before completely removing Release()
in favor of direct delete everywhere.
Committed: https://pdfium.googlesource.com/pdfium/+/aba528a362248a54b27a7e9e046e2b65ab83f624
Patch Set 1 #Patch Set 2 : brute-force delete unowned numbered object in one place #
Total comments: 13
Patch Set 3 : make block whole again. #Patch Set 4 : Test stream/dictionary loop #Patch Set 5 : Add test for stream loop #Patch Set 6 : Add unowned ptr to stream #
Total comments: 2
Patch Set 7 : typo #
Messages
Total messages: 49 (27 generated)
The CQ bit was checked by tsepez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Assert that only 0-numbered objects are Released() This condition holds because numbered objects are brute-force deleted by the indirect object holder, rather than being released. Be careful about recursive deletion, check before advancing. Fix a few tests where the test was violating ownership rules. This should be the last step before just removing Release() in favor of direct delete. ========== to ========== Assert that only 0-numbered objects are Released() This condition holds because numbered objects are brute-force deleted by the indirect object holder, rather than being released. Be careful about recursive deletion, check before advancing. Fix a few tests where the test was violating ownership rules. This should be the last step before just removing Release() in favor of direct delete. ==========
tsepez@chromium.org changed reviewers: + thestig@chromium.org
Lei, for review.
Description was changed from ========== Assert that only 0-numbered objects are Released() This condition holds because numbered objects are brute-force deleted by the indirect object holder, rather than being released. Be careful about recursive deletion, check before advancing. Fix a few tests where the test was violating ownership rules. This should be the last step before just removing Release() in favor of direct delete. ========== to ========== Assert that only 0-numbered objects are Released() This condition holds because numbered objects are brute-force deleted by the indirect object holder, rather than being released. Be careful about recursive deletion, check before advancing. Fix a few tests where the test was violating ownership rules. This should be the last step before completely removing Release() in favor of direct delete everywhere. ==========
Description was changed from ========== Assert that only 0-numbered objects are Released() This condition holds because numbered objects are brute-force deleted by the indirect object holder, rather than being released. Be careful about recursive deletion, check before advancing. Fix a few tests where the test was violating ownership rules. This should be the last step before completely removing Release() in favor of direct delete everywhere. ========== to ========== Assert that only 0-numbered objects are Released() This condition holds because numbered objects are brute-force deleted by the indirect object holder, rather than being released. Be careful about recursive deletion, check before advancing, since we no longer count on Release() doing this for us. Fix a few tests where the test was violating ownership rules. This should be the last step before completely removing Release() in favor of direct delete everywhere. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/linux/builds/2280)
The CQ bit was checked by tsepez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Are you thinking |kInvalidObjNum| == 0? |kInvalidObjNum| is actually 0xFFFFFFFF.
On 2016/09/30 01:08:48, Lei Zhang wrote: > Are you thinking |kInvalidObjNum| == 0? |kInvalidObjNum| is actually 0xFFFFFFFF. I'm aware of that, hence the logic in the array/dict object dtors to not try release when owned object invalid.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win on master.tryserver.client.pdfium (JOB_TIMED_OUT, no build URL) win_no_v8 on master.tryserver.client.pdfium (JOB_TIMED_OUT, no build URL) win_xfa on master.tryserver.client.pdfium (JOB_TIMED_OUT, no build URL) win_xfa_clang on master.tryserver.client.pdfium (JOB_TIMED_OUT, no build URL)
Can I help explain anything?
https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_object.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_object.cpp:41: CHECK(!m_ObjNum); So should we also CHECK(m_ObjNum != kInvalidObjNum); ?
https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_object.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_object.cpp:41: CHECK(!m_ObjNum); On 2016/09/30 21:44:52, Lei Zhang wrote: > So should we also CHECK(m_ObjNum != kInvalidObjNum); ? That's implied by the current check.
https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_object.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_object.cpp:41: CHECK(!m_ObjNum); On 2016/09/30 22:21:37, Tom Sepez wrote: > On 2016/09/30 21:44:52, Lei Zhang wrote: > > So should we also CHECK(m_ObjNum != kInvalidObjNum); ? > > That's implied by the current check. It is? CHECK(m_ObjNum != 0); CHECK(m_ObjNum != 0xFFFFFFFF);
On 2016/09/30 22:36:05, Lei Zhang wrote: > https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... > File core/fpdfapi/fpdf_parser/cpdf_object.cpp (right): > > https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... > core/fpdfapi/fpdf_parser/cpdf_object.cpp:41: CHECK(!m_ObjNum); > On 2016/09/30 22:21:37, Tom Sepez wrote: > > On 2016/09/30 21:44:52, Lei Zhang wrote: > > > So should we also CHECK(m_ObjNum != kInvalidObjNum); ? > > > > That's implied by the current check. > > It is? > > CHECK(m_ObjNum != 0); > CHECK(m_ObjNum != 0xFFFFFFFF); Am I losing my mind? CHECK(!m_ObjNum) => CHECK(m_ObjNum == 0) ==> CHECK(0xffffffff == 0) ==> CHECK(false) ==> abort()
On 2016/09/30 22:51:33, Tom Sepez wrote: > Am I losing my mind? Nah. It's obviously me.
https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_parser.cpp:970: CPDF_Stream* pStream = pObject->AsStream(); Do you know if |pObject| is expected to be a stream? Can we move this up to line 963 and bail out early if it's not a stream? Then the if (m_pDocument) block that has now been split in two can become one again. https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_parser.cpp:977: if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration( Or should this stay up with the other if (m_pDocument) block? By moving this down, do we miss out on objects with higher generation numbers? https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_stream.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_stream.cpp:25: m_pDict.release(); // lowercase release, release ownership. Does this leak or is this to avoid a (future) double free? If CPDF_Stream owns the dictionary object, isn't it bad if someone already freed it?
https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_parser.cpp:970: CPDF_Stream* pStream = pObject->AsStream(); On 2016/09/30 23:49:01, Lei Zhang wrote: > Do you know if |pObject| is expected to be a stream? Can we move this up to line > 963 and bail out early if it's not a stream? Then the if (m_pDocument) block > that has now been split in two can become one again. Done. https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_parser.cpp:977: if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration( On 2016/09/30 23:49:01, Lei Zhang wrote: > Or should this stay up with the other if (m_pDocument) block? By moving this > down, do we miss out on objects with higher generation numbers? Done. https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_stream.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_stream.cpp:25: m_pDict.release(); // lowercase release, release ownership. On 2016/09/30 23:49:01, Lei Zhang wrote: > Does this leak or is this to avoid a (future) double free? If CPDF_Stream owns > the dictionary object, isn't it bad if someone already freed it Yes, this is only for the possibility of cycles, where someone has freed the dict through an unowned pointer. We do this in the tests.
The CQ bit was checked by tsepez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_stream.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_stream.cpp:25: m_pDict.release(); // lowercase release, release ownership. On 2016/10/01 00:13:11, Tom Sepez wrote: > On 2016/09/30 23:49:01, Lei Zhang wrote: > > Does this leak or is this to avoid a (future) double free? If CPDF_Stream owns > > the dictionary object, isn't it bad if someone already freed it > > Yes, this is only for the possibility of cycles, where someone has > freed the dict through an unowned pointer. We do this in the tests. Which test? Not seeing it / missed it.
On 2016/10/01 00:34:15, Lei Zhang wrote: > https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... > File core/fpdfapi/fpdf_parser/cpdf_stream.cpp (right): > > https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... > core/fpdfapi/fpdf_parser/cpdf_stream.cpp:25: m_pDict.release(); // lowercase > release, release ownership. > On 2016/10/01 00:13:11, Tom Sepez wrote: > > On 2016/09/30 23:49:01, Lei Zhang wrote: > > > Does this leak or is this to avoid a (future) double free? If CPDF_Stream > owns > > > the dictionary object, isn't it bad if someone already freed it > > > > Yes, this is only for the possibility of cycles, where someone has > > freed the dict through an unowned pointer. We do this in the tests. > > Which test? Not seeing it / missed it. https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_par...
https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_parser.cpp:977: if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration( On 2016/10/01 00:13:11, Tom Sepez wrote: > On 2016/09/30 23:49:01, Lei Zhang wrote: > > Or should this stay up with the other if (m_pDocument) block? By moving this > > down, do we miss out on objects with higher generation numbers? > > Done. So I still wonder if changing the order of AsStream() and ReplaceIndirectObjectIfHigherGeneration() will effect parsing of PDFs with objects with generation numbers. I'm worried we don't have enough test coverage there. https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_stream.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_stream.cpp:25: m_pDict.release(); // lowercase release, release ownership. On 2016/10/01 00:34:15, Lei Zhang wrote: > Which test? Not seeing it / missed it. I don't see PDFObjectTest.CloneCheckLoop creating any CPDF_Streams.
> https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... > core/fpdfapi/fpdf_parser/cpdf_stream.cpp:25: m_pDict.release(); // lowercase > release, release ownership. > On 2016/10/01 00:34:15, Lei Zhang wrote: > > Which test? Not seeing it / missed it. > > I don't see PDFObjectTest.CloneCheckLoop creating any CPDF_Streams. Argh, you're right. I guess we don't test this. It follows by extension from dictionary and array that a stream, containing a dictionary should do the same as the other two.
The CQ bit was checked by tsepez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Ok, added a new test, which blows up with segv/double free etc. on current master branch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL again. https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... File core/fpdfapi/fpdf_parser/cpdf_parser.cpp (right): https://codereview.chromium.org/2375343004/diff/20001/core/fpdfapi/fpdf_parse... core/fpdfapi/fpdf_parser/cpdf_parser.cpp:977: if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration( On 2016/10/01 01:24:06, Lei Zhang wrote: > On 2016/10/01 00:13:11, Tom Sepez wrote: > > On 2016/09/30 23:49:01, Lei Zhang wrote: > > > Or should this stay up with the other if (m_pDocument) block? By moving this > > > down, do we miss out on objects with higher generation numbers? > > > > Done. > > So I still wonder if changing the order of AsStream() and > ReplaceIndirectObjectIfHigherGeneration() will effect parsing of PDFs with > objects with generation numbers. I'm worried we don't have enough test coverage > there. Done.
The CQ bit was checked by tsepez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2375343004/diff/100001/core/fpdfapi/fpdf_pars... File core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp (right): https://codereview.chromium.org/2375343004/diff/100001/core/fpdfapi/fpdf_pars... core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp:793: // Create an dictionary/array pair with a reference loop. a dictionary
https://codereview.chromium.org/2375343004/diff/100001/core/fpdfapi/fpdf_pars... File core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp (right): https://codereview.chromium.org/2375343004/diff/100001/core/fpdfapi/fpdf_pars... core/fpdfapi/fpdf_parser/cpdf_object_unittest.cpp:793: // Create an dictionary/array pair with a reference loop. On 2016/10/03 21:14:46, Lei Zhang wrote: > a dictionary Done.
The CQ bit was checked by tsepez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2375343004/#ps120001 (title: "typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Assert that only 0-numbered objects are Released() This condition holds because numbered objects are brute-force deleted by the indirect object holder, rather than being released. Be careful about recursive deletion, check before advancing, since we no longer count on Release() doing this for us. Fix a few tests where the test was violating ownership rules. This should be the last step before completely removing Release() in favor of direct delete everywhere. ========== to ========== Assert that only 0-numbered objects are Released() This condition holds because numbered objects are brute-force deleted by the indirect object holder, rather than being released. Be careful about recursive deletion, check before advancing, since we no longer count on Release() doing this for us. Fix a few tests where the test was violating ownership rules. This should be the last step before completely removing Release() in favor of direct delete everywhere. Committed: https://pdfium.googlesource.com/pdfium/+/aba528a362248a54b27a7e9e046e2b65ab83... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://pdfium.googlesource.com/pdfium/+/aba528a362248a54b27a7e9e046e2b65ab83...
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2387193003/ by thestig@chromium.org. The reason for reverting is: Broke PDFExtensionTest when rolling DEPS in Chromium.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
