Fix use-after-free in CPDF_Color::~CPDF_Color

The root cause of this issue is shown as below:
Patterns are managed in CPDF_DocPageData. When
a document is closed, all patterns will be
released in the deconstruction of CPDF_DocPageData.
However, some patterns which are referenced in
CPDF_Color can't get the notification from the
destroy of CPDF_DocPageData. It will cause
use-after-free in CPDF_Color::~CPDF_Color.

BUG=392719
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/1b9c5c4

[jun_fang, 2014-08-04 03:23:44]

Hi All,

Please start to review it.

[Tom Sepez, 2014-08-04 18:02:32]

https://codereview.chromium.org/439693002/diff/1/core/include/fpdfapi/fpdf_re...
File core/include/fpdfapi/fpdf_resource.h (right):

https://codereview.chromium.org/439693002/diff/1/core/include/fpdfapi/fpdf_re...
core/include/fpdfapi/fpdf_resource.h:733: 
Too bad this doesn't have a constructor to set up the pointer members in all
cases.

https://codereview.chromium.org/439693002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/439693002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:15: m_pColor = NULL;
This can move inside the if {} since its already NULL if the if-branch isn't
taken.

[palmer, 2014-08-04 18:17:11]

https://codereview.chromium.org/439693002/diff/1/core/include/fpdfapi/fpdf_re...
File core/include/fpdfapi/fpdf_resource.h (right):

https://codereview.chromium.org/439693002/diff/1/core/include/fpdfapi/fpdf_re...
core/include/fpdfapi/fpdf_resource.h:733: 
> Too bad this doesn't have a constructor to set up the pointer members in all
> cases.

We need to start making sure every class and struct has the constructors it
needs, and that all constructors assert at least minimal correctness and safety
invariants. Now is a good time to start. :)

https://codereview.chromium.org/439693002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/439693002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:15: m_pColor = NULL;
> This can move inside the if {} since its already NULL if the if-branch isn't
> taken.

Alternately, save a level of indentation:

if (!m_pColor)
    return;

m_pColor->SetValue(NULL, NULL, 0);
m_pColor = NULL;

[jun_fang, 2014-08-04 18:37:11]

please review it again.

https://codereview.chromium.org/439693002/diff/1/core/include/fpdfapi/fpdf_re...
File core/include/fpdfapi/fpdf_resource.h (right):

https://codereview.chromium.org/439693002/diff/1/core/include/fpdfapi/fpdf_re...
core/include/fpdfapi/fpdf_resource.h:733: 
On 2014/08/04 18:02:32, Tom Sepez wrote:
> Too bad this doesn't have a constructor to set up the pointer members in all
> cases.

This class is a interface class. It doesn't need to be instantiated.So it just
have a protected constructor CPDF_Pattern(const CFX_AffineMatrix*
pParentMatrix). I will add the member variables initialization in this
constructor.

https://codereview.chromium.org/439693002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/439693002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:15: m_pColor = NULL;
On 2014/08/04 18:02:32, Tom Sepez wrote:
> This can move inside the if {} since its already NULL if the if-branch isn't
> taken.

Acknowledge

[Tom Sepez, 2014-08-05 17:34:15]

https://codereview.chromium.org/439693002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/439693002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:12: m_pPatternObj = NULL;
nit: these can (and should) occur as part of a : - style initializer list before
the first {.

https://codereview.chromium.org/439693002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:26: m_pColor = NULL;
Do we leak m_pColor here? Who owns this memory?

https://codereview.chromium.org/439693002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:35: m_pColor = NULL;
m_pColor is nulled in the parent class constructor.

[jun_fang, 2014-08-05 18:21:40]

Hi Tom, update per your comments. please review it again.

https://codereview.chromium.org/439693002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp (right):

https://codereview.chromium.org/439693002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:12: m_pPatternObj = NULL;
On 2014/08/05 17:34:15, Tom Sepez wrote:
> nit: these can (and should) occur as part of a : - style initializer list
before
> the first {.

OK. I will update them in the next patch.

https://codereview.chromium.org/439693002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:26: m_pColor = NULL;
On 2014/08/05 17:34:14, Tom Sepez wrote:
> Do we leak m_pColor here? Who owns this memory?

No. m_pColor refers to the object of CPDF_Color by calling
pPattern->SaveColor(this). We save a reference, m_pColor, in CPDF_Pattern so
that we can inform CPDF_Color to set the saved pointer of pattern to NULL when
the object of CPDF_Pattern is deconstructed.

https://codereview.chromium.org/439693002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_pattern.cpp:35: m_pColor = NULL;
On 2014/08/05 17:34:14, Tom Sepez wrote:
> m_pColor is nulled in the parent class constructor.

I will update it in the next patch.

[Tom Sepez, 2014-08-06 18:46:03]

lgtm

[jun_fang, 2014-08-06 19:15:38]

Committed patchset #3 manually as r1b9c5c4 (presubmit successful).