30502 2009-10-18 22:48:34 -0700 Dereference of uninitialized variable 2009-10-19 10:36:11 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P3 Minor --- 1 tkent webkit-unassigned ap commit-queue oldest_to_newest 155569 0 tkent 2009-10-18 22:48:34 -0700 WebCore/dom/Element.cpp setBooleanAttribute(); ExceptionCode ex; removeAttribute(name, ex); removeAttribute(): void Element::removeAttribute(const QualifiedName& name, ExceptionCode& ec) { if (namedAttrMap) { namedAttrMap->removeNamedItem(name, ec); if (ec == NOT_FOUND_ERR) ec = 0; } } removeNamedItem() doesn't set any value to ec if the specified attribute exists. So, if removeAttribute() is called by setBooleanAttribute(), uninitialized ec can be referred at "if (ec == NOT_FOUND_ERR)". Note: This never makes a real bug because ex in setBooleanAttribute() is not used after removeAttribute() call. Valgrind complaints about this. 155578 1 41398 tkent 2009-10-18 23:11:45 -0700 Created attachment 41398 Proposed patch 155589 2 mrowe 2009-10-18 23:52:14 -0700 Wouldn’t it make more sense to initialize the variable at the level that it matters? setBooleanAttribute doesn’t care about the exception code so there’s no need for it to do anything with it. After your change the code in removeAttribute will still do the wrong thing if called by another caller. 155590 3 tkent 2009-10-18 23:55:30 -0700 (In reply to comment #2) > Wouldn’t it make more sense to initialize the variable at the level that it > matters? setBooleanAttribute doesn’t care about the exception code so there’s > no need for it to do anything with it. After your change the code in > removeAttribute will still do the wrong thing if called by another caller. I think our convention for ExceptionCode is "initialize it with 0 when it is declared." 155594 4 tkent 2009-10-19 00:00:56 -0700 > I think our convention for ExceptionCode is "initialize it with 0 when it is > declared." So, if removeAttribute() set 0 before removenamedItem() and other callers initialize ec with 0, we would have redundant initialization. 155596 5 mrowe 2009-10-19 00:14:54 -0700 (In reply to comment #3) > (In reply to comment #2) > > Wouldn’t it make more sense to initialize the variable at the level that it > > matters? setBooleanAttribute doesn’t care about the exception code so there’s > > no need for it to do anything with it. After your change the code in > > removeAttribute will still do the wrong thing if called by another caller. > > I think our convention for ExceptionCode is "initialize it with 0 when it is > declared.” A quick grep in WebCore for “ExceptionCode ec;” shows that is not the case. 155601 6 41398 ap 2009-10-19 00:20:04 -0700 Comment on attachment 41398 Proposed patch As Mark said - if a caller doesn't plan to look at ExceptionCode, it can pass an uninitialized value. See bug 21939 for more detailed discussion of the current policy (especially comments 3 and 12 there). 155604 7 41400 tkent 2009-10-19 00:30:01 -0700 Created attachment 41400 Proposed patch (rev.2) ok, I follow it. Updated the patch so that removeAttribute() initializes ec. 155684 8 41400 ap 2009-10-19 08:14:34 -0700 Comment on attachment 41400 Proposed patch (rev.2) r=me It may make sense to add a comment explaining that this assignment is only needed to please tools like valgrind - since it's useless otherwise, people will be tempted to remove it. 155702 9 41400 yong.li.webkit 2009-10-19 08:49:35 -0700 Comment on attachment 41400 Proposed patch (rev.2) Let commit bot land it 155747 10 41400 commit-queue 2009-10-19 09:42:44 -0700 Comment on attachment 41400 Proposed patch (rev.2) Rejecting patch 41400 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11468 test cases. http/tests/navigation/postredirect-reload.html -> crashed Exiting early after 1 failures. 8689 tests run. 390.95s total testing time 8688 test cases (99%) succeeded 1 test case (<1%) crashed 5 test cases (<1%) had stderr output 155759 11 41400 eric 2009-10-19 10:11:57 -0700 Comment on attachment 41400 Proposed patch (rev.2) That crash looks unrelated to your patch. I've filed bug 30519. Adding this back to the commit-queue. 155772 12 41400 commit-queue 2009-10-19 10:36:06 -0700 Comment on attachment 41400 Proposed patch (rev.2) Clearing flags on attachment: 41400 Committed r49792: <http://trac.webkit.org/changeset/49792> 155773 13 commit-queue 2009-10-19 10:36:11 -0700 All reviewed patches have been landed. Closing bug. 41398 2009-10-18 23:11:45 -0700 2009-10-19 00:30:01 -0700 Proposed patch 0001-uninitialized-variable.patch text/plain 1389 tkent RnJvbSA2Y2VjM2NmOGJiODg3ZTBiZWFjNWEzOGVlZGQ4NDMwZmM3YzYxM2ExIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBLZW50IFRhbXVyYSA8dGtlbnRAY2hyb21pdW0ub3JnPgpEYXRl OiBNb24sIDE5IE9jdCAyMDA5IDE1OjA4OjQwICswOTAwClN1YmplY3Q6IFtQQVRDSF0gdW5pbml0 aWFsaXplZC12YXJpYWJsZQoKLS0tCiBXZWJDb3JlL0NoYW5nZUxvZyAgICAgICB8ICAgMTEgKysr KysrKysrKysKIFdlYkNvcmUvZG9tL0VsZW1lbnQuY3BwIHwgICAgNCArKy0tCiAyIGZpbGVzIGNo YW5nZWQsIDEzIGluc2VydGlvbnMoKyksIDIgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvV2Vi Q29yZS9DaGFuZ2VMb2cgYi9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCAwMjdlOWFhLi40YzIyMjgz IDEwMDY0NAotLS0gYS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9XZWJDb3JlL0NoYW5nZUxvZwpA QCAtMSwzICsxLDE0IEBACisyMDA5LTEwLTE4ICBLZW50IFRhbXVyYSAgPHRrZW50QGNocm9taXVt Lm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBG aXggdW5pbml0aWFsaXplZCB2YXJpYWJsZSByZWZlcmVuY2UgaW4gRWxlbWVudDo6cmVtb3ZlQXR0 cmlidXRlKCkKKyAgICAgICAgY2FsbGVkIGJ5IHNldEJvb2xlYW5BdHRyaWJ1dGUoKS4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTMwNTAyCisKKyAgICAg ICAgKiBkb20vRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpFbGVtZW50OjpzZXRCb29s ZWFuQXR0cmlidXRlKToKKwogMjAwOS0xMC0xOCAgS2V2aW4gT2xsaXZpZXIgIDxrZXZpbm9AdGhl b2xsaXZpZXJzLmNvbT4KIAogICAgICAgICBDVVJMIGJ1aWxkIGZpeCwgdXNlIHByb3BlciBoZWFk ZXIgbmFtZS4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwIGIvV2ViQ29yZS9k b20vRWxlbWVudC5jcHAKaW5kZXggNTBmZjAzMy4uZjcxZjVmOSAxMDA2NDQKLS0tIGEvV2ViQ29y ZS9kb20vRWxlbWVudC5jcHAKKysrIGIvV2ViQ29yZS9kb20vRWxlbWVudC5jcHAKQEAgLTE0MCw4 ICsxNDAsOCBAQCB2b2lkIEVsZW1lbnQ6OnNldEJvb2xlYW5BdHRyaWJ1dGUoY29uc3QgUXVhbGlm aWVkTmFtZSYgbmFtZSwgYm9vbCBiKQogICAgIGlmIChiKQogICAgICAgICBzZXRBdHRyaWJ1dGUo bmFtZSwgbmFtZS5sb2NhbE5hbWUoKSk7CiAgICAgZWxzZSB7Ci0gICAgICAgIEV4Y2VwdGlvbkNv ZGUgZXg7Ci0gICAgICAgIHJlbW92ZUF0dHJpYnV0ZShuYW1lLCBleCk7CisgICAgICAgIEV4Y2Vw dGlvbkNvZGUgZWMgPSAwOworICAgICAgICByZW1vdmVBdHRyaWJ1dGUobmFtZSwgZWMpOwogICAg IH0KIH0KIAotLSAKMS42LjMuMwoK 41400 2009-10-19 00:30:01 -0700 2009-10-19 10:36:06 -0700 Proposed patch (rev.2) 0001-uninitialized-variable-2.patch text/plain 1346 tkent RnJvbSA4MWI3OTUxZTg5NDMwNWFjMDcxZDQwZDhlOTg4YjEwNDE2N2QwZDM3IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBLZW50IFRhbXVyYSA8dGtlbnRAY2hyb21pdW0ub3JnPgpEYXRl OiBNb24sIDE5IE9jdCAyMDA5IDE1OjA4OjQwICswOTAwClN1YmplY3Q6IFtQQVRDSF0gdW5pbml0 aWFsaXplZC12YXJpYWJsZS0yCgotLS0KIFdlYkNvcmUvQ2hhbmdlTG9nICAgICAgIHwgICAxMCAr KysrKysrKysrCiBXZWJDb3JlL2RvbS9FbGVtZW50LmNwcCB8ICAgIDEgKwogMiBmaWxlcyBjaGFu Z2VkLCAxMSBpbnNlcnRpb25zKCspLCAwIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1dlYkNv cmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDI3ZTlhYS4uOWRmNGQwOSAx MDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAg LTEsMyArMSwxMyBAQAorMjAwOS0xMC0xOCAgS2VudCBUYW11cmEgIDx0a2VudEBjaHJvbWl1bS5v cmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRml4 IHVuaW5pdGlhbGl6ZWQgdmFyaWFibGUgcmVmZXJlbmNlIGluIEVsZW1lbnQ6OnJlbW92ZUF0dHJp YnV0ZSgpLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MzA1MDIKKworICAgICAgICAqIGRvbS9FbGVtZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkVs ZW1lbnQ6OnJlbW92ZUF0dHJpYnV0ZSk6CisKIDIwMDktMTAtMTggIEtldmluIE9sbGl2aWVyICA8 a2V2aW5vQHRoZW9sbGl2aWVycy5jb20+CiAKICAgICAgICAgQ1VSTCBidWlsZCBmaXgsIHVzZSBw cm9wZXIgaGVhZGVyIG5hbWUuCmRpZmYgLS1naXQgYS9XZWJDb3JlL2RvbS9FbGVtZW50LmNwcCBi L1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwCmluZGV4IDUwZmYwMzMuLjkzMGE2YTIgMTAwNjQ0Ci0t LSBhL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwCisrKyBiL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3Bw CkBAIC0xMjMsNiArMTIzLDcgQEAgUGFzc1JlZlB0cjxFbGVtZW50PiBFbGVtZW50OjpjbG9uZUVs ZW1lbnRXaXRob3V0Q2hpbGRyZW4oKQogdm9pZCBFbGVtZW50OjpyZW1vdmVBdHRyaWJ1dGUoY29u c3QgUXVhbGlmaWVkTmFtZSYgbmFtZSwgRXhjZXB0aW9uQ29kZSYgZWMpCiB7CiAgICAgaWYgKG5h bWVkQXR0ck1hcCkgeworICAgICAgICBlYyA9IDA7CiAgICAgICAgIG5hbWVkQXR0ck1hcC0+cmVt b3ZlTmFtZWRJdGVtKG5hbWUsIGVjKTsKICAgICAgICAgaWYgKGVjID09IE5PVF9GT1VORF9FUlIp CiAgICAgICAgICAgICBlYyA9IDA7Ci0tIAoxLjYuMy4zCgo=