Make CPDF_Array not do indirect object creation.

Make CPDF_Array not do indirect object creation.

We remove the indirect object holder argument and check that
call sites pass ownable objects, adding a reference in one
place that always was passing an indirect object.

Also check that the invariant isn't violated, we need to fail
here in the wild and investigate -- these are existing UAFs.

Committed: https://pdfium.googlesource.com/pdfium/+/5a399de2945d7b244802565d8e9d2f6e662561da

[Tom Sepez, 2016-09-20 19:21:48]

Description was changed from

==========
Make CPDF_Array not do indirect object creation.

We remove the indirect object holder argument and check that
call sites pass ownable objects, adding a reference in one
place that always was indirect.
==========

to

==========
Make CPDF_Array not do indirect object creation.

We remove the indirect object holder argument and check that
call sites pass ownable objects, adding a reference in one
place that always was indirect.

Also check that the invariant isn't violated, we need to fail here in the wild
and investigate else UAF.
==========

[Tom Sepez, 2016-09-20 19:21:48]

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

[Tom Sepez, 2016-09-20 19:22:32]

Description was changed from

==========
Make CPDF_Array not do indirect object creation.

We remove the indirect object holder argument and check that
call sites pass ownable objects, adding a reference in one
place that always was indirect.

Also check that the invariant isn't violated, we need to fail here in the wild
and investigate else UAF.
==========

to

==========
Make CPDF_Array not do indirect object creation.

We remove the indirect object holder argument and check that
call sites pass ownable objects, adding a reference in one
place that always was passing an indirect object.

Also check that the invariant isn't violated, we need to fail
here in the wild and investigate -- these are existing UAFs.
==========

[Tom Sepez, 2016-09-20 19:22:44]

Dan, next one.

[dsinclair, 2016-09-20 19:32:19]

https://codereview.chromium.org/2355083002/diff/20001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_array.cpp (left):

https://codereview.chromium.org/2355083002/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_array.cpp:213: ASSERT(IsArray());
Why remove the asserts?

https://codereview.chromium.org/2355083002/diff/20001/fpdfsdk/fpdfsave.cpp
File fpdfsdk/fpdfsave.cpp (right):

https://codereview.chromium.org/2355083002/diff/20001/fpdfsdk/fpdfsave.cpp#ne...
fpdfsdk/fpdfsave.cpp:191: pArray->InsertAt(iLast + 1, pData);
Does this need to be wrapped in a Reference? We did an AddIndirectObject(pData)
on 188.

https://codereview.chromium.org/2355083002/diff/20001/fpdfsdk/fpdfsave.cpp#ne...
fpdfsdk/fpdfsave.cpp:212: pArray->InsertAt(iLast + 1, pData);
ditto

[Tom Sepez, 2016-09-20 20:07:22]

https://codereview.chromium.org/2355083002/diff/20001/core/fpdfapi/fpdf_parse...
File core/fpdfapi/fpdf_parser/cpdf_array.cpp (left):

https://codereview.chromium.org/2355083002/diff/20001/core/fpdfapi/fpdf_parse...
core/fpdfapi/fpdf_parser/cpdf_array.cpp:213: ASSERT(IsArray());
On 2016/09/20 19:32:18, dsinclair wrote:
> Why remove the asserts?

I moved it to Add().  No sense in having it in N places.

https://codereview.chromium.org/2355083002/diff/20001/fpdfsdk/fpdfsave.cpp
File fpdfsdk/fpdfsave.cpp (right):

https://codereview.chromium.org/2355083002/diff/20001/fpdfsdk/fpdfsave.cpp#ne...
fpdfsdk/fpdfsave.cpp:191: pArray->InsertAt(iLast + 1, pData);
On 2016/09/20 19:32:18, dsinclair wrote:
> Does this need to be wrapped in a Reference? We did an
AddIndirectObject(pData)
> on 188.

Good catch.  I missed this one. and the one below it.

https://codereview.chromium.org/2355083002/diff/20001/fpdfsdk/fpdfsave.cpp#ne...
fpdfsdk/fpdfsave.cpp:212: pArray->InsertAt(iLast + 1, pData);
On 2016/09/20 19:32:18, dsinclair wrote:
> ditto

Done.

[dsinclair, 2016-09-20 20:08:33]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-09-20 20:08:33]

lgtm

[commit-bot: I haz the power, 2016-09-20 20:08:43]

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-09-20 20:23:23]

Description was changed from

==========
Make CPDF_Array not do indirect object creation.

We remove the indirect object holder argument and check that
call sites pass ownable objects, adding a reference in one
place that always was passing an indirect object.

Also check that the invariant isn't violated, we need to fail
here in the wild and investigate -- these are existing UAFs.
==========

to

==========
Make CPDF_Array not do indirect object creation.

We remove the indirect object holder argument and check that
call sites pass ownable objects, adding a reference in one
place that always was passing an indirect object.

Also check that the invariant isn't violated, we need to fail
here in the wild and investigate -- these are existing UAFs.

Committed:
https://pdfium.googlesource.com/pdfium/+/5a399de2945d7b244802565d8e9d2f6e6625...
==========

[commit-bot: I haz the power, 2016-09-20 20:23:24]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/5a399de2945d7b244802565d8e9d2f6e6625...