<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi,<div class=""><br class=""></div><div class="">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:<br class="">
<pre style="margin-top: 0px; margin-bottom: 0px;" class=""><!--StartFragment-->(newCapacity<span style=" color:#c0c0c0;" class=""> </span>><span style=" color:#c0c0c0;" class=""> </span><span style=" color:#800080;" class="">std</span>::<span style=" color:#800080;" class="">numeric_limits</span><<span style=" color:#808000;" class="">unsigned</span>>::max()<span style=" color:#c0c0c0;" class=""> </span>/<span style=" color:#c0c0c0;" class=""> </span><span style=" color:#808000;" class="">sizeof</span>(<span style=" color:#800080;" class="">T</span>))<!--EndFragment--></pre><div class=""><br class=""></div><div class="">Kr, <br class=""><div class="">
<div style="color: rgb(0, 0, 0); letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">--</div><div class="">Chris Dumez - Apple Inc.</div><div class="">Cupertino, CA</div><div class=""><br class=""></div></div><br class="Apple-interchange-newline"><br class="Apple-interchange-newline">
</div>
<br class=""><div><blockquote type="cite" class=""><div class="">On Nov 19, 2014, at 1:57 PM, Geoffrey Garen <<a href="mailto:ggaren@apple.com" class="">ggaren@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">I don’t haver a specific opinion on size_t vs unsigned, but I have a related point:<div class=""><br class=""></div><div class=""><i class="">If</i> Vector uses unsigned, <i class="">then</i> it must RELEASE_ASSERT that it does not overflow unsigned when growing. </div><div class=""><br class=""></div><div class="">Even if most normal content allocates < 4GB, exploit content surely will try to allocate > 4GB.</div><div class=""><br class=""></div><div class="">Chris, maybe you can make this change in trunk, since trunk uses unsigned?</div><div class=""><br class=""></div><div class="">Geoff</div><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Nov 19, 2014, at 1:50 PM, Alexey Proskuryakov <<a href="mailto:ap@webkit.org" class="">ap@webkit.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=windows-1251" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">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?</div><div class=""><br class=""></div><div class="">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.</div><br class=""><div class=""><div class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">- Alexey</div><div class=""><br class=""></div></div><br class="Apple-interchange-newline"></div><div class="">19 нояб. 2014 г., в 13:19, Filip Pizlo <<a href="mailto:fpizlo@apple.com" class="">fpizlo@apple.com</a>> написал(а):</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">ArrayBuffers are very special because they are part of ECMAScript.<div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">-Filip</div><div class=""><br class=""></div><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Nov 19, 2014, at 12:54 PM, Alexey Proskuryakov <<a href="mailto:ap@webkit.org" class="">ap@webkit.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=windows-1251" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">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.</div><br class=""><div class=""><div class="">
<div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">- Alexey</div><div class=""><br class=""></div></div><br class="Apple-interchange-newline">
</div>
<div class="">19 нояб. 2014 г., в 12:32, Filip Pizlo <<a href="mailto:fpizlo@apple.com" class="">fpizlo@apple.com</a>> написал(а):</div><br class="Apple-interchange-newline"><blockquote type="cite" class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">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.</div><div class=""><br class=""></div>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.<div class=""><br class=""></div><div class="">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.<br class=""><div class=""><br class=""></div><div class="">-Filip<br class=""><div class=""><div class=""><br class=""></div><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Nov 19, 2014, at 12:20 PM, Chris Dumez <<a href="mailto:cdumez@apple.com" class="">cdumez@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi all,<br class=""><br class="">I recently started updating the WTF::Vector API to use unsigned types instead of size_t [1][2], because:<br class="">- 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]<br class="">- 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).<br class="">- I heard from Joseph that this type mismatch is forcing us (and other projects using WTF) to disable some build time warnings<br class="">- The few people I talked to before making that change said we should do it<br class=""><br class="">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.”.<br class="">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.<br class=""><br class="">Kr,<br class=""><div apple-content-edited="true" class="">
<div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class="">--</div><div class="">Chris Dumez - Apple Inc.</div><div class="">Cupertino, CA</div><div class=""><br class=""></div></div><br class="Apple-interchange-newline">[1] <a href="http://trac.webkit.org/changeset/176275" class="">http://trac.webkit.org/changeset/176275</a><br class="">[2] <a href="http://trac.webkit.org/changeset/176293" class="">http://trac.webkit.org/changeset/176293</a><br class="">[3] <a href="http://trac.webkit.org/changeset/148891" class="">http://trac.webkit.org/changeset/148891</a>
</div>
<br class=""></div>_______________________________________________<br class="">webkit-dev mailing list<br class=""><a href="mailto:webkit-dev@lists.webkit.org" class="">webkit-dev@lists.webkit.org</a><br class=""><a href="https://lists.webkit.org/mailman/listinfo/webkit-dev" class="">https://lists.webkit.org/mailman/listinfo/webkit-dev</a><br class=""></div></blockquote></div><br class=""></div></div></div></div></div>_______________________________________________<br class="">webkit-dev mailing list<br class=""><a href="mailto:webkit-dev@lists.webkit.org" class="">webkit-dev@lists.webkit.org</a><br class=""><a href="https://lists.webkit.org/mailman/listinfo/webkit-dev" class="">https://lists.webkit.org/mailman/listinfo/webkit-dev</a><br class=""></blockquote></div><br class=""><div apple-content-edited="true" class="">
<div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""></div></div></div></div></div></blockquote></div><br class=""></div></div></blockquote></div><br class=""><div apple-content-edited="true" class="">
<div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div class=""><br class=""></div></div></div></div>_______________________________________________<br class="">webkit-dev mailing list<br class=""><a href="mailto:webkit-dev@lists.webkit.org" class="">webkit-dev@lists.webkit.org</a><br class=""><a href="https://lists.webkit.org/mailman/listinfo/webkit-dev" class="">https://lists.webkit.org/mailman/listinfo/webkit-dev</a><br class=""></div></blockquote></div><br class=""></div></div>_______________________________________________<br class="">webkit-dev mailing list<br class=""><a href="mailto:webkit-dev@lists.webkit.org" class="">webkit-dev@lists.webkit.org</a><br class="">https://lists.webkit.org/mailman/listinfo/webkit-dev<br class=""></div></blockquote></div><br class=""></div></div></body></html>