Store the address of the page data map's value for proper referencing.

Store the address of the page data map's value for proper referencing.

CPDF_Pattern objects are counted and maintained in m_PatternedMap.
When a CPDF_Pattern object "pattern" is deleted, it's address is marked as NULL in m_PatternMap.
This patch stores the address of CPDF_Pattern's adderss in all objects that references "pattern",
to ensure valid referencing after deletion.

BUG=416319, 419976, 418392
R=tsepez@chromium.org

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

[Bo Xu, 2014-10-14 17:12:40]

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

[Bo Xu, 2014-10-14 17:13:27]

Hi Tom, I tested the patch on past security issues, did not see regression.

[Tom Sepez, 2014-10-14 18:49:23]

Seems like the right fix is that the color object should hold a pointer to the
CountedObject<> and increment/decrement its ref counts.  This is hard to do
without a smart pointer class, though.  If you want to go with this approach,
then see comments.

https://codereview.chromium.org/656753002/diff/20001/core/include/fpdfapi/fpd...
File core/include/fpdfapi/fpdf_resource.h (right):

https://codereview.chromium.org/656753002/diff/20001/core/include/fpdfapi/fpd...
core/include/fpdfapi/fpdf_resource.h:795: CPDF_ColorSpace**	m_pPtrCS;
What if there's more than one Color that has a reference to this CS? If so, then
this seems like a use case for something like chromium's base/linked_ptr.h. 
Otherwise, it it always a Color object that contains the stale pointer?  If so,
I'd make this a CPDF_Color* m_pBackPointer; and later clear
pattern->m_pBackPointer->m_pPattern (or whatever its called).

[Tom Sepez, 2014-10-14 20:39:31]

https://codereview.chromium.org/656753002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/pageint.h (right):

https://codereview.chromium.org/656753002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/pageint.h:434: CPDF_ColorSpace**          
FindColorSpacePtr(CPDF_Object* pCSObj);
nit:  are these const methods?  Is the arg a const CPDF_Object* ?

[Bo Xu, 2014-10-14 20:41:06]

https://codereview.chromium.org/656753002/diff/20001/core/include/fpdfapi/fpd...
File core/include/fpdfapi/fpdf_resource.h (right):

https://codereview.chromium.org/656753002/diff/20001/core/include/fpdfapi/fpd...
core/include/fpdfapi/fpdf_resource.h:795: CPDF_ColorSpace**	m_pPtrCS;
On 2014/10/14 18:49:23, Tom Sepez wrote:
> What if there's more than one Color that has a reference to this CS? If so,
then
> this seems like a use case for something like chromium's base/linked_ptr.h. 
> Otherwise, it it always a Color object that contains the stale pointer?  If
so,
> I'd make this a CPDF_Color* m_pBackPointer; and later clear
> pattern->m_pBackPointer->m_pPattern (or whatever its called).
> 

Sorry, I didn't quite get the meaning of CPDF_Color* m_pBackPointer;

m_pCS is counted by m_ColorSpaceMap and it can be referenced by more than one
CPDF_Color object or CPDF_Pattern object (by inc or dec the counter).

Suppose in m_ColorSpaceMap, there is a <key, value> pair:
<pCSObj, csData> that corresponds to m_pCS, then m_pPtrCS = &(csData->m_Obj)

[Bo Xu, 2014-10-14 20:53:34]

https://codereview.chromium.org/656753002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/pageint.h (right):

https://codereview.chromium.org/656753002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/pageint.h:434: CPDF_ColorSpace**          
FindColorSpacePtr(CPDF_Object* pCSObj);
On 2014/10/14 20:39:31, Tom Sepez wrote:
> nit:  are these const methods?  Is the arg a const CPDF_Object* ?

They are const methods, I will put that in.

[Tom Sepez, 2014-10-14 21:01:34]

Gotcha.  Your map is actually a map<CPDF_Object*,
CPDF_CountedObject<CPDF_ColorSpace*>*>, so I'd prefer that you had:

  	CPDF_CountedObject<CPDF_ColorSpace*>* m_pPtrCS;

through which you can maniuplate ref counts if desired down the road, and it
avoids taking the address of the internal member of the
CPDF_CountedObject<CPDF_ColorSpace*> which could be private down the road.

Having done this, m_pCS is redundant, since you can always get to the object
through the CountedObject handle, but you'll keep it to avoid changing lots of
code (and its one hop faster anyways).  m_pCS and m_pPtrCS eventually lead to
the same CS, right?  m_pCS then gets commented as // cached copy of
m_PtrCS->m_obj.

[Tom Sepez, 2014-10-14 21:18:23]

In other words, I'd someday like to change this to:
    std::map<CPDF_Object*, std::shared_ptr<CPDF_ColorSpace>> m_ColorSpaceMap;

and:
    std::shared_ptr<CPDF_ColorSpace> m_pPtrCS;

and this gets us closer to that model.

[Bo Xu, 2014-10-14 21:21:07]

On 2014/10/14 21:01:34, Tom Sepez wrote:
> Gotcha.  Your map is actually a map<CPDF_Object*,
> CPDF_CountedObject<CPDF_ColorSpace*>*>, so I'd prefer that you had:
> 
>   	CPDF_CountedObject<CPDF_ColorSpace*>* m_pPtrCS;
> 
> through which you can maniuplate ref counts if desired down the road, and it
> avoids taking the address of the internal member of the
> CPDF_CountedObject<CPDF_ColorSpace*> which could be private down the road.
> 
> Having done this, m_pCS is redundant, since you can always get to the object
> through the CountedObject handle, but you'll keep it to avoid changing lots of
> code (and its one hop faster anyways).  m_pCS and m_pPtrCS eventually lead to
> the same CS, right?  m_pCS then gets commented as // cached copy of
> m_PtrCS->m_obj.

Right. And I was thinking of removing m_pCS to but thought should keep it here
for now.

I really like your idea. It makes more sense and straightforward to use
CPDF_CountedObject<CPDF_ColorSpace*>* m_pPtrCS. Let me give it a try.

[Bo Xu, 2014-10-14 22:47:00]

On 2014/10/14 21:18:23, Tom Sepez wrote:
> In other words, I'd someday like to change this to:
>     std::map<CPDF_Object*, std::shared_ptr<CPDF_ColorSpace>> m_ColorSpaceMap;
> 
> and:
>     std::shared_ptr<CPDF_ColorSpace> m_pPtrCS;
> 
> and this gets us closer to that model.

Hi Tom, please see Patch 3. m_pCS still needs to be kept for now. The reason is
the map only stores indirect objects in PDF and direct object like
(pCSObj->GetType() == PDFOBJ_NAME) are not stored there. We could re-factor and
unify this in the future.

[Tom Sepez, 2014-10-14 23:07:44]

I think this is getting close.  Just some nits plus one question.  You needn't
consider the follow-on work just yet.

https://codereview.chromium.org/656753002/diff/40001/core/include/fpdfapi/fpd...
File core/include/fpdfapi/fpdf_resource.h (right):

https://codereview.chromium.org/656753002/diff/40001/core/include/fpdfapi/fpd...
core/include/fpdfapi/fpdf_resource.h:50: typedef
CFX_MapPtrTemplate<CPDF_Dictionary*,
CPDF_CountedObject<CPDF_Font*>*>		CPDF_FontMap;
nit: we might as well make typedefs for all of these, since they're easier to
understand.

https://codereview.chromium.org/656753002/diff/40001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/656753002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:150: FX_POSITION pos = NULL;
nit: no need to initialize since we assign in the next line?

https://codereview.chromium.org/656753002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:151: pos =
m_PatternMap.GetStartPosition();
nit: for future cleanup, there should be a method of the templated map class
(maybe called deleteAll or something) that does this.

https://codereview.chromium.org/656753002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:184: while (pos) {
nit: for future cleanup, again this should be a method of the templated map
class.  There's too much boilerplate here.

https://codereview.chromium.org/656753002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:244: delete ipData;
Not clear to me why some of these delete their countedobjects but others wait
til later?  Its probably fine, but I'd like to understand why.

https://codereview.chromium.org/656753002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:678: CPDF_CountedColorSpace*
CPDF_DocPageData::FindColorSpacePtr(CPDF_Object* pCSObj) const
nit:  Again, this might be a method of the templated map class to save some
boilerplate code.

[Bo Xu, 2014-10-14 23:37:49]

I will resolve all the nits in another patch

https://codereview.chromium.org/656753002/diff/40001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/656753002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:244: delete ipData;
On 2014/10/14 23:07:44, Tom Sepez wrote:
> Not clear to me why some of these delete their countedobjects but others wait
> til later?  Its probably fine, but I'd like to understand why.

For colorspace, pattern and font which are managed in their corresponding map, I
deter the deletion.

This is because, if |pattern1| = |ptData->mObj| is deleted, ptData->mObj is set
to NULL and we still want to keep ptData alive. Then when deleting pattern2 or
colorspace3 (they reference pattern1), we can check:

(assuming m_pCountedBaseCS  = ptData)
CPDF_ColorSpace* pCS = m_pCountedBaseCS ? m_pCountedBaseCS->m_Obj : NULL;

[Tom Sepez, 2014-10-15 00:05:35]

lgtm

[Bo Xu, 2014-10-15 00:10:16]

Committed patchset #4 (id:60001) manually as
e1177425a656f915657f948d965193a019702a52 (presubmit successful).

[Bo Xu, 2014-10-15 21:31:26]

https://codereview.chromium.org/656753002/diff/60001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/656753002/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:219: csData->m_Obj->ReleaseCS();
Hi Tom, I am trying to put them in a member function in fx_basic.h, but the
colorSpace here uses ReleaseCS instead of delete as others.

Any suggestion for this? I tried to overload "delete" for CPDF_ColorSpace and
use ReleaseCS inside, but here does not seem to recognize the overloaded delete
though...

[Tom Sepez, 2014-10-15 22:05:37]

https://codereview.chromium.org/656753002/diff/60001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/656753002/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:219: csData->m_Obj->ReleaseCS();
On 2014/10/15 21:31:26, Bo Xu wrote:
> Hi Tom, I am trying to put them in a member function in fx_basic.h, but the
> colorSpace here uses ReleaseCS instead of delete as others.
> 
> Any suggestion for this? I tried to overload "delete" for CPDF_ColorSpace and
> use ReleaseCS inside, but here does not seem to recognize the overloaded
delete
> though...

As a first step, I think its fine to keep this special case as-is.