Fix a bug that occurs when an object has the same object number with the root object

Fix a bug that occurs when an object has the same object number with the root object

Before this fix, the root will be released when an indirect object has the
same object number with the root. However, the root object is loaded when
the trailer is parsed. It shall not be updated or replaced anymore.

BUG=425040
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/dba5bfd174589ad08c4231b039297b59fa2ccc3b

[jun_fang, 2014-12-15 16:48:15]

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

[jun_fang, 2014-12-15 16:48:15]

Hi guys, please start to review this fix. Thanks!

[Bo Xu, 2014-12-15 17:49:25]

https://codereview.chromium.org/803103002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/803103002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1005: if (m_pDocument &&
m_pDocument->GetRoot()->GetObjNum() != pStream->m_ObjNum) {
Could GetRoot() return null?

[jun_fang, 2014-12-15 18:46:48]

https://codereview.chromium.org/803103002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/803103002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1005: if (m_pDocument &&
m_pDocument->GetRoot()->GetObjNum() != pStream->m_ObjNum) {
On 2014/12/15 17:49:25, Bo Xu wrote:
> Could GetRoot() return null?

m_pRootDict is returned from GetRoot().It's assigned to a root dictionary during
the process of loading documents. If m_pDocument is not NULL, that means the
document is loaded successfully. Otherwise, it can't get here. 

There are two ways to load a document, FPDF_LoadCustomDocument and
FPDFAvail_GetDocument. In both of them, 'm_pDocument->GetRoot() == NULL' is
checked in the functions of CPDF_Parser::StartParse and
CPDF_Parser::StartAsynParse.

[Tom Sepez, 2014-12-15 19:33:43]

https://codereview.chromium.org/803103002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/803103002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1005: if (m_pDocument &&
m_pDocument->GetRoot()->GetObjNum() != pStream->m_ObjNum) {
How about returning false if this is the case? Is there a reason to continue?

[jun_fang, 2014-12-15 20:17:19]

On 2014/12/15 19:33:43, Tom Sepez wrote:
>
https://codereview.chromium.org/803103002/diff/20001/core/src/fpdfapi/fpdf_pa...
> File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):
> 
>
https://codereview.chromium.org/803103002/diff/20001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1005: if (m_pDocument &&
> m_pDocument->GetRoot()->GetObjNum() != pStream->m_ObjNum) {
> How about returning false if this is the case? Is there a reason to continue?

Updated for your comments. Please check #Patch 3.

[Tom Sepez, 2014-12-17 18:20:10]

https://codereview.chromium.org/803103002/diff/40001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/803103002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1005: if (m_pDocument &&
m_pDocument->GetRoot()->GetObjNum() != pStream->m_ObjNum) {
If there's no document, we now fail instead of just continuing.  I think you
need a nested if here.

https://codereview.chromium.org/803103002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1013: if (pStream->GetType()
!= PDFOBJ_STREAM) {
can this test move to after line 1004?  Then when you fail at line 1008, there's
no need to release.

[Tom Sepez, 2014-12-17 18:21:28]

https://codereview.chromium.org/803103002/diff/40001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/803103002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1013: if (pStream->GetType()
!= PDFOBJ_STREAM) {
On 2014/12/17 18:20:10, Tom Sepez wrote:
> can this test move to after line 1004?  Then when you fail at line 1008,
there's
> no need to release.
Rather, there's no need to check before calling release.  You can avoid an if
block, thats all. Sorry about confusion.

[jun_fang, 2014-12-17 19:55:53]

https://codereview.chromium.org/803103002/diff/40001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/803103002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1005: if (m_pDocument &&
m_pDocument->GetRoot()->GetObjNum() != pStream->m_ObjNum) {
On 2014/12/17 18:20:10, Tom Sepez wrote:
> If there's no document, we now fail instead of just continuing.  I think you
> need a nested if here.

I thought that it should return if there was no document. I checked the related
logic again and think that it shall keep the original logic unchanged.

https://codereview.chromium.org/803103002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1013: if (pStream->GetType()
!= PDFOBJ_STREAM) {
On 2014/12/17 18:21:28, Tom Sepez wrote:
> On 2014/12/17 18:20:10, Tom Sepez wrote:
> > can this test move to after line 1004?  Then when you fail at line 1008,
> there's
> > no need to release.
> Rather, there's no need to check before calling release.  You can avoid an if
> block, thats all. Sorry about confusion.

Before this fix, release is only called when pStream's type is PDFOBJ_STREAM. So
I add the check before calling release.

[Tom Sepez, 2014-12-17 20:07:54]

Ok. LGTM, you can address those issues as you like.

[jun_fang, 2014-12-17 21:59:28]

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