154201 CVE-2016-4623 2016-02-12 15:43:35 -0800 JSObject::putByIndexBeyondVectorLengthWithoutAttributes needs to go to the sparse map based on MAX_STORAGE_VECTOR_INDEX 2016-07-19 17:27:06 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build All All RESOLVED FIXED P2 Normal --- 1 fpizlo fpizlo bfulgham ggaren oldest_to_newest 1164400 0 fpizlo 2016-02-12 15:43:35 -0800 Patch forthcoming. 1164408 1 271241 fpizlo 2016-02-12 15:58:39 -0800 Created attachment 271241 the patch 1164410 2 fpizlo 2016-02-12 16:07:46 -0800 Landed in http://trac.webkit.org/changeset/196524 1164587 3 271241 ggaren 2016-02-13 15:20:48 -0800 Comment on attachment 271241 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=271241&action=review > Source/JavaScriptCore/runtime/StringPrototype.cpp:1208 > + throwOutOfMemoryError(exec); > + return JSValue::encode(jsUndefined()); A few thoughts here: (1) There are two other places in this function where we might exceed MAX_STORAGE_VECTOR_INDEX. I think we want consistent behavior in those places. (2) It is unlikely this limit will harm web compatibility. I suspect that total process memory usage at the point of MAX_STORAGE_VECTOR_INDEX is greater than 1GB. Chrome unconditionally kills any process over ~1GB. So does iOS. (3) Perhaps a more obvious behavior for web developers would be to prohibit strings larger than MAX_STORAGE_VECTOR_INDEX? ECMAScript specifies that a string can be as large as 134 million GBs, but that is obviously absurd. (4) It's surprising that the array that results from a split can cause problems when the input string to split did not cause problems. I think the length of the array will at worst be equal to the length of the string. (Of course, the array can take up 8X more memory, if it uses one 64bit pointer per 8bit character. Still, surprising that 8X makes the difference between OK and not OK.) 1164588 4 fpizlo 2016-02-13 15:39:51 -0800 (In reply to comment #3) > Comment on attachment 271241 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271241&action=review > > > Source/JavaScriptCore/runtime/StringPrototype.cpp:1208 > > + throwOutOfMemoryError(exec); > > + return JSValue::encode(jsUndefined()); > > A few thoughts here: > > (1) There are two other places in this function where we might exceed > MAX_STORAGE_VECTOR_INDEX. I think we want consistent behavior in those > places. I agree. We should do something for those, too, if we think it was a good idea to do it here. > > (2) It is unlikely this limit will harm web compatibility. I suspect that > total process memory usage at the point of MAX_STORAGE_VECTOR_INDEX is > greater than 1GB. Chrome unconditionally kills any process over ~1GB. So > does iOS. > > (3) Perhaps a more obvious behavior for web developers would be to prohibit > strings larger than MAX_STORAGE_VECTOR_INDEX? ECMAScript specifies that a > string can be as large as 134 million GBs, but that is obviously absurd. That's a good idea. > > (4) It's surprising that the array that results from a split can cause > problems when the input string to split did not cause problems. I think the > length of the array will at worst be equal to the length of the string. (Of > course, the array can take up 8X more memory, if it uses one 64bit pointer > per 8bit character. Still, surprising that 8X makes the difference between > OK and not OK.) I think that MAX_STORAGE_VECTOR_INDEX is too small right now, which exacerbates the problem. I wouldn't be surprised if split() gave an OOME for a very large string because I would assume that the split() has to allocate the array and the substrings. Some developers will realize that the engine should optimize the substrings by either doing the classic substring optimization, but even that will allocate some kind of object. Some developers will realize that the engine should use single-character-string optimizations, but even then we're still allocating the array. So even if the array takes the same amount of memory as the string, it's not surprising that you had enough memory for the string but not enough for the array. I don't think anyone would expect the GC to kill the string the moment that the array was allocated, even if the string was logically dead after the split. 271241 2016-02-12 15:58:39 -0800 2016-02-12 16:04:44 -0800 the patch blah.patch text/plain 3495 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTk2NTIyKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBA CisyMDE2LTAyLTEyICBGaWxpcCBQaXpsbyAgPGZwaXpsb0BhcHBsZS5jb20+CisKKyAgICAgICAg SlNPYmplY3Q6OnB1dEJ5SW5kZXhCZXlvbmRWZWN0b3JMZW5ndGhXaXRob3V0QXR0cmlidXRlcyBu ZWVkcyB0byBnbyB0byB0aGUgc3BhcnNlIG1hcCBiYXNlZCBvbiBNQVhfU1RPUkFHRV9WRUNUT1Jf SU5ERVgKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1 NDIwMQorICAgICAgICByZGFyOi8vcHJvYmxlbS8yNDI5MTM4NworCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEkgZGVjaWRlZCBhZ2FpbnN0IGFkZGluZyBh IHRlc3QgZm9yIHRoaXMsIGJlY2F1c2UgaXQgcnVucyBmb3IgYSB2ZXJ5IGxvbmcgdGltZS4KKwor ICAgICAgICAqIHJ1bnRpbWUvSlNPYmplY3QuY3BwOgorICAgICAgICAoSlNDOjpKU09iamVjdDo6 cHV0QnlJbmRleEJleW9uZFZlY3Rvckxlbmd0aFdpdGhvdXRBdHRyaWJ1dGVzKTogRml4IHRoZSBi dWcuCisgICAgICAgICogcnVudGltZS9TdHJpbmdQcm90b3R5cGUuY3BwOgorICAgICAgICAoSlND OjpzdHJpbmdQcm90b0Z1bmNTcGxpdCk6IEZpeCBhIHJlbGF0ZWQgYnVnOiBpZiB0aGlzIGNvZGUg Y3JlYXRlcyBhbiBhcnJheSB0aGF0IHdvdWxkIGhhdmUKKyAgICAgICAgICAgIGhpdCB0aGUgYWJv dmUgYnVnLCB0aGVuIGl0IHdvdWxkIHByb2JhYmx5IG1hbmlmZXN0IGFzIGEgc3BpbiBvciBhcyBz d2FwcGluZy4KKwogMjAxNi0wMi0xMiAgSm9uYXRoYW4gRGF2aXMgIDxqb25kQGFwcGxlLmNvbT4K IAogICAgICAgICBBZGQgV2ViQXNzZW1ibHkgdG8gdGhlIHN0YXR1cyBwYWdlCkluZGV4OiBTb3Vy Y2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU09iamVjdC5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNPYmplY3QuY3BwCShyZXZpc2lvbiAxOTY1MDAp CisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU09iamVjdC5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTE5NDMsNyArMTk0Myw3IEBAIHZvaWQgSlNPYmplY3Q6OnB1dEJ5SW5kZXhCZXlv bmRWZWN0b3JMZW4KICAgICAKICAgICBWTSYgdm0gPSBleGVjLT52bSgpOwogICAgIAotICAgIGlm IChpID49IE1BWF9BUlJBWV9JTkRFWCAtIDEKKyAgICBpZiAoaSA+IE1BWF9TVE9SQUdFX1ZFQ1RP Ul9JTkRFWAogICAgICAgICB8fCAoaSA+PSBNSU5fU1BBUlNFX0FSUkFZX0lOREVYCiAgICAgICAg ICAgICAmJiAhaXNEZW5zZUVub3VnaEZvclZlY3RvcihpLCBjb3VudEVsZW1lbnRzPGluZGV4aW5n U2hhcGU+KGJ1dHRlcmZseSkpKQogICAgICAgICB8fCBpbmRleElzU3VmZmljaWVudGx5QmV5b25k TGVuZ3RoRm9yU3BhcnNlTWFwKGksIGJ1dHRlcmZseS0+dmVjdG9yTGVuZ3RoKCkpKSB7CkluZGV4 OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9TdHJpbmdQcm90b3R5cGUuY3BwCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1N0cmluZ1Byb3RvdHlwZS5j cHAJKHJldmlzaW9uIDE5NjUwMCkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL1N0 cmluZ1Byb3RvdHlwZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTExOTAsOCArMTE5MCwyMyBAQCBF bmNvZGVkSlNWYWx1ZSBKU0NfSE9TVF9DQUxMIHN0cmluZ1Byb3RvCiAKICAgICAgICAgICAgIC8v IDMuIEluY3JlbWVudCBsZW5ndGhBIGJ5IDEuCiAgICAgICAgICAgICAvLyA0LiBJZiBsZW5ndGhB ID09IGxpbSwgcmV0dXJuIEEuCi0gICAgICAgICAgICBpZiAoKytyZXN1bHRMZW5ndGggPT0gbGlt aXQpCisgICAgICAgICAgICArK3Jlc3VsdExlbmd0aDsKKyAgICAgICAgICAgIGlmIChyZXN1bHRM ZW5ndGggPT0gbGltaXQpCiAgICAgICAgICAgICAgICAgcmV0dXJuIEpTVmFsdWU6OmVuY29kZShy ZXN1bHQpOworICAgICAgICAgICAgaWYgKHJlc3VsdExlbmd0aCA+PSBNQVhfU1RPUkFHRV9WRUNU T1JfSU5ERVgpIHsKKyAgICAgICAgICAgICAgICAvLyBMZXQncyBjb25zaWRlciB3aGF0J3MgYmVz dCBmb3IgdXNlcnMgaGVyZS4gV2UncmUgYWJvdXQgdG8gaW5jcmVhc2UgdGhlIGxlbmd0aCBvZgor ICAgICAgICAgICAgICAgIC8vIHRoZSBzcGxpdCBhcnJheSBiZXlvbmQgdGhlIG1heGltdW0gbGVu Z3RoIHRoYXQgd2UgY2FuIHN1cHBvcnQgZWZmaWNpZW50bHkuIFRoaXMKKyAgICAgICAgICAgICAg ICAvLyB3aWxsIGNhdXNlIHVzIHRvIHVzZSBhIEhhc2hNYXAgZm9yIHRoZSBuZXcgZW50cmllcyBh ZnRlciB0aGlzIHBvaW50LiBUaGF0J3MgZ29pbmcKKyAgICAgICAgICAgICAgICAvLyB0byByZXN1 bHQgaW4gYSB2ZXJ5IGxvbmcgcnVubmluZyB0aW1lIG9mIHRoaXMgZnVuY3Rpb24gYW5kIHZlcnkg bGFyZ2UgbWVtb3J5CisgICAgICAgICAgICAgICAgLy8gdXNhZ2UuIEluIG15IGV4cGVyaW1lbnRz LCBKU0Mgd2lsbCBzaXQgc3Bpbm5pbmcgZm9yIG1pbnV0ZXMgYWZ0ZXIgZ2V0dGluZyBoZXJlIGFu ZAorICAgICAgICAgICAgICAgIC8vIGl0IHdhcyB1c2luZyA+NEdCIG9mIG1lbW9yeSBhbmQgZXZl bnR1YWxseSBncmV3IHRvIDhHQi4gSXQga2VwdCBydW5uaW5nIHdpdGhvdXQKKyAgICAgICAgICAg ICAgICAvLyBmaW5pc2hpbmcgdW50aWwgSSBraWxsZWQgaXQuIFRoYXQncyBwcm9iYWJseSBub3Qg d2hhdCB0aGUgdXNlciB3YW50ZWQuIFRoZSB1c2VyLAorICAgICAgICAgICAgICAgIC8vIG9yIHRo ZSBwcm9ncmFtIHRoYXQgdGhlIHVzZXIgaXMgcnVubmluZywgcHJvYmFibHkgbWFkZSBhIG1pc3Rh a2UgYnkgY2FsbGluZyB0aGlzCisgICAgICAgICAgICAgICAgLy8gbWV0aG9kIGluIHN1Y2ggYSB3 YXkgdGhhdCBpdCByZXN1bHRlZCBpbiBzdWNoIGFuIG9ibm94aW91cyBhcnJheS4gVGhlcmVmb3Jl LCB0bworICAgICAgICAgICAgICAgIC8vIHByb3RlY3Qgb3Vyc2VsdmVzLCB3ZSBiYWlsIGF0IHRo aXMgcG9pbnQuCisgICAgICAgICAgICAgICAgdGhyb3dPdXRPZk1lbW9yeUVycm9yKGV4ZWMpOwor ICAgICAgICAgICAgICAgIHJldHVybiBKU1ZhbHVlOjplbmNvZGUoanNVbmRlZmluZWQoKSk7Cisg ICAgICAgICAgICB9CiAKICAgICAgICAgICAgIC8vIDUuIExldCBwID0gZS4KICAgICAgICAgICAg IC8vIDguIExldCBxID0gcC4K