Avoid signed integer overflow when calculating filesizes for VMS's ftp listings.

Avoid signed integer overflow when calculating filesizes for VMS's ftp listings.

BUG=677863
Committed: https://crrev.com/6d16eeca1aeace6b17e8d668f887fcc810a218ef
Cr-Commit-Position: refs/heads/master@{#441471}

[eroman, 2017-01-04 18:31:39]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 18:32:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-01-04 18:33:21]

Patchset #1 (id:1) has been deleted

[eroman, 2017-01-04 18:33:26]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 18:33:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-01-04 18:34:30]

eroman@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2017-01-04 18:46:41]

LGTM.  I also have a VMS CL for you today.  It's the New Year's VMS patch
exchange!

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser_vms_unittest.cc (right):

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:111: "README.TXT;1 
9223372036854775807/9223372036854775807  18-APR-2000 "
Shouldn't the first number be smaller here, if you want to test something new
and exciting?

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:111: "README.TXT;1 
9223372036854775807/9223372036854775807  18-APR-2000 "
I'd also suggest something like 9223372036854775806/9223372036854775806 (only
overflows after multiplication)

[eroman, 2017-01-04 18:55:49]

Thanks for the speedy review

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser_vms_unittest.cc (right):

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:111: "README.TXT;1 
9223372036854775807/9223372036854775807  18-APR-2000 "
On 2017/01/04 18:46:41, mmenke wrote:
> Shouldn't the first number be smaller here, if you want to test something new
> and exciting?

The first number is the blocks_used, the second is the blocks_allocated.

It is sufficient for blocks_used <= blocks_allocated for the test to exercises
the overflow condition

I confirmed that as written the test triggers the overflow case.

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:111: "README.TXT;1 
9223372036854775807/9223372036854775807  18-APR-2000 "
On 2017/01/04 18:46:41, mmenke wrote:
> I'd also suggest something like 9223372036854775806/9223372036854775806 (only
> overflows after multiplication)

My current test is INT64_MAX/INT64_MAX.

What additional edge does INT64_MAX-1 / INT64_MAX-1 test ?

[mmenke, 2017-01-04 19:00:26]

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser_vms_unittest.cc (right):

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:111: "README.TXT;1 
9223372036854775807/9223372036854775807  18-APR-2000 "
On 2017/01/04 18:55:49, eroman (slow) wrote:
> On 2017/01/04 18:46:41, mmenke wrote:
> > I'd also suggest something like 9223372036854775806/9223372036854775806
(only
> > overflows after multiplication)
> 
> My current test is INT64_MAX/INT64_MAX.
> 
> What additional edge does INT64_MAX-1 / INT64_MAX-1 test ?

Multiplying by 512 (block size) causes an overflow, rather than parsing as
string.

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:111: "README.TXT;1 
9223372036854775807/9223372036854775807  18-APR-2000 "
On 2017/01/04 18:55:49, eroman (slow) wrote:
> On 2017/01/04 18:46:41, mmenke wrote:
> > Shouldn't the first number be smaller here, if you want to test something
new
> > and exciting?
> 
> The first number is the blocks_used, the second is the blocks_allocated.
> 
> It is sufficient for blocks_used <= blocks_allocated for the test to exercises
> the overflow condition
> 
> I confirmed that as written the test triggers the overflow case.

Oops, I misread the code...What I really want is a case where blocks used does
not overflow, and blocks allocated does, since we parse both (I hadn't realized
that x and x/y follow different blocks used overflow paths).

So I suggest an additional test with #/9223372036854775807, where # is less than
9223372036854775807 / 512.

[mmenke, 2017-01-04 19:01:33]

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser_vms_unittest.cc (right):

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:111: "README.TXT;1 
9223372036854775807/9223372036854775807  18-APR-2000 "
On 2017/01/04 19:00:26, mmenke wrote:
> On 2017/01/04 18:55:49, eroman (slow) wrote:
> > On 2017/01/04 18:46:41, mmenke wrote:
> > > I'd also suggest something like 9223372036854775806/9223372036854775806
> (only
> > > overflows after multiplication)
> > 
> > My current test is INT64_MAX/INT64_MAX.
> > 
> > What additional edge does INT64_MAX-1 / INT64_MAX-1 test ?
> 
> Multiplying by 512 (block size) causes an overflow, rather than parsing as
> string.

Rather than parsing the string as an int64, rather

[eroman, 2017-01-04 19:04:31]

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser_vms_unittest.cc (right):

https://codereview.chromium.org/2611933003/diff/20001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:111: "README.TXT;1 
9223372036854775807/9223372036854775807  18-APR-2000 "
On 2017/01/04 19:01:33, mmenke wrote:
> On 2017/01/04 19:00:26, mmenke wrote:
> > On 2017/01/04 18:55:49, eroman (slow) wrote:
> > > On 2017/01/04 18:46:41, mmenke wrote:
> > > > I'd also suggest something like 9223372036854775806/9223372036854775806
> > (only
> > > > overflows after multiplication)
> > > 
> > > My current test is INT64_MAX/INT64_MAX.
> > > 
> > > What additional edge does INT64_MAX-1 / INT64_MAX-1 test ?
> > 
> > Multiplying by 512 (block size) causes an overflow, rather than parsing as
> > string.
> 
> Rather than parsing the string as an int64, rather

I have updated the test description to clarify, and added a new test.

- The existing test was verifying what happens when block_count * 512 overflows
- The new tests verifies when parsing block_count itself overflows

[eroman, 2017-01-04 19:05:25]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 19:05:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-01-04 19:07:24]

Oops, was thinking you were using MAX_INT64+1.  Thanks for correcting me!

Still LGTM.

https://codereview.chromium.org/2611933003/diff/40001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser_vms_unittest.cc (right):

https://codereview.chromium.org/2611933003/diff/40001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:117: "README.TXT;1 
19223372036854775807/19223372036854775807  18-APR-2000 "
Also a test where block site is not too large (Nor is block size * 512), but
block allocation size is?  (Admittedly, weird case).

[commit-bot: I haz the power, 2017-01-04 20:15:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 20:15:40]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-01-04 20:15:49]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 20:16:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-01-04 20:16:58]

https://codereview.chromium.org/2611933003/diff/40001/net/ftp/ftp_directory_l...
File net/ftp/ftp_directory_listing_parser_vms_unittest.cc (right):

https://codereview.chromium.org/2611933003/diff/40001/net/ftp/ftp_directory_l...
net/ftp/ftp_directory_listing_parser_vms_unittest.cc:117: "README.TXT;1 
19223372036854775807/19223372036854775807  18-APR-2000 "
On 2017/01/04 19:07:24, mmenke wrote:
> Also a test where block site is not too large (Nor is block size * 512), but
> block allocation size is?  (Admittedly, weird case).

Done.

[commit-bot: I haz the power, 2017-01-04 21:24:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 21:24:09]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-01-04 21:32:23]

The CQ bit was checked by eroman@chromium.org

[eroman, 2017-01-04 21:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2611933003/#ps60001
(title: "moar test")

[commit-bot: I haz the power, 2017-01-04 21:32:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 21:37:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483565543106380,
"parent_rev": "c799466aa01c6949e66ccdd64ca38a6c9e79a2bd", "commit_rev":
"988d2fd97cf23d3820c9ae1b7a32d57c44adbb3e"}

[commit-bot: I haz the power, 2017-01-04 21:38:47]

Description was changed from

==========
Avoid signed integer overflow when calculating filesizes for VMS's ftp listings.

BUG=677863
==========

to

==========
Avoid signed integer overflow when calculating filesizes for VMS's ftp listings.

BUG=677863

Review-Url: https://codereview.chromium.org/2611933003
==========

[commit-bot: I haz the power, 2017-01-04 21:38:48]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2017-01-04 21:43:00]

Description was changed from

==========
Avoid signed integer overflow when calculating filesizes for VMS's ftp listings.

BUG=677863

Review-Url: https://codereview.chromium.org/2611933003
==========

to

==========
Avoid signed integer overflow when calculating filesizes for VMS's ftp listings.

BUG=677863

Committed: https://crrev.com/6d16eeca1aeace6b17e8d668f887fcc810a218ef
Cr-Commit-Position: refs/heads/master@{#441471}
==========

[commit-bot: I haz the power, 2017-01-04 21:43:01]

Patchset 3 (id:??) landed as
https://crrev.com/6d16eeca1aeace6b17e8d668f887fcc810a218ef
Cr-Commit-Position: refs/heads/master@{#441471}