Fix crashing in CPDFXFA_Document::GetPage()

Fix crashing in CPDFXFA_Document::GetPage()

BUG=pdfium:332
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/752395430376536487352815e6107c32dac7d69e

[jun_fang, 2015-12-21 07:19:59]

jun_fang@foxitsoftware.com changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org, zhiquan_ye@foxitsoftware.com

[jun_fang, 2015-12-21 07:19:59]

Please review this CL. Thanks!

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
File fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp (right):

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:488: if (!pPageView ||
(FXFA_PAGEVIEWEVENT_POSTADDED != dwFlags &&
SDK needs to handle changes on page view.
The events include FXFA_PAGEVIEWEVENT_POSTADDED and
FXFA_PAGEVIEWEVENT_POSTREMOVED.

[Lei Zhang, 2015-12-21 17:47:59]

Is there any way to write a test for this?

If yes, can we use a smaller hand crafted test file? pdfium_332.pdf is 2.7 MB.

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
File fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp (left):

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:148: if (!m_pPDFDoc && !m_pXFADoc)
Do you still care about these being nullptrs?

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
File fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp (right):

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:147: CPDFXFA_Page*
CPDFXFA_Document::GetPage(int page_index) {
What happen if |page_index| is negative?

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:487: FX_DWORD dwFlags) {
Is |dwFlags| always just a single value, or can it be something like
(XFA_PAGEVIEWEVENT_PostAdded | XFA_PAGEVIEWEVENT_PostRemoved) ?

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:488: if (!pPageView ||
(FXFA_PAGEVIEWEVENT_POSTADDED != dwFlags &&
On 2015/12/21 07:19:59, jun_fang wrote:
> SDK needs to handle changes on page view.
> The events include FXFA_PAGEVIEWEVENT_POSTADDED and
> FXFA_PAGEVIEWEVENT_POSTREMOVED.

The posted value is XFA_PAGEVIEWEVENT_PostRemoved, but the checked value is
FXFA_PAGEVIEWEVENT_POSTREMOVED. Can we use one of the two consistently?

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:493: if (FXFA_PAGEVIEWEVENT_POSTADDED ==
dwFlags) {
nit: Prefer to just write: var == value

I believe Clang at least, will warn if one accidentally writes: var = value

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp
File fpdfsdk/src/fsdk_mgr.cpp (right):

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fsdk_mgr.cp...
fpdfsdk/src/fsdk_mgr.cpp:1124: if (m_pSDKDoc && GetFocusAnnot())
Why bother calling GetFocusAnnot() ?

[jun_fang, 2016-01-04 09:30:24]

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
File fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp (right):

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:147: CPDFXFA_Page*
CPDFXFA_Document::GetPage(int page_index) {
On 2015/12/21 17:47:59, Lei Zhang wrote:
> What happen if |page_index| is negative?

It shouldn't be less than 0. Will add a test to make sure |page_index| >= 0.

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:487: FX_DWORD dwFlags) {
On 2015/12/21 17:47:58, Lei Zhang wrote:
> Is |dwFlags| always just a single value, or can it be something like
> (XFA_PAGEVIEWEVENT_PostAdded | XFA_PAGEVIEWEVENT_PostRemoved) ?

It's just a single value.

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:488: if (!pPageView ||
(FXFA_PAGEVIEWEVENT_POSTADDED != dwFlags &&
On 2015/12/21 17:47:59, Lei Zhang wrote:
> On 2015/12/21 07:19:59, jun_fang wrote:
> > SDK needs to handle changes on page view.
> > The events include FXFA_PAGEVIEWEVENT_POSTADDED and
> > FXFA_PAGEVIEWEVENT_POSTREMOVED.
> 
> The posted value is XFA_PAGEVIEWEVENT_PostRemoved, but the checked value is
> FXFA_PAGEVIEWEVENT_POSTREMOVED. Can we use one of the two consistently?

Acknowledged.

https://codereview.chromium.org/1539193003/diff/20001/fpdfsdk/src/fpdfxfa/fpd...
fpdfsdk/src/fpdfxfa/fpdfxfa_doc.cpp:493: if (FXFA_PAGEVIEWEVENT_POSTADDED ==
dwFlags) {
On 2015/12/21 17:47:58, Lei Zhang wrote:
> nit: Prefer to just write: var == value
> 
> I believe Clang at least, will warn if one accidentally writes: var = value

Acknowledged.

[jun_fang, 2016-01-04 10:11:16]

On 2015/12/21 17:47:59, Lei Zhang wrote:
> Is there any way to write a test for this?
> 
> If yes, can we use a smaller hand crafted test file? pdfium_332.pdf is 2.7 MB.
> 

Good idea! I think that we can do as you suggested. However, it may take time
to analyze the structure of XFA file because the struct of XFA is much more
complicated
than PDF's. Furthermore, Foxit didn't do tests in this way. We don't have
experience
on this.

[rpop, 2016-01-12 19:28:27]

On 2016/01/04 10:11:16, jun_fang wrote:
> On 2015/12/21 17:47:59, Lei Zhang wrote:
> > Is there any way to write a test for this?
> > 
> > If yes, can we use a smaller hand crafted test file? pdfium_332.pdf is 2.7
MB.
> > 
> 
> Good idea! I think that we can do as you suggested. However, it may take time
> to analyze the structure of XFA file because the struct of XFA is much more
> complicated
> than PDF's. Furthermore, Foxit didn't do tests in this way. We don't have
> experience
> on this.

Jun, are you working on this? Can you provide an ETA for updating this CL with
the test requested?

[jun_fang, 2016-01-13 00:02:20]

On 2016/01/12 19:28:27, rpop wrote:
> On 2016/01/04 10:11:16, jun_fang wrote:
> > On 2015/12/21 17:47:59, Lei Zhang wrote:
> > > Is there any way to write a test for this?
> > > 
> > > If yes, can we use a smaller hand crafted test file? pdfium_332.pdf is 2.7
> MB.
> > > 
> > 
> > Good idea! I think that we can do as you suggested. However, it may take
time
> > to analyze the structure of XFA file because the struct of XFA is much more
> > complicated
> > than PDF's. Furthermore, Foxit didn't do tests in this way. We don't have
> > experience
> > on this.
> 
> Jun, are you working on this? Can you provide an ETA for updating this CL with
> the test requested?

What does ETA mean here?

[Lei Zhang, 2016-01-13 00:59:14]

On 2016/01/13 00:02:20, jun_fang wrote:
> What does ETA mean here?

ETA -> Estimated Time of Arrival -> When will we have a test case to add to this
CL?

[jun_fang, 2016-01-13 02:49:22]

On 2016/01/13 00:59:14, Lei Zhang wrote:
> On 2016/01/13 00:02:20, jun_fang wrote:
> > What does ETA mean here?
> 
> ETA -> Estimated Time of Arrival -> When will we have a test case to add to
this
> CL?

Thanks! Plan to finish by end of this week.

[jun_fang, 2016-01-19 11:34:29]

On 2016/01/13 02:49:22, jun_fang wrote:
> On 2016/01/13 00:59:14, Lei Zhang wrote:
> > On 2016/01/13 00:02:20, jun_fang wrote:
> > > What does ETA mean here?
> > 
> > ETA -> Estimated Time of Arrival -> When will we have a test case to add to
> this
> > CL?
> 
> Thanks! Plan to finish by end of this week.

We are still working on creating a small enough test file.
We tried but failed to reproduce with those files.

[Tom Sepez, 2016-01-19 21:32:05]

On 2016/01/19 11:34:29, jun_fang wrote:
> On 2016/01/13 02:49:22, jun_fang wrote:
> > On 2016/01/13 00:59:14, Lei Zhang wrote:
> > > On 2016/01/13 00:02:20, jun_fang wrote:
> > > > What does ETA mean here?
> > > 
> > > ETA -> Estimated Time of Arrival -> When will we have a test case to add
to
> > this
> > > CL?
> > 
> > Thanks! Plan to finish by end of this week.
> 
> We are still working on creating a small enough test file.
> We tried but failed to reproduce with those files.

Ok, lets land this change as-is then and the test afterwards.

[Tom Sepez, 2016-01-19 21:33:00]

lgtm

[jun_fang, 2016-01-20 00:05:03]

Description was changed from

==========
Fix crashing in CPDFXFA_Document::GetPage()

BUG=pdfium:332
==========

to

==========
Fix crashing in CPDFXFA_Document::GetPage()

BUG=pdfium:332
R=tsepez@chromium.org

Committed:
https://pdfium.googlesource.com/pdfium/+/752395430376536487352815e6107c32dac7...
==========

[jun_fang, 2016-01-20 00:05:07]

Committed patchset #5 (id:80001) manually as
752395430376536487352815e6107c32dac7d69e (presubmit successful).