Hi, I believe the Vector class is already checking for overflows. I looked quickly and it seems that when trying to allocate / reallocate the Vector buffer, we will call CRASH() if: (newCapacity > std::numeric_limits<unsigned>::max() / sizeof(T))
Kr, -- Chris Dumez - Apple Inc. Cupertino, CA > On Nov 19, 2014, at 1:57 PM, Geoffrey Garen <gga...@apple.com> wrote: > > I don’t haver a specific opinion on size_t vs unsigned, but I have a related > point: > > If Vector uses unsigned, then it must RELEASE_ASSERT that it does not > overflow unsigned when growing. > > Even if most normal content allocates < 4GB, exploit content surely will try > to allocate > 4GB. > > Chris, maybe you can make this change in trunk, since trunk uses unsigned? > > Geoff > >> On Nov 19, 2014, at 1:50 PM, Alexey Proskuryakov <a...@webkit.org >> <mailto:a...@webkit.org>> wrote: >> >> >> That makes good sense for internal implementation, do you think that class >> API should also use a custom type, or should it use size_t? >> >> With Vector though, I don't know how we would differentiate code paths that >> need large allocations from ones that don't. Nearly anything that is exposed >> as a JS API or deals with external world can hit sizes over 4Gb. That's not >> out of reach in most scenarios, not even for resources loaded from network. >> >> - Alexey >> >> >> 19 нояб. 2014 г., в 13:19, Filip Pizlo <fpi...@apple.com >> <mailto:fpi...@apple.com>> написал(а): >> >>> ArrayBuffers are very special because they are part of ECMAScript. >>> >>> We use unsigned for the length, because once upon a time, that would have >>> been the right type; these days the right type would be a 53-bit integer. >>> So, size_t would be wrong. I believe that ArrayBuffers should be changed >>> to use a very special size type; probably it wouldn't even be a primitive >>> type but a class that carefully checked that you never overflowed 53 bits. >>> >>> -Filip >>> >>> >>>> On Nov 19, 2014, at 12:54 PM, Alexey Proskuryakov <a...@webkit.org >>>> <mailto:a...@webkit.org>> wrote: >>>> >>>> >>>> This is not exactly about Vector, but if one uses >>>> FileReader.prototype.readAsArrayBuffer() on a large file, I think that it >>>> overflows ArrayBuffer. WebKit actually crashes when uploading >>>> multi-gigabyte files to YouTube, Google Drive and other similar services, >>>> although I haven't checked whether it's because of ArrayBuffers, or >>>> because of a use of int/unsigned in another code path. >>>> >>>> I think that we should use size_t everywhere except for perhaps a few key >>>> places where memory impact is critical (and of course we need explicit >>>> checks when casting down to an unsigned). Or perhaps the rule can be even >>>> simpler, and unsigned may never be used for indices and sizes, period. >>>> >>>> - Alexey >>>> >>>> >>>> 19 нояб. 2014 г., в 12:32, Filip Pizlo <fpi...@apple.com >>>> <mailto:fpi...@apple.com>> написал(а): >>>> >>>>> Whatever we do, the clients of Vector should be consistent about what >>>>> type they use. I'm actually fine with Vector internally using unsigned >>>>> even if the API uses size_t, but right now we have lots of code that uses >>>>> a mix of size_t and unsigned when indexing into Vectors. That's >>>>> confusing. >>>>> >>>>> If I picked one type to use for all Vector indices, it would be unsigned >>>>> rather than size_t. Vector being limited to unsigned doesn't imply 4GB >>>>> unless you do Vector<char>. Usually we have Vectors of pointer-width >>>>> things, which means 32GB on 64-bit systems (UINT_MAX * sizeof(void*)). >>>>> Even in a world where we had more than 32GB of memory to use within a >>>>> single web process, I would hope that we wouldn't use it all on a single >>>>> Vector and that if we did, we would treat that one specially for a bunch >>>>> of other sensible reasons (among them being that allocating a contiguous >>>>> slab of virtual memory of that size is rather taxing). So, size_t would >>>>> buy us almost nothing since if we had a vector grow large enough to need >>>>> it, we would be having a bad time already. >>>>> >>>>> I wonder: are there cases that anyone remembers where we have tried to >>>>> use Vector to store more than UINT_MAX things? Another interesting >>>>> question is: What's the largest number of things that we store into any >>>>> Vector? We could use such a metric to project how big Vectors might get >>>>> in the future. >>>>> >>>>> -Filip >>>>> >>>>> >>>>>> On Nov 19, 2014, at 12:20 PM, Chris Dumez <cdu...@apple.com >>>>>> <mailto:cdu...@apple.com>> wrote: >>>>>> >>>>>> Hi all, >>>>>> >>>>>> I recently started updating the WTF::Vector API to use unsigned types >>>>>> instead of size_t [1][2], because: >>>>>> - WTF::Vector is already using unsigned type for size/capacity >>>>>> internally to save memory on 64-bit, causing a mismatch between the API >>>>>> and the internal representation [3] >>>>>> - Some reviewers have asked me to use unsigned for loop counters >>>>>> iterating over vectors (which looks unsafe as the Vector API, e.g. >>>>>> size(), returns a size_t). >>>>>> - I heard from Joseph that this type mismatch is forcing us (and other >>>>>> projects using WTF) to disable some build time warnings >>>>>> - The few people I talked to before making that change said we should do >>>>>> it >>>>>> >>>>>> However, Alexey recently raised concerns about this change. it doesn't >>>>>> "strike him as the right direction. 4Gb is not much, and we should have >>>>>> more of WebKit work with the right data types, not less.”. >>>>>> I did not initially realize that this change was controversial, but now >>>>>> that it seems it is, I thought I would raise the question on webkit-dev >>>>>> to see what people think about this. >>>>>> >>>>>> Kr, >>>>>> -- >>>>>> Chris Dumez - Apple Inc. >>>>>> Cupertino, CA >>>>>> >>>>>> >>>>>> [1] http://trac.webkit.org/changeset/176275 >>>>>> <http://trac.webkit.org/changeset/176275> >>>>>> [2] http://trac.webkit.org/changeset/176293 >>>>>> <http://trac.webkit.org/changeset/176293> >>>>>> [3] http://trac.webkit.org/changeset/148891 >>>>>> <http://trac.webkit.org/changeset/148891> >>>>>> _______________________________________________ >>>>>> webkit-dev mailing list >>>>>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >>>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev >>>>>> <https://lists.webkit.org/mailman/listinfo/webkit-dev> >>>>> >>>>> _______________________________________________ >>>>> webkit-dev mailing list >>>>> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >>>>> https://lists.webkit.org/mailman/listinfo/webkit-dev >>>>> <https://lists.webkit.org/mailman/listinfo/webkit-dev> >>>> >>>> >>> >> >> >> _______________________________________________ >> webkit-dev mailing list >> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> >> https://lists.webkit.org/mailman/listinfo/webkit-dev > > _______________________________________________ > webkit-dev mailing list > webkit-dev@lists.webkit.org > https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
