Allow offsets in blobs larger than 2GB on 32 bit Chromium builds.

Allow offsets in blobs larger than 2GB on 32 bit Chromium builds.

The recent patch crrev.com/895933007 introduced a regression by using size_t
for offsets, which on 32bit builds is sizeof(size_t) = 4 which was causing
broken offset values due to assigning a uint64_t variable to such variables.

This CL fixes that by simply converting the types to uint64_t.

TEST=*FileSystemProvider*BigFile* on a 32 bit build Chromium build with
    chromeos=1.
BUG=375297, 458122
Committed: https://crrev.com/f6629637feb3bbf186de75da8fe4028a5244389f
Cr-Commit-Position: refs/heads/master@{#318808}

[mtomasz, 2015-02-27 02:17:29]

mtomasz@chromium.org changed reviewers:
+ jianli@chromium.org

[mtomasz, 2015-02-27 02:17:47]

@jianli: PTAL. Thanks.

[mtomasz, 2015-03-02 00:08:36]

On 2015/02/27 02:17:47, mtomasz wrote:
> @jianli: PTAL. Thanks.

@jianli: Ping. This is a beta blocker.

[jianli, 2015-03-02 21:14:35]

It seems that r315702 introduced a lot of size_t usages and we do not fix all of
them in this patch. Will you work with dmurph to fix all of them or simply
revert the original patch? Thanks.

https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
storage/browser/blob/blob_storage_context.h:139: size_t memory_usage_;
Should this also be fixed?

[mtomasz, 2015-03-02 22:57:09]

https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
File storage/browser/blob/blob_storage_context.h (right):

https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
storage/browser/blob/blob_storage_context.h:139: size_t memory_usage_;
On 2015/03/02 21:14:35, jianli wrote:
> Should this also be fixed?

This is not related. Memory usage fits into size_t.

[mtomasz, 2015-03-02 22:57:54]

On 2015/03/02 21:14:35, jianli wrote:
> It seems that r315702 introduced a lot of size_t usages and we do not fix all
of
> them in this patch. Will you work with dmurph to fix all of them or simply
> revert the original patch? Thanks.
> 
>
https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
> File storage/browser/blob/blob_storage_context.h (right):
> 
>
https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
> storage/browser/blob/blob_storage_context.h:139: size_t memory_usage_;
> Should this also be fixed?

No need to revert. This CL fixes all of the issues.

[michaeln, 2015-03-02 23:04:37]

this one lgtm 2

[dmurph, 2015-03-02 23:05:22]

dmurph@chromium.org changed reviewers:
+ dmurph@chromium.org

[dmurph, 2015-03-02 23:05:23]

LGTM

This has the same fixes as the patch I just made.  To address jianli, the other
'size_t' introductions are exclusively for in-memory indexing and in-memory
size, these guys slipped by though.

I independently tested this change and it should fix the failing test.

[mtomasz, 2015-03-02 23:27:25]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2015-03-02 23:29:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/959113002/60001

[commit-bot: I haz the power, 2015-03-03 00:20:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-03 00:20:59]

Patchset 4 (id:??) landed as
https://crrev.com/f6629637feb3bbf186de75da8fe4028a5244389f
Cr-Commit-Position: refs/heads/master@{#318808}

[jianli, 2015-03-03 00:57:15]

On 2015/03/02 22:57:54, mtomasz wrote:
> On 2015/03/02 21:14:35, jianli wrote:
> > It seems that r315702 introduced a lot of size_t usages and we do not fix
all
> of
> > them in this patch. Will you work with dmurph to fix all of them or simply
> > revert the original patch? Thanks.
> > 
> >
>
https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
> > File storage/browser/blob/blob_storage_context.h (right):
> > 
> >
>
https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
> > storage/browser/blob/blob_storage_context.h:139: size_t memory_usage_;
> > Should this also be fixed?
> 
> No need to revert. This CL fixes all of the issues.

I just have a concern that we might not find out all the issues with these mixed
uses of size_t and uint64. Maybe I have not checked all the latest changes and
fixes. But how do we make sure we will not overflow size_t? For example, line 87
at InternalBlobData::GetUnsharedMemoryUsage.

[dmurph, 2015-03-03 01:05:33]

On 2015/03/03 00:57:15, jianli wrote:
> On 2015/03/02 22:57:54, mtomasz wrote:
> > On 2015/03/02 21:14:35, jianli wrote:
> > > It seems that r315702 introduced a lot of size_t usages and we do not fix
> all
> > of
> > > them in this patch. Will you work with dmurph to fix all of them or simply
> > > revert the original patch? Thanks.
> > > 
> > >
> >
>
https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
> > > File storage/browser/blob/blob_storage_context.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/959113002/diff/60001/storage/browser/blob/blo...
> > > storage/browser/blob/blob_storage_context.h:139: size_t memory_usage_;
> > > Should this also be fixed?
> > 
> > No need to revert. This CL fixes all of the issues.
> 
> I just have a concern that we might not find out all the issues with these
mixed
> uses of size_t and uint64. Maybe I have not checked all the latest changes and
> fixes. But how do we make sure we will not overflow size_t? For example, line
87
> at InternalBlobData::GetUnsharedMemoryUsage.

All of the other usages of size_t are specifically referring to in-memory
storage, where it is impossible to go above size_t.  That location was the only
place where I forgot to change back to uint64 for offset and length
calculations.

[jianli, 2015-03-03 01:12:39]

Is it possible to have overflow for the addition of 2 size_t in this case?

On Mon, Mar 2, 2015 at 5:05 PM, <dmurph@chromium.org> wrote:

> On 2015/03/03 00:57:15, jianli wrote:
>
>> On 2015/03/02 22:57:54, mtomasz wrote:
>> > On 2015/03/02 21:14:35, jianli wrote:
>> > > It seems that r315702 introduced a lot of size_t usages and we do not
>> fix
>> all
>> > of
>> > > them in this patch. Will you work with dmurph to fix all of them or
>> simply
>> > > revert the original patch? Thanks.
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/959113002/diff/60001/
> storage/browser/blob/blob_storage_context.h
>
>> > > File storage/browser/blob/blob_storage_context.h (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/959113002/diff/60001/
> storage/browser/blob/blob_storage_context.h#newcode139
>
>> > > storage/browser/blob/blob_storage_context.h:139: size_t
>> memory_usage_;
>> > > Should this also be fixed?
>> >
>> > No need to revert. This CL fixes all of the issues.
>>
>
>  I just have a concern that we might not find out all the issues with these
>>
> mixed
>
>> uses of size_t and uint64. Maybe I have not checked all the latest
>> changes and
>> fixes. But how do we make sure we will not overflow size_t? For example,
>> line
>>
> 87
>
>> at InternalBlobData::GetUnsharedMemoryUsage.
>>
>
> All of the other usages of size_t are specifically referring to in-memory
> storage, where it is impossible to go above size_t.  That location was the
> only
> place where I forgot to change back to uint64 for offset and length
> calculations.
>
> https://codereview.chromium.org/959113002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mtomasz, 2015-03-03 02:08:48]

Sounds like a valid concern. Let's track this issue separately.
I created crbug.com/463345