Check NULL pointer dereferencing from GetDirect

Check NULL pointer dereferencing from GetDirect

BUG=431770
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/0c32f02cf5dabd80c03f3af34874b5f84535b1ef

[Bo Xu, 2014-11-14 22:35:49]

Hi Tom, please review this one.

[Bo Xu, 2014-11-14 22:37:09]

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

[Bo Xu, 2014-11-14 22:37:34]

Hi Tom, please review this one.

[Tom Sepez, 2014-11-14 22:41:45]

https://codereview.chromium.org/728993002/diff/1/xfa/src/fxfa/src/app/xfa_ffd...
File xfa/src/fxfa/src/app/xfa_ffdoc.cpp (right):

https://codereview.chromium.org/728993002/diff/1/xfa/src/fxfa/src/app/xfa_ffd...
xfa/src/fxfa/src/app/xfa_ffdoc.cpp:244: return FALSE;
Doesn't getDirect() check the object type, and return |this| so that checking
the type here is redundant?  We could save some code if we always called
getDirect().

[Tom Sepez, 2014-11-14 22:44:42]

https://codereview.chromium.org/728993002/diff/1/xfa/src/fxfa/src/app/xfa_ffd...
File xfa/src/fxfa/src/app/xfa_ffdoc.cpp (right):

https://codereview.chromium.org/728993002/diff/1/xfa/src/fxfa/src/app/xfa_ffd...
xfa/src/fxfa/src/app/xfa_ffdoc.cpp:237: if (pElementXFA == NULL) {
Actually, doesn't GetElementValue() do the indirection resolution for you?

[Bo Xu, 2014-11-14 23:44:53]

https://codereview.chromium.org/728993002/diff/1/xfa/src/fxfa/src/app/xfa_ffd...
File xfa/src/fxfa/src/app/xfa_ffdoc.cpp (right):

https://codereview.chromium.org/728993002/diff/1/xfa/src/fxfa/src/app/xfa_ffd...
xfa/src/fxfa/src/app/xfa_ffdoc.cpp:244: return FALSE;
On 2014/11/14 22:41:44, Tom Sepez wrote:
> Doesn't getDirect() check the object type, and return |this| so that checking
> the type here is redundant?  We could save some code if we always called
> getDirect().

Right! I should have seen this in the first place.

[Tom Sepez, 2014-11-14 23:58:44]

https://codereview.chromium.org/728993002/diff/20001/xfa/src/fxfa/src/app/xfa...
File xfa/src/fxfa/src/app/xfa_ffdoc.cpp (right):

https://codereview.chromium.org/728993002/diff/20001/xfa/src/fxfa/src/app/xfa...
xfa/src/fxfa/src/app/xfa_ffdoc.cpp:237: pElementXFA = pElementXFA->GetDirect();
Need to check for null before indirection.

[Bo Xu, 2014-11-15 00:09:18]

https://codereview.chromium.org/728993002/diff/1/xfa/src/fxfa/src/app/xfa_ffd...
File xfa/src/fxfa/src/app/xfa_ffdoc.cpp (right):

https://codereview.chromium.org/728993002/diff/1/xfa/src/fxfa/src/app/xfa_ffd...
xfa/src/fxfa/src/app/xfa_ffdoc.cpp:237: if (pElementXFA == NULL) {
On 2014/11/14 22:44:41, Tom Sepez wrote:
> Actually, doesn't GetElementValue() do the indirection resolution for you?

Done.

[Tom Sepez, 2014-11-15 00:12:19]

LGTM.

Sometime down the road, can I beg you guys to change the name of
GetElementValue() to GetDirectElement() for consistency with GetDirectInteger()
and so forth?

[Bo Xu, 2014-11-15 00:18:41]

On 2014/11/15 00:12:19, Tom Sepez wrote:
> LGTM.
> 
> Sometime down the road, can I beg you guys to change the name of
> GetElementValue() to GetDirectElement() for consistency with
GetDirectInteger()
> and so forth?

I totally agree. A lot of bad naming exists in the current code. Jun and I
sometimes get confused too :(

[Bo Xu, 2014-11-15 01:22:47]

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