Make CPDF_PageContentGenerator methods take object numbers

Make CPDF_PageContentGenerator methods take object numbers

This patch fixes a possibility that an owned CPDF_Stream is handed to the
indirect object holder inside RealizeResource().  Its arguments are
changed to take an object number, as is done elsewhere in the code, to
suggest that only indirect objects are acceptable.

BUG=660756
Committed: https://pdfium.googlesource.com/pdfium/+/137a344ad02056107e2e01d5d55f5e97d21fa74b

[Tom Sepez, 2016-11-10 21:10:52]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 21:10:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-11-10 21:12:22]

Description was changed from

==========
Speculative fix for crash in CPDF_PageContentGenerator::ProcessImage

Speculative because I don't have a good repro, but the symptoms
match the analysis.  This patch fixes a possility that an owned
CPDF_Stream is handed to the indirect object holder inside
RealizeResource().  Its arguments are changed to take an object
number, as is done elsewhere in the code, to suggest that only
indirect objects are acceptable.

BUG=600756
==========

to

==========
Speculative fix for crash in CPDF_PageContentGenerator::ProcessImage

Speculative because I don't have a good repro, but the symptoms
match the analysis.  This patch fixes a possility that an owned
CPDF_Stream is handed to the indirect object holder inside
RealizeResource().  Its arguments are changed to take an object
number, as is done elsewhere in the code, to suggest that only
indirect objects are acceptable.

BUG=660756
==========

[Tom Sepez, 2016-11-10 21:12:54]

tsepez@chromium.org changed reviewers:
+ thestig@chromium.org

[Tom Sepez, 2016-11-10 21:15:26]

Lei, ready for review.

https://codereview.chromium.org/2489423002/diff/1/core/fpdfapi/edit/cpdf_page...
File core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp (right):

https://codereview.chromium.org/2489423002/diff/1/core/fpdfapi/edit/cpdf_page...
core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp:105: pStream =
ToStream(pStream->Clone().release());
Note: this is the clone that avoids the UAF.  The setunownedimage below clobbers
the original stream and if this is called again on the same image, we get issue.

[Lei Zhang, 2016-11-10 21:37:44]

I can try to repro on Windows later today. The ASAN bot is red, BTW.

[commit-bot: I haz the power, 2016-11-10 22:04:52]

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

[commit-bot: I haz the power, 2016-11-10 22:04:53]

Dry run: Try jobs failed on following builders:
  linux_asan_lsan on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_asan_lsan...)
  linux_xfa_asan_lsan on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa_asan_...)

[Lei Zhang, 2016-11-10 22:25:20]

On 2016/11/10 21:37:44, Lei Zhang (slow) wrote:
> I can try to repro on Windows later today. The ASAN bot is red, BTW.

Repros in a regular Chromium build. Just load the PDF and try to print it.

[Tom Sepez, 2016-11-10 22:40:06]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 22:40:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-11-10 22:40:22]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 22:40:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Tom Sepez, 2016-11-10 23:00:30]

On 2016/11/10 22:25:20, Lei Zhang (slow) wrote:
> On 2016/11/10 21:37:44, Lei Zhang (slow) wrote:
> > I can try to repro on Windows later today. The ASAN bot is red, BTW.
> 
> Repros in a regular Chromium build. Just load the PDF and try to print it.

I'm getting a different crash when I try that then what CF hit.  The crash I get
is blink/skia

[Lei Zhang, 2016-11-10 23:09:57]

https://codereview.chromium.org/2489423002/diff/20001/core/fpdfapi/edit/cpdf_...
File core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp (right):

https://codereview.chromium.org/2489423002/diff/20001/core/fpdfapi/edit/cpdf_...
core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp:98: CPDF_Image* pImage =
pImageObj->GetImage();
I tried patch set 2 and I think it's crashing here with a NULL |pImage|.

[Lei Zhang, 2016-11-10 23:18:13]

On 2016/11/10 23:09:57, Lei Zhang (slow) wrote:
>
https://codereview.chromium.org/2489423002/diff/20001/core/fpdfapi/edit/cpdf_...
> File core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp (right):
> 
>
https://codereview.chromium.org/2489423002/diff/20001/core/fpdfapi/edit/cpdf_...
> core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp:98: CPDF_Image* pImage =
> pImageObj->GetImage();
> I tried patch set 2 and I think it's crashing here with a NULL |pImage|.

Nope, that's not it. It's crashing in CPDF_Object::IsInline(), but the stack
trace isn't that helpful on its own. Looking further.

[Lei Zhang, 2016-11-10 23:20:31]

https://codereview.chromium.org/2489423002/diff/20001/core/fpdfapi/edit/cpdf_...
File core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp (right):

https://codereview.chromium.org/2489423002/diff/20001/core/fpdfapi/edit/cpdf_...
core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp:102: CPDF_Stream* pStream =
pImage->GetStream();
Err, it's a NULL |pStream|.

[Tom Sepez, 2016-11-14 22:12:35]

Description was changed from

==========
Speculative fix for crash in CPDF_PageContentGenerator::ProcessImage

Speculative because I don't have a good repro, but the symptoms
match the analysis.  This patch fixes a possility that an owned
CPDF_Stream is handed to the indirect object holder inside
RealizeResource().  Its arguments are changed to take an object
number, as is done elsewhere in the code, to suggest that only
indirect objects are acceptable.

BUG=660756
==========

to

==========
Make CPDF_PageContentGenerator methods take object numbers

This patch fixes a possibility that an owned CPDF_Stream is handed to the
indirect object holder inside RealizeResource().  Its arguments are changed to
take an object number, as is done elsewhere in the code, to suggest that only
indirect objects are acceptable.

BUG=660756
==========

[Tom Sepez, 2016-11-14 22:13:04]

Description was changed from

==========
Make CPDF_PageContentGenerator methods take object numbers

This patch fixes a possibility that an owned CPDF_Stream is handed to the
indirect object holder inside RealizeResource().  Its arguments are changed to
take an object number, as is done elsewhere in the code, to suggest that only
indirect objects are acceptable.

BUG=660756
==========

to

==========
Make CPDF_PageContentGenerator methods take object numbers

This patch fixes a possibility that an owned CPDF_Stream is handed to the
indirect object holder inside RealizeResource().  Its arguments are
changed to take an object number, as is done elsewhere in the code, to
suggest that only indirect objects are acceptable.

BUG=660756
==========

[Tom Sepez, 2016-11-14 22:15:48]

tsepez@chromium.org changed reviewers:
+ dsinclair@chromium.org

[Tom Sepez, 2016-11-14 22:16:26]

+Dan who is not "not reviewing code".

[Tom Sepez, 2016-11-14 22:16:36]

The CQ bit was checked by tsepez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 22:16:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-11-14 22:23:42]

lgtm

[Tom Sepez, 2016-11-14 22:58:50]

The CQ bit was unchecked by tsepez@chromium.org

[Tom Sepez, 2016-11-14 22:58:53]

The CQ bit was checked by tsepez@chromium.org

[commit-bot: I haz the power, 2016-11-14 22:59:00]

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-11-14 23:03:58]

Description was changed from

==========
Make CPDF_PageContentGenerator methods take object numbers

This patch fixes a possibility that an owned CPDF_Stream is handed to the
indirect object holder inside RealizeResource().  Its arguments are
changed to take an object number, as is done elsewhere in the code, to
suggest that only indirect objects are acceptable.

BUG=660756
==========

to

==========
Make CPDF_PageContentGenerator methods take object numbers

This patch fixes a possibility that an owned CPDF_Stream is handed to the
indirect object holder inside RealizeResource().  Its arguments are
changed to take an object number, as is done elsewhere in the code, to
suggest that only indirect objects are acceptable.

BUG=660756

Committed:
https://pdfium.googlesource.com/pdfium/+/137a344ad02056107e2e01d5d55f5e97d21f...
==========

[commit-bot: I haz the power, 2016-11-14 23:03:59]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/137a344ad02056107e2e01d5d55f5e97d21f...

[Tom Sepez, 2016-11-21 18:00:14]

LGTM, thanks.

[Tom Sepez, 2016-11-21 18:03:56]

Ooops, wrong CL for last message, meant to say, see also
https://codereview.chromium.org/2513273003/ for why inline
isn't the same as owning stream.