65128 2011-07-25 13:17:13 -0700 DFG JIT bytecode parser misuses pointers into objects allocated as part of a WTF::Vector 2011-07-25 17:25:48 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 0 fpizlo webkit-unassigned barraclough fpizlo thorton webkit.review.bot oldest_to_newest 441818 0 fpizlo 2011-07-25 13:17:13 -0700 The DFG JIT bytecode parser gets a reference to a DFGNode in the DFGGraph, which is a subtype of WTF::Vector<DFGNode, 64>. It then adds to the graph, and after adding to it, uses the reference several times, and while potentially adding to the graph again. Adding to the graph means adding to the Vector, which in turn means that the Vector may resize its backing store. When the backing store is resized, the old reference to the DFGNode in the Vector may become a dangling reference and subsequent uses may either corrupt memory, crash, or otherwise do bad things. The DFG bytecode parser should not keep references to the innards of Vector alive after the Vector has been resized. 441821 1 thorton 2011-07-25 13:19:42 -0700 <rdar://problem/9834708> 441822 2 101900 fpizlo 2011-07-25 13:22:53 -0700 Created attachment 101900 the patch Still waiting for the relevant tests to finish running; when they do I'll change the review/commit-queue states to ?. 441827 3 101900 fpizlo 2011-07-25 13:30:48 -0700 Comment on attachment 101900 the patch tests pass, ready for review 441877 4 101900 darin 2011-07-25 14:46:48 -0700 Comment on attachment 101900 the patch View in context: https://bugs.webkit.org/attachment.cgi?id=101900&action=review > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1211 > + phiNode = m_graph[entry.m_phi]; // reload after vector resize This won’t do what you think it does! It will copy the value from the new memory location into the old memory location. You can’t re-point a reference to a new address with an assignment statement. 441878 5 fpizlo 2011-07-25 14:47:33 -0700 (In reply to comment #4) > (From update of attachment 101900 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101900&action=review > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1211 > > + phiNode = m_graph[entry.m_phi]; // reload after vector resize > > This won’t do what you think it does! > > It will copy the value from the new memory location into the old memory location. > > You can’t re-point a reference to a new address with an assignment statement. Good catch! Fix on the way... 441887 6 101916 fpizlo 2011-07-25 14:57:02 -0700 Created attachment 101916 the patch (fixed for real this time) This addresses the issue that Darin noticed. Tests still running, will change review/commit-queue to ? once (if) they pass. 441891 7 101916 fpizlo 2011-07-25 15:04:17 -0700 Comment on attachment 101916 the patch (fixed for real this time) Tests pass, ready for review. 441929 8 101916 barraclough 2011-07-25 15:49:30 -0700 Comment on attachment 101916 the patch (fixed for real this time) Ooops, good point. 442013 9 101916 webkit.review.bot 2011-07-25 17:25:43 -0700 Comment on attachment 101916 the patch (fixed for real this time) Clearing flags on attachment: 101916 Committed r91728: <http://trac.webkit.org/changeset/91728> 442014 10 webkit.review.bot 2011-07-25 17:25:48 -0700 All reviewed patches have been landed. Closing bug. 101900 2011-07-25 13:22:53 -0700 2011-07-25 14:57:02 -0700 the patch dfgparsevectorref_patch_1.diff text/plain 2303 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gOTE3MDIpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTkgQEAK KzIwMTEtMDctMjUgIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KKworICAgICAgICBE RkcgSklUIGJ5dGVjb2RlIHBhcnNlciBtaXN1c2VzIHBvaW50ZXJzIGludG8gb2JqZWN0cyBhbGxv Y2F0ZWQgYXMgcGFydCBvZiBhCisgICAgICAgIFdURjo6VmVjdG9yLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjUxMjgKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAgICAgCisgICAgICAgIFRoZSBieXRlY29kZSBwYXJz ZXIgY29kZSBzZWVtcyB0byBiZSByaWdodCB0byBoYXZlIGEgREZHTm9kZSYgcGhpTm9kZSByZWZl cmVuY2UKKyAgICAgICAgaW50byB0aGUgZ3JhcGgsIHNpbmNlIHRoaXMgbWFrZXMgdGhlIGNvZGUg Z3JlYXRseSBtb3JlIHJlYWRhYmxlLiAgVGhpcyBwYXRjaAorICAgICAgICB0aHVzIG1ha2VzIHRo ZSBtaW5pbWFsIGNoYW5nZSBuZWNlc3NhcnkgdG8gbWFrZSB0aGUgY29kZSByaWdodDogaXQgcmVs b2FkcworICAgICAgICB0aGUgcGhpTm9kZSByZWZlcmVuY2UgYWZ0ZXIgdGhlIHR3byBjYWxscyB0 byBhZGRUb0dyYXBoKCkuCisKKyAgICAgICAgKiBkZmcvREZHQnl0ZUNvZGVQYXJzZXIuY3BwOgor ICAgICAgICAoSlNDOjpERkc6OkJ5dGVDb2RlUGFyc2VyOjpwcm9jZXNzUGhpU3RhY2spOgorCiAy MDExLTA3LTI1ICBKdWFuIEMuIE1vbnRlbWF5b3IgIDxqbW9udEBhcHBsZS5jb20+CiAKICAgICAg ICAgSlNPTiBlcnJvcnMgc2hvdWxkIGJlIGluZm9ybWF0aXZlCkluZGV4OiBTb3VyY2UvSmF2YVNj cmlwdENvcmUvZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNwcAkocmV2aXNpb24gOTE2OTQp CisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNwcAkod29y a2luZyBjb3B5KQpAQCAtMTE3Miw3ICsxMTcyLDYgQEAgdm9pZCBCeXRlQ29kZVBhcnNlcjo6cHJv Y2Vzc1BoaVN0YWNrKCkKICAgICAgICAgUGhpU3RhY2tFbnRyeSBlbnRyeSA9IHBoaVN0YWNrLmxh c3QoKTsKICAgICAgICAgcGhpU3RhY2sucmVtb3ZlTGFzdCgpOwogICAgICAgICAKLSAgICAgICAg Tm9kZSYgcGhpTm9kZSA9IG1fZ3JhcGhbZW50cnkubV9waGldOwogICAgICAgICBQcmVkZWNlc3Nv ckxpc3QmIHByZWRlY2Vzc29ycyA9IGVudHJ5Lm1fYmxvY2stPm1fcHJlZGVjZXNzb3JzOwogICAg ICAgICB1bnNpZ25lZCB2YXJObyA9IGVudHJ5Lm1fdmFyTm87CiAKQEAgLTExOTAsNiArMTE4OSw3 IEBAIHZvaWQgQnl0ZUNvZGVQYXJzZXI6OnByb2Nlc3NQaGlTdGFjaygpCiAgICAgICAgICAgICAg ICAgdmFsdWVJblByZWRlY2Vzc29yID0gbV9ncmFwaFt2YWx1ZUluUHJlZGVjZXNzb3JdLmNoaWxk MSgpOwogICAgICAgICAgICAgQVNTRVJUKG1fZ3JhcGhbdmFsdWVJblByZWRlY2Vzc29yXS5vcCA9 PSBTZXRMb2NhbCB8fCBtX2dyYXBoW3ZhbHVlSW5QcmVkZWNlc3Nvcl0ub3AgPT0gUGhpKTsKIAor ICAgICAgICAgICAgTm9kZSYgcGhpTm9kZSA9IG1fZ3JhcGhbZW50cnkubV9waGldOwogICAgICAg ICAgICAgaWYgKHBoaU5vZGUucmVmQ291bnQoKSkKICAgICAgICAgICAgICAgICBtX2dyYXBoLnJl Zih2YWx1ZUluUHJlZGVjZXNzb3IpOwogCkBAIC0xMjA3LDYgKzEyMDcsOCBAQCB2b2lkIEJ5dGVD b2RlUGFyc2VyOjpwcm9jZXNzUGhpU3RhY2soKQogICAgICAgICAgICAgfQogCiAgICAgICAgICAg ICBOb2RlSW5kZXggbmV3UGhpID0gYWRkVG9HcmFwaChQaGkpOworICAgICAgICAgICAgCisgICAg ICAgICAgICBwaGlOb2RlID0gbV9ncmFwaFtlbnRyeS5tX3BoaV07IC8vIHJlbG9hZCBhZnRlciB2 ZWN0b3IgcmVzaXplCiAgICAgICAgICAgICBOb2RlJiBuZXdQaGlOb2RlID0gbV9ncmFwaFtuZXdQ aGldOwogICAgICAgICAgICAgaWYgKHBoaU5vZGUucmVmQ291bnQoKSkKICAgICAgICAgICAgICAg ICBtX2dyYXBoLnJlZihuZXdQaGkpOwo= 101916 2011-07-25 14:57:02 -0700 2011-07-25 17:25:43 -0700 the patch (fixed for real this time) dfgparsevectorref_patch_2.diff text/plain 4033 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gOTE3MDIpCisrKyBTb3VyY2Uv SmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjEgQEAK KzIwMTEtMDctMjUgIEZpbGlwIFBpemxvICA8ZnBpemxvQGFwcGxlLmNvbT4KKworICAgICAgICBE RkcgSklUIGJ5dGVjb2RlIHBhcnNlciBtaXN1c2VzIHBvaW50ZXJzIGludG8gb2JqZWN0cyBhbGxv Y2F0ZWQgYXMgcGFydCBvZiBhCisgICAgICAgIFdURjo6VmVjdG9yLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjUxMjgKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAgICAgCisgICAgICAgIFRoZSBieXRlY29kZSBwYXJz ZXIgY29kZSBzZWVtcyB0byBiZSByaWdodCB0byBoYXZlIGEgREZHTm9kZSYgcGhpTm9kZSByZWZl cmVuY2UKKyAgICAgICAgaW50byB0aGUgZ3JhcGgsIHNpbmNlIHRoaXMgbWFrZXMgdGhlIGNvZGUg Z3JlYXRseSBtb3JlIHJlYWRhYmxlLiAgVGhpcyBwYXRjaAorICAgICAgICB0aHVzIG1ha2VzIHRo ZSBtaW5pbWFsIGNoYW5nZSBuZWNlc3NhcnkgdG8gbWFrZSB0aGUgY29kZSByaWdodDogaXQgdXNl cyBhCisgICAgICAgIHBvaW50ZXIgKHRvIGRpc2FtYmlndWF0ZSBiZXR3ZWVuIHJlbG9hZGluZyB0 aGUgcG9pbnRlciBhbmQgcGVyZm9ybWluZyBhCisgICAgICAgIGNvcHkgZnJvbSBvbmUgbG9jYXRp b24gb2YgdGhlIHZlY3RvciB0byBhbm90aGVyKSBhbmQgcmVsb2FkcyBpdCBhZnRlciB0aGUKKyAg ICAgICAgY2FsbHMgdG8gYWRkVG9HcmFwaCgpLgorCisgICAgICAgICogZGZnL0RGR0J5dGVDb2Rl UGFyc2VyLmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpCeXRlQ29kZVBhcnNlcjo6cHJvY2Vzc1Bo aVN0YWNrKToKKwogMjAxMS0wNy0yNSAgSnVhbiBDLiBNb250ZW1heW9yICA8am1vbnRAYXBwbGUu Y29tPgogCiAgICAgICAgIEpTT04gZXJyb3JzIHNob3VsZCBiZSBpbmZvcm1hdGl2ZQpJbmRleDog U291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHAKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHAJKHJl dmlzaW9uIDkxNjk0KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdCeXRlQ29kZVBh cnNlci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTExNzIsNyArMTE3Miw2IEBAIHZvaWQgQnl0ZUNv ZGVQYXJzZXI6OnByb2Nlc3NQaGlTdGFjaygpCiAgICAgICAgIFBoaVN0YWNrRW50cnkgZW50cnkg PSBwaGlTdGFjay5sYXN0KCk7CiAgICAgICAgIHBoaVN0YWNrLnJlbW92ZUxhc3QoKTsKICAgICAg ICAgCi0gICAgICAgIE5vZGUmIHBoaU5vZGUgPSBtX2dyYXBoW2VudHJ5Lm1fcGhpXTsKICAgICAg ICAgUHJlZGVjZXNzb3JMaXN0JiBwcmVkZWNlc3NvcnMgPSBlbnRyeS5tX2Jsb2NrLT5tX3ByZWRl Y2Vzc29yczsKICAgICAgICAgdW5zaWduZWQgdmFyTm8gPSBlbnRyeS5tX3Zhck5vOwogCkBAIC0x MTkwLDM0ICsxMTg5LDM3IEBAIHZvaWQgQnl0ZUNvZGVQYXJzZXI6OnByb2Nlc3NQaGlTdGFjaygp CiAgICAgICAgICAgICAgICAgdmFsdWVJblByZWRlY2Vzc29yID0gbV9ncmFwaFt2YWx1ZUluUHJl ZGVjZXNzb3JdLmNoaWxkMSgpOwogICAgICAgICAgICAgQVNTRVJUKG1fZ3JhcGhbdmFsdWVJblBy ZWRlY2Vzc29yXS5vcCA9PSBTZXRMb2NhbCB8fCBtX2dyYXBoW3ZhbHVlSW5QcmVkZWNlc3Nvcl0u b3AgPT0gUGhpKTsKIAotICAgICAgICAgICAgaWYgKHBoaU5vZGUucmVmQ291bnQoKSkKKyAgICAg ICAgICAgIE5vZGUqIHBoaU5vZGUgPSAmbV9ncmFwaFtlbnRyeS5tX3BoaV07CisgICAgICAgICAg ICBpZiAocGhpTm9kZS0+cmVmQ291bnQoKSkKICAgICAgICAgICAgICAgICBtX2dyYXBoLnJlZih2 YWx1ZUluUHJlZGVjZXNzb3IpOwogCi0gICAgICAgICAgICBpZiAocGhpTm9kZS5jaGlsZDEoKSA9 PSBOb05vZGUpIHsKLSAgICAgICAgICAgICAgICBwaGlOb2RlLmNoaWxkcmVuLmZpeGVkLmNoaWxk MSA9IHZhbHVlSW5QcmVkZWNlc3NvcjsKKyAgICAgICAgICAgIGlmIChwaGlOb2RlLT5jaGlsZDEo KSA9PSBOb05vZGUpIHsKKyAgICAgICAgICAgICAgICBwaGlOb2RlLT5jaGlsZHJlbi5maXhlZC5j aGlsZDEgPSB2YWx1ZUluUHJlZGVjZXNzb3I7CiAgICAgICAgICAgICAgICAgY29udGludWU7CiAg ICAgICAgICAgICB9Ci0gICAgICAgICAgICBpZiAocGhpTm9kZS5jaGlsZDIoKSA9PSBOb05vZGUp IHsKLSAgICAgICAgICAgICAgICBwaGlOb2RlLmNoaWxkcmVuLmZpeGVkLmNoaWxkMiA9IHZhbHVl SW5QcmVkZWNlc3NvcjsKKyAgICAgICAgICAgIGlmIChwaGlOb2RlLT5jaGlsZDIoKSA9PSBOb05v ZGUpIHsKKyAgICAgICAgICAgICAgICBwaGlOb2RlLT5jaGlsZHJlbi5maXhlZC5jaGlsZDIgPSB2 YWx1ZUluUHJlZGVjZXNzb3I7CiAgICAgICAgICAgICAgICAgY29udGludWU7CiAgICAgICAgICAg ICB9Ci0gICAgICAgICAgICBpZiAocGhpTm9kZS5jaGlsZDMoKSA9PSBOb05vZGUpIHsKLSAgICAg ICAgICAgICAgICBwaGlOb2RlLmNoaWxkcmVuLmZpeGVkLmNoaWxkMyA9IHZhbHVlSW5QcmVkZWNl c3NvcjsKKyAgICAgICAgICAgIGlmIChwaGlOb2RlLT5jaGlsZDMoKSA9PSBOb05vZGUpIHsKKyAg ICAgICAgICAgICAgICBwaGlOb2RlLT5jaGlsZHJlbi5maXhlZC5jaGlsZDMgPSB2YWx1ZUluUHJl ZGVjZXNzb3I7CiAgICAgICAgICAgICAgICAgY29udGludWU7CiAgICAgICAgICAgICB9CiAKICAg ICAgICAgICAgIE5vZGVJbmRleCBuZXdQaGkgPSBhZGRUb0dyYXBoKFBoaSk7CisgICAgICAgICAg ICAKKyAgICAgICAgICAgIHBoaU5vZGUgPSAmbV9ncmFwaFtlbnRyeS5tX3BoaV07IC8vIHJlbG9h ZCBhZnRlciB2ZWN0b3IgcmVzaXplCiAgICAgICAgICAgICBOb2RlJiBuZXdQaGlOb2RlID0gbV9n cmFwaFtuZXdQaGldOwotICAgICAgICAgICAgaWYgKHBoaU5vZGUucmVmQ291bnQoKSkKKyAgICAg ICAgICAgIGlmIChwaGlOb2RlLT5yZWZDb3VudCgpKQogICAgICAgICAgICAgICAgIG1fZ3JhcGgu cmVmKG5ld1BoaSk7CiAKLSAgICAgICAgICAgIG5ld1BoaU5vZGUuY2hpbGRyZW4uZml4ZWQuY2hp bGQxID0gcGhpTm9kZS5jaGlsZDEoKTsKLSAgICAgICAgICAgIG5ld1BoaU5vZGUuY2hpbGRyZW4u Zml4ZWQuY2hpbGQyID0gcGhpTm9kZS5jaGlsZDIoKTsKLSAgICAgICAgICAgIG5ld1BoaU5vZGUu Y2hpbGRyZW4uZml4ZWQuY2hpbGQzID0gcGhpTm9kZS5jaGlsZDMoKTsKLQotICAgICAgICAgICAg cGhpTm9kZS5jaGlsZHJlbi5maXhlZC5jaGlsZDEgPSBuZXdQaGk7Ci0gICAgICAgICAgICBwaGlO b2RlLmNoaWxkcmVuLmZpeGVkLmNoaWxkMSA9IHZhbHVlSW5QcmVkZWNlc3NvcjsKLSAgICAgICAg ICAgIHBoaU5vZGUuY2hpbGRyZW4uZml4ZWQuY2hpbGQzID0gTm9Ob2RlOworICAgICAgICAgICAg bmV3UGhpTm9kZS5jaGlsZHJlbi5maXhlZC5jaGlsZDEgPSBwaGlOb2RlLT5jaGlsZDEoKTsKKyAg ICAgICAgICAgIG5ld1BoaU5vZGUuY2hpbGRyZW4uZml4ZWQuY2hpbGQyID0gcGhpTm9kZS0+Y2hp bGQyKCk7CisgICAgICAgICAgICBuZXdQaGlOb2RlLmNoaWxkcmVuLmZpeGVkLmNoaWxkMyA9IHBo aU5vZGUtPmNoaWxkMygpOworCisgICAgICAgICAgICBwaGlOb2RlLT5jaGlsZHJlbi5maXhlZC5j aGlsZDEgPSBuZXdQaGk7CisgICAgICAgICAgICBwaGlOb2RlLT5jaGlsZHJlbi5maXhlZC5jaGls ZDEgPSB2YWx1ZUluUHJlZGVjZXNzb3I7CisgICAgICAgICAgICBwaGlOb2RlLT5jaGlsZHJlbi5m aXhlZC5jaGlsZDMgPSBOb05vZGU7CiAgICAgICAgIH0KICAgICB9CiB9Cg==