Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(245)

Issue 818193004: XFA: merge patch from CL 729293003, use FX_ArraySize for safety (Closed)

Created:
6 years ago by brucedawson
Modified:
5 years, 11 months ago
Reviewers:
Bo Xu, Tom Sepez
CC:
pdfium-reviews_googlegroups.com
Base URL:
https://pdfium.googlesource.com/pdfium.git@xfa
Target Ref:
refs/heads/xfa
Visibility:
Public.

Description

XFA: merge patch from CL 729293003, use FX_ArraySize for safety Note that the merge of this fix to XFA found six bugs. Five were fixed in https://codereview.chromium.org/826573003 and one was fixed in https://codereview.chromium.org/831293002. These bugs are now impossible to compile. Replace manual/error-prone/hard-to-verify arraysize calculations with safe FX_ArraySize macro. pdfium has numerous places where the number of elements in an array is calculated with expressions like: sizeof(cFormats)/sizeof(FX_LPCWSTR) This is suboptimal because it is verbose, it is easy to get wrong, and it cannot be determined through casual inspection whether the code is correct. It will give incorrect results if cFormats is a pointer instead of an array and it will give incorrect results if FX_LPCWSTR is not the type of the array elements. The FX_WSTRC macro in fx_string.h which I fixed was particularly scary because it would silently misbehave if passed a pointer. The FX_ArraySize macro which I have added and started using (taken from arraysize in v8's macros.h) is easier to use and will always give correct results. If passed a pointer it will fail to compile. For this change I only fixed instances of sizeof(FX_LPCWSTR). There appear to be about 150 other places in the pdfium code that could benefit from using FX_ArraySize. TBR=bo_xu@foxitsoftware.com, tsepez@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/5c57933509aedb46399d320da16dfcc3e5acf28d

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+26 lines, -10 lines) Patch
M core/include/fxcrt/fx_basic.h View 1 chunk +16 lines, -0 lines 0 comments Download
M core/include/fxcrt/fx_string.h View 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/include/javascript/JS_Define.h View 1 chunk +1 line, -1 line 0 comments Download
M fpdfsdk/src/javascript/PublicMethods.cpp View 4 chunks +8 lines, -8 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
brucedawson
Merge to XFA. Still working out the optimal process.
6 years ago (2014-12-23 20:27:19 UTC) #1
brucedawson
Landing this patch is on hold pending a resolution of https://code.google.com/p/pdfium/issues/detail?id=96.
6 years ago (2014-12-23 21:37:30 UTC) #2
brucedawson
Landing this page then went on hold pending a resolution of: https://codereview.chromium.org/831293002 But all is ...
5 years, 11 months ago (2015-01-05 19:50:03 UTC) #3
brucedawson
Committed patchset #1 (id:1) manually as 5c57933509aedb46399d320da16dfcc3e5acf28d (presubmit successful).
5 years, 11 months ago (2015-01-05 19:53:22 UTC) #4
brucedawson
Change committed, TBR FYI.
5 years, 11 months ago (2015-01-05 19:54:32 UTC) #5
Tom Sepez
5 years, 11 months ago (2015-01-05 19:58:23 UTC) #6
Message was sent while issue was closed.
lgtm

Powered by Google App Engine
This is Rietveld 408576698