Fix bounds checking in CJS_PublicMethods::MakeRegularDate().

Fix bounds checking in CJS_PublicMethods::MakeRegularDate().

The function is looking ahead N characters at both its "format" and "value"
strings without validating that accesses are in bounds.  Add those validations.

There are also duplicate checks in the else-branches which re-test the inverse
of the if-branch.  These are removed for simplicity.

I also tidied some stray whitespace in the function while I was at it.

BUG=393831
R=jun_fang@foxitsoftware.com

Committed: https://pdfium.googlesource.com/pdfium/+/5ffacd6

[Tom Sepez, 2014-07-16 21:54:59]

John, Jun, Please review.  Thanks.

[jam, 2014-07-17 17:43:27]

On 2014/07/16 21:54:59, Tom Sepez wrote:
> John, Jun, Please review.  Thanks.

I defer to Jun since I'm not familiar with this code.

[jun_fang, 2014-07-18 17:30:59]

https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
File fpdfsdk/src/javascript/PublicMethods.cpp (right):

https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
fpdfsdk/src/javascript/PublicMethods.cpp:658: int remaining = format.GetLength()
- i - 1;
should alignment with the previous line? I saw it didn't align with the previous
one when I checked the raw file.

https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
fpdfsdk/src/javascript/PublicMethods.cpp:660: if (remaining < 1 ||
format.GetAt(i+1) != c)
remaining == 1 rather than remaining < 1

for example:
int remaining = format.GetLength() - i - 1;
let's assume 'format.GetLength() = 1, i =0'
the remaining is 0 can meet the if condition, but format.GetAt(1) is incorrect
access.

https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
fpdfsdk/src/javascript/PublicMethods.cpp:705: else if (remaining < 2 ||
format.GetAt(i+2) != c)
remaining == 2

https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
fpdfsdk/src/javascript/PublicMethods.cpp:751: else if (remaining < 3 ||
format.GetAt(i+3) != c)
remaining == 3

https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
fpdfsdk/src/javascript/PublicMethods.cpp:787: else if (remaining < 4 ||
format.GetAt(i+4) != c)
remaining == 4

[Tom Sepez, 2014-07-18 18:00:05]

https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
File fpdfsdk/src/javascript/PublicMethods.cpp (right):

https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
fpdfsdk/src/javascript/PublicMethods.cpp:658: int remaining = format.GetLength()
- i - 1;
On 2014/07/18 17:30:59, jun_fang wrote:
> should alignment with the previous line? I saw it didn't align with the
previous
> one when I checked the raw file.
I "untabified" the entire function. That should make the aligmnent consistent
regardless of tab settings.

https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
fpdfsdk/src/javascript/PublicMethods.cpp:660: if (remaining < 1 ||
format.GetAt(i+1) != c)
On 2014/07/18 17:30:59, jun_fang wrote:
> remaining == 1 rather than remaining < 1
> 
> for example:
> int remaining = format.GetLength() - i - 1;
> let's assume 'format.GetLength() = 1, i =0'
> the remaining is 0 can meet the if condition, but format.GetAt(1) is incorrect
> access.
No, its an "or", not an "and", so when remaining is 0, 0 is < 1 and we
short-circuit the evaluation of GetAt(i+1) and go directly to the switch, which
doesn't depend upon the char at i+i.

[jun_fang, 2014-07-18 18:21:00]

On 2014/07/18 18:00:05, Tom Sepez wrote:
>
https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
> File fpdfsdk/src/javascript/PublicMethods.cpp (right):
> 
>
https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
> fpdfsdk/src/javascript/PublicMethods.cpp:658: int remaining =
format.GetLength()
> - i - 1;
> On 2014/07/18 17:30:59, jun_fang wrote:
> > should alignment with the previous line? I saw it didn't align with the
> previous
> > one when I checked the raw file.
> I "untabified" the entire function. That should make the aligmnent consistent
> regardless of tab settings.
> 
>
https://codereview.chromium.org/395303004/diff/1/fpdfsdk/src/javascript/Publi...
> fpdfsdk/src/javascript/PublicMethods.cpp:660: if (remaining < 1 ||
> format.GetAt(i+1) != c)
> On 2014/07/18 17:30:59, jun_fang wrote:
> > remaining == 1 rather than remaining < 1
> > 
> > for example:
> > int remaining = format.GetLength() - i - 1;
> > let's assume 'format.GetLength() = 1, i =0'
> > the remaining is 0 can meet the if condition, but format.GetAt(1) is
incorrect
> > access.
> No, its an "or", not an "and", so when remaining is 0, 0 is < 1 and we
> short-circuit the evaluation of GetAt(i+1) and go directly to the switch,
which
> doesn't depend upon the char at i+i.

Yes. You are right. Another concern is that when you use 'remaining < 4', it has
the only possibility 'remaining == 3'. The similar cases are 'remaining < 1',
'remaining < 2' and 'remaining <3'. If there is only one possibility, is it good
to use the clear way.

[Tom Sepez, 2014-07-18 18:25:01]

Yes. You are right. Another concern is that when you use 'remaining < 4', it has
> the only possibility 'remaining == 3'. The similar cases are 'remaining < 1',
> 'remaining < 2' and 'remaining <3'. If there is only one possibility, is it
good
> to use the clear way.
Ok, sure. I'll change it if you think its clearer.

[jun_fang, 2014-07-18 19:42:17]

On 2014/07/18 18:25:01, Tom Sepez wrote:
> Yes. You are right. Another concern is that when you use 'remaining < 4', it
has
> > the only possibility 'remaining == 3'. The similar cases are 'remaining <
1',
> > 'remaining < 2' and 'remaining <3'. If there is only one possibility, is it
> good
> > to use the clear way.
> Ok, sure. I'll change it if you think its clearer.

LGTM

[Tom Sepez, 2014-07-18 21:42:17]

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