83712 2012-04-11 13:11:55 -0700 Do not dereference a newly allocated JSArray if it is null 2012-04-11 14:05:17 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED INVALID P2 Normal --- 1 benjamin benjamin ggaren oliver oldest_to_newest 600527 0 benjamin 2012-04-11 13:11:55 -0700 In JSArray::tryCreateUninitialized(), we dereference array: "array->tryFinishCreationUninitialized", that looks bad. 600535 1 136735 benjamin 2012-04-11 13:18:44 -0700 Created attachment 136735 Patch 600545 2 136735 oliver 2012-04-11 13:30:40 -0700 Comment on attachment 136735 Patch The initial allocation of the JSArray must succeed -- if it didn't we would crash in the constructor. But also the GC guarantees allocation will work. tryFinishCreationUninitialized on the jsarray is the bit that may fail, and if it fails that results in a bogus JSArray. That's where the try... comes from. 600566 3 benjamin 2012-04-11 13:44:15 -0700 (In reply to comment #2) > (From update of attachment 136735 [details]) > The initial allocation of the JSArray must succeed -- if it didn't we would crash in the constructor. But also the GC guarantees allocation will work. tryFinishCreationUninitialized on the jsarray is the bit that may fail, and if it fails that results in a bogus JSArray. That's where the try... comes from. That is a very good thing to know. So would you agree, all the tests like the following are wrong after JSArray::tryCreateUninitialized()?: if (!array) doSomething() If so, I'll remove that in an other patch. 600568 4 oliver 2012-04-11 13:48:47 -0700 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 136735 [details] [details]) > > The initial allocation of the JSArray must succeed -- if it didn't we would crash in the constructor. But also the GC guarantees allocation will work. tryFinishCreationUninitialized on the jsarray is the bit that may fail, and if it fails that results in a bogus JSArray. That's where the try... comes from. > > That is a very good thing to know. > > So would you agree, all the tests like the following are wrong after JSArray::tryCreateUninitialized()?: > if (!array) > doSomething() > > If so, I'll remove that in an other patch. I don't understand what you're asking. If you mean: JSArray* array = JSArray::tryCreateUninitialised(..) if (!array) doSomething(); then no, the null check is necessary. The bit of initialisation that makes it a "try" allocation is: tryFinishCreationUninitialized(..) That allocates the array's backing store. And that allocation may fail. If that allocation fails then the total allocation from JSArray::tryCreateUninitialised is invalid. 600580 5 benjamin 2012-04-11 14:05:17 -0700 > I don't understand what you're asking. > > If you mean: > > JSArray* array = JSArray::tryCreateUninitialised(..) > if (!array) > doSomething(); > > then no, the null check is necessary. Thanks for looking into this. That is exactly what I was wondering about. 136735 2012-04-11 13:18:44 -0700 2012-04-11 13:30:40 -0700 Patch bug-83712-20120411131843.patch text/plain 2188 benjamin U3VidmVyc2lvbiBSZXZpc2lvbjogMTEzODg2CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA5 MjQ4YjRjMTFlNDZkMGQ5ODI1MjRmY2ViODk4NmM1ODgwMGQ5ODJjLi41NzQ3NGYzNzViMDhhYzE4 YzIzMGYzYjUyZGQ0YzUzM2VmNjg4NmVhIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs NSArMSwyNCBAQAogMjAxMi0wNC0xMSAgQmVuamFtaW4gUG91bGFpbiAgPGJwb3VsYWluQGFwcGxl LmNvbT4KIAorICAgICAgICBEbyBub3QgZGVyZWZlcmVuY2UgYSBuZXdseSBhbGxvY2F0ZWQgSlNB cnJheSBpZiBpdCBpcyBudWxsCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD04MzcxMgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIFRoZSBjYWxsZXJzIG9mIEpTQXJyYXk6OnRyeUNyZWF0ZVVuaW5pdGlhbGl6ZWQo KSBoYW5kbGUgdGhlIGNhc2VzIHdoZXJlIHRoZSBhbGxvY2F0aW9uIGZhaWxzCisgICAgICAgIGFu ZCB0aGUgcmVzdWx0IGlzIG51bGwgKGJ5IHRocm93aW5nIHRocm93T3V0T2ZNZW1vcnlFcnJvcigp IG9yIGNyYXNoaW5nKS4KKworICAgICAgICBUaGlzIGNvZGUgd2FzIGJvZ3VzIGJlY2F1c2UgSlNB cnJheTo6dHJ5Q3JlYXRlVW5pbml0aWFsaXplZCgpIHdvdWxkIGNyYXNoIHdoZW4gZGVyZWZlcmVu Y2luZworICAgICAgICBhIG51bGwgcG9pbnRlciBieSBjYWxsaW5nIGFycmF5LT50cnlGaW5pc2hD cmVhdGlvblVuaW5pdGlhbGl6ZWQoKS4KKworICAgICAgICBUaGlzIHBhdGNoIGFkZHMgYSBudWxs IGNoZWNrIHRoZSB0aGUgY2FsbGVycyBvZiBKU0FycmF5Ojp0cnlDcmVhdGVVbmluaXRpYWxpemVk KCkgY2FuIGhhbmRsZQorICAgICAgICB0aGUgYWxsb2NhdGlvbiBmYWlsdXJlIGNvcnJlY3RseS4K KworICAgICAgICAqIHJ1bnRpbWUvSlNBcnJheS5oOgorICAgICAgICAoSlNDOjpKU0FycmF5Ojp0 cnlDcmVhdGVVbmluaXRpYWxpemVkKToKKworMjAxMi0wNC0xMSAgQmVuamFtaW4gUG91bGFpbiAg PGJwb3VsYWluQGFwcGxlLmNvbT4KKwogICAgICAgICBPcHRpbWl6ZSBTdHJpbmcuc3BsaXQoKSBm b3IgMSBjaGFyYWN0ZXIgc2VwYXJhdG9yCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD04MzU0NgogCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvcnVudGltZS9KU0FycmF5LmggYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0Fy cmF5LmgKaW5kZXggOThlZTMyZTMxMWNjZjc2NGUwMDBhZTk1ZjYxMDRjZDE5ZjIzNTc2Mi4uYjhj MDdmZTUxZWUxYTczMGQ0NTEyZjI1OGUyZjRiOWEwODY1MTVlMCAxMDA2NDQKLS0tIGEvU291cmNl L0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNBcnJheS5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9ydW50aW1lL0pTQXJyYXkuaApAQCAtMzMzLDggKzMzMyw5IEBAIG5hbWVzcGFjZSBKU0Mg ewogCiAgICAgaW5saW5lIEpTQXJyYXkqIEpTQXJyYXk6OnRyeUNyZWF0ZVVuaW5pdGlhbGl6ZWQo SlNHbG9iYWxEYXRhJiBnbG9iYWxEYXRhLCBTdHJ1Y3R1cmUqIHN0cnVjdHVyZSwgdW5zaWduZWQg aW5pdGlhbExlbmd0aCkKICAgICB7Ci0gICAgICAgIEpTQXJyYXkqIGFycmF5ID0gbmV3IChOb3RO dWxsLCBhbGxvY2F0ZUNlbGw8SlNBcnJheT4oZ2xvYmFsRGF0YS5oZWFwKSkgSlNBcnJheShnbG9i YWxEYXRhLCBzdHJ1Y3R1cmUpOwotICAgICAgICByZXR1cm4gYXJyYXktPnRyeUZpbmlzaENyZWF0 aW9uVW5pbml0aWFsaXplZChnbG9iYWxEYXRhLCBpbml0aWFsTGVuZ3RoKTsKKyAgICAgICAgaWYg KEpTQXJyYXkqIGFycmF5ID0gbmV3IChOb3ROdWxsLCBhbGxvY2F0ZUNlbGw8SlNBcnJheT4oZ2xv YmFsRGF0YS5oZWFwKSkgSlNBcnJheShnbG9iYWxEYXRhLCBzdHJ1Y3R1cmUpKQorICAgICAgICAg ICAgcmV0dXJuIGFycmF5LT50cnlGaW5pc2hDcmVhdGlvblVuaW5pdGlhbGl6ZWQoZ2xvYmFsRGF0 YSwgaW5pdGlhbExlbmd0aCk7CisgICAgICAgIHJldHVybiAwOwogICAgIH0KIAogICAgIEpTQXJy YXkqIGFzQXJyYXkoSlNWYWx1ZSk7Cg==