Fix the problem that memory is accessed after released due to invalid type-cast

Fix the problem that memory is accessed after released due to invalid type-cast

BUG=387774
R=palmer@chromium.org, tsepez@chromium.org

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

[jun_fang, 2014-08-02 23:22:45]

Hi Tom, please review this fix. Thanks!

[Tom Sepez, 2014-08-04 17:51:02]

On 2014/08/02 23:22:45, jun_fang wrote:
> Hi Tom, please review this fix. Thanks!
LGTM.

I did a quick manual inspection to see if I could find other occurrences of this
pattern. I didn't see any others apart from some cases where we cast first and
check later.  While this is safe at the machine instruction level, it looks
wrong, and some tools may someday flag it.  There probably should be a follow-up
CL to change that.  For example:

doc_tagged.cpp:195 - Probably safe in practice, but we should check first then
cast rather than other way around:
            CPDF_Reference* pKidRef = (CPDF_Reference*)pTopKids->GetElement(i);
            if (pKidRef->GetType() != PDFOBJ_REFERENCE ||
pKidRef->GetRefObjNum() != pDict->GetObjNum()) {
                continue;

fpdf_parser_parser.cpp:1130,1138,1597 - again, check first and then cast
   CPDF_Reference* pRef = m_pTrailer ?
(CPDF_Reference*)m_pTrailer->GetElement(FX_BSTRC("Root")) : NULL;
    if (pRef == NULL || pRef->GetType() != PDFOBJ_REFERENCE) {

fpdf_parser_document:195,348 - again, check first then cast.

[palmer, 2014-08-04 18:34:20]

> fpdf_parser_parser.cpp:1130,1138,1597 - again, check first and then cast
>    CPDF_Reference* pRef = m_pTrailer ?
> (CPDF_Reference*)m_pTrailer->GetElement(FX_BSTRC("Root")) : NULL;
>     if (pRef == NULL || pRef->GetType() != PDFOBJ_REFERENCE) {

Wouldn't it be best to replace this "homebrew RTTI" with real
dynamic_cast<Foo>(...)?

[Tom Sepez, 2014-08-04 18:41:52]

> Wouldn't it be best to replace this "homebrew RTTI" with real
> dynamic_cast<Foo>(...)?
I'm pretty sure we compile this with -fno-rtti so this won't work (and if we're
not, we should).

[palmer, 2014-08-04 18:44:13]

> I'm pretty sure we compile this with -fno-rtti so this won't work (and if
we're
> not, we should).

Why? This code needs RTTI, and it seems like the standard way is better than
homebrew. By saying -fno-rtti, we aren't actually saving anything.

[jun_fang, 2014-08-06 20:16:11]

Hi guys, please review the fix again.

[palmer, 2014-08-06 20:23:04]

lgtm

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

https://codereview.chromium.org/441503003/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1598: }
Remove this trailing whitespace.

[jun_fang, 2014-08-06 21:06:49]

Removed trailing whitespace

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

https://codereview.chromium.org/441503003/diff/60001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1598: }
On 2014/08/06 20:23:04, Chromium Palmer wrote:
> Remove this trailing whitespace.

OK.

[jun_fang, 2014-08-06 21:10:55]

Committed patchset #5 manually as ref72d48 (presubmit successful).