26630 2009-06-22 16:15:32 -0700 Annotate WTF assertion methods to prevent false-positives from clang static analyzer 2011-08-13 10:41:28 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Mac OS X 10.5 RESOLVED DUPLICATE 66152 http://clang-analyzer.llvm.org/annotations.html#custom_assertions P2 Normal --- 42796 1 ddkilzer ddkilzer darin harrison mrowe sam oldest_to_newest 127491 0 ddkilzer 2009-06-22 16:15:32 -0700 * SUMMARY When using the clang static analyzer on C and Objective-C source code that uses ASSERT() macros, the following false-positive issue is reported: source.m:7:5: warning: Called function pointer is a null or undefined pointer value ASSERT(ptr); ^ Assertions.h:165:9: note: instantiated from: CRASH(); \ ^ Assertions.h:134:5: note: instantiated from: ((void(*)())0)(); /* More reliable, but doesn't say BBADBEEF */ \ ^ 1 diagnostic generated. * RESOLUTION The fix is to annotate the WTFReport*() methods in Assertions.h with an attribute that tells the clang static analyzer that this method effectively never returns. That, in turn, prevents the false positive issue in the CRASH() macro from being emitted. This has the desired effect of squelching the false positive. * NOTES See the URL for more details about this custom annotation and how it's used by the clang static analyzer. 127493 1 mrowe 2009-06-22 16:18:50 -0700 Would switching to abort() rather than dereferencing a bad address also have the same effect as adding annotations to our own code? 127495 2 31691 ddkilzer 2009-06-22 16:26:04 -0700 Created attachment 31691 Patch v1 Proposed fix. 127496 3 ddkilzer 2009-06-22 16:36:08 -0700 (In reply to comment #1) > Would switching to abort() rather than dereferencing a bad address also have > the same effect as adding annotations to our own code? Yes, it has the same effect. 127497 4 31691 mrowe 2009-06-22 16:41:06 -0700 Comment on attachment 31691 Patch v1 Let's look in to a solution using abort() rather than some whacky attributes instead. 127678 5 ddkilzer 2009-06-23 13:50:41 -0700 (In reply to comment #4) > (From update of attachment 31691 [review]) > Let's look in to a solution using abort() rather than some whacky attributes > instead. This will essentially require disabling -Wmissing-noreturn in WebCore as there are about 71 files that don't compile when we switch CRASH() to use abort() from stdlib.h. Otherwise every function that uses ASSERT_NOT_REACHED, like this one in JSPropertyNameIterator.cpp,: JSValue JSPropertyNameIterator::toPrimitive(ExecState*, PreferredPrimitiveType) const { ASSERT_NOT_REACHED(); return JSValue(); } must be modified thusly (and don't forget the UNUSED_PARAM() macros that must be added if the argument names are present): ASSERT_NO_RETURN JSValue JSPropertyNameIterator::toPrimitive(ExecState*, PreferredPrimitiveType) const { #ifndef NDEBUG ASSERT_NOT_REACHED(); #else return JSValue(); #endif } where "ASSERT_NO_RETURN" is defined in wtf/AlwaysInline.h thusly: #if defined(NDEBUG) #define ASSERT_NO_RETURN #else #define ASSERT_NO_RETURN NO_RETURN #endif I think it's easier to stop using -Wmissing-noreturn (e.g., switch to using -Wno-missing-noreturn) than to add the above markup to every method that uses ASSERT_NOT_REACHED(). 127682 6 mrowe 2009-06-23 14:00:42 -0700 (In reply to comment #5) > I think it's easier to stop using -Wmissing-noreturn (e.g., switch to using > -Wno-missing-noreturn) than to add the above markup to every method that uses > ASSERT_NOT_REACHED(). Agreed. 141922 7 ddkilzer 2009-08-21 11:47:55 -0700 (In reply to comment #4) > (From update of attachment 31691 [details]) > Let's look in to a solution using abort() rather than some whacky attributes > instead. Some concern was expressed by David Harrison and Darin Adler about using abort() instead of the current method of dereferencing 0xbbadbeef in that (1) the crash would not occur in the same frame as the ASSERT() or CRASH() macro was declared and that (2) gcc may not preserve stack variables at the actual point of the crash when the frame was changed back to the ASSERT() or CRASH() macro in gdb. Switching to abort() thus requires additional testing on various combinations of operating systems and architectures to make sure that stack variables are preserved when the crash occurs and the frame is moved to where the crash happens (in addition to the changes in Comment #5). 254925 8 darin 2010-07-22 12:46:18 -0700 Another possibility is to combine the current approach with an abort function call after the code designed to cause a crash. 450851 9 sam 2011-08-13 10:41:08 -0700 I wind up fixing this with bug 66152. *** This bug has been marked as a duplicate of bug 66152 *** 450853 10 sam 2011-08-13 10:41:28 -0700 (In reply to comment #9) > I wind up fixing this with bug 66152. > > *** This bug has been marked as a duplicate of bug 66152 *** *wound* 31691 2009-06-22 16:26:04 -0700 2010-06-10 18:50:58 -0700 Patch v1 bug-26630-v1.diff text/plain 4348 ddkilzer Y29tbWl0IDU2MTVjMTAxZWQ2MjllZDdlOWUwYmRhNGM0YWFjOWQzODE1ZWIwYjQKQXV0aG9yOiBE YXZpZCBLaWx6ZXIgPGRka2lsemVyQGFwcGxlLmNvbT4KRGF0ZTogICBNb24gSnVuIDIyIDE2OjIz OjU0IDIwMDkgLTA3MDAKCiAgICAgICAgICAgIEJ1ZyAyNjYzMDogQW5ub3RhdGUgV1RGIGFzc2Vy dGlvbiBtZXRob2RzIHRvIHByZXZlbnQgZmFsc2UtcG9zaXRpdmVzIGZyb20gY2xhbmcgc3RhdGlj IGFuYWx5emVyCiAgICAKICAgICAgICAgICAgPGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0yNjYzMD4KICAgIAogICAgICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KICAgIAogICAgICAgICAgICAqIHd0Zi9Bc3NlcnRpb25zLmg6IEFkZGVkIENMQU5HX0FO QUxZWkVSX05PUkVUVVJOIG1hY3JvCiAgICAgICAgICAgIGRlZmluaXRpb24uIEFkZGVkICNpbmNs dWRlIDxzdGRib29sLmg+IGZvciBkZWZpbml0aW9uIG9mIGZhbHNlCiAgICAgICAgICAgIGluIEMg c291cmNlLgogICAgICAgICAgICAoV1RGUmVwb3J0QXNzZXJ0aW9uRmFpbHVyZSk6IEFkZGVkIENM QU5HX0FOQUxZWkVSX05PUkVUVVJOCiAgICAgICAgICAgIGFubm90YXRpb24gdG8gZml4IGZhbHNl LXBvc2l0aXZlcyBpbiBBU1NFUlQoKSBtYWNyby4KICAgICAgICAgICAgKFdURlJlcG9ydEFzc2Vy dGlvbkZhaWx1cmVXaXRoTWVzc2FnZSk6IERpdHRvIGZvcgogICAgICAgICAgICBBU1NFUlRfV0lU SF9NRVNTQUdFKCkgbWFjcm8uCiAgICAgICAgICAgIChXVEZSZXBvcnRBcmd1bWVudEFzc2VydGlv bkZhaWx1cmUpOiBEaXR0byBmb3IgQVNTRVJUX0FSRygpCiAgICAgICAgICAgIG1hY3JvLgogICAg ICAgICAgICAoV1RGUmVwb3J0RmF0YWxFcnJvcik6IERpdHRvIGZvciBGQVRBTCgpIG1hY3JvLgoK ZGlmZiAtLWdpdCBhL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZyBiL0phdmFTY3JpcHRDb3JlL0No YW5nZUxvZwppbmRleCA1MjgxMTIzLi5jYjk5Nzg0IDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29y ZS9DaGFuZ2VMb2cKKysrIGIvSmF2YVNjcmlwdENvcmUvQ2hhbmdlTG9nCkBAIC0xICsxLDIwIEBA CisyMDA5LTA2LTIyICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+CisKKyAgICAg ICAgQnVnIDI2NjMwOiBBbm5vdGF0ZSBXVEYgYXNzZXJ0aW9uIG1ldGhvZHMgdG8gcHJldmVudCBm YWxzZS1wb3NpdGl2ZXMgZnJvbSBjbGFuZyBzdGF0aWMgYW5hbHl6ZXIKKworICAgICAgICA8aHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI2NjMwPgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogd3RmL0Fzc2VydGlvbnMuaDog QWRkZWQgQ0xBTkdfQU5BTFlaRVJfTk9SRVRVUk4gbWFjcm8KKyAgICAgICAgZGVmaW5pdGlvbi4g QWRkZWQgI2luY2x1ZGUgPHN0ZGJvb2wuaD4gZm9yIGRlZmluaXRpb24gb2YgZmFsc2UKKyAgICAg ICAgaW4gQyBzb3VyY2UuCisgICAgICAgIChXVEZSZXBvcnRBc3NlcnRpb25GYWlsdXJlKTogQWRk ZWQgQ0xBTkdfQU5BTFlaRVJfTk9SRVRVUk4KKyAgICAgICAgYW5ub3RhdGlvbiB0byBmaXggZmFs c2UtcG9zaXRpdmVzIGluIEFTU0VSVCgpIG1hY3JvLgorICAgICAgICAoV1RGUmVwb3J0QXNzZXJ0 aW9uRmFpbHVyZVdpdGhNZXNzYWdlKTogRGl0dG8gZm9yCisgICAgICAgIEFTU0VSVF9XSVRIX01F U1NBR0UoKSBtYWNyby4KKyAgICAgICAgKFdURlJlcG9ydEFyZ3VtZW50QXNzZXJ0aW9uRmFpbHVy ZSk6IERpdHRvIGZvciBBU1NFUlRfQVJHKCkKKyAgICAgICAgbWFjcm8uCisgICAgICAgIChXVEZS ZXBvcnRGYXRhbEVycm9yKTogRGl0dG8gZm9yIEZBVEFMKCkgbWFjcm8uCisKID09IFJvbGxlZCBv dmVyIHRvIENoYW5nZUxvZy0yMDA5LTA2LTE2ID09CmRpZmYgLS1naXQgYS9KYXZhU2NyaXB0Q29y ZS93dGYvQXNzZXJ0aW9ucy5oIGIvSmF2YVNjcmlwdENvcmUvd3RmL0Fzc2VydGlvbnMuaAppbmRl eCA5NjQzNTE3Li4zYzBmN2VjIDEwMDY0NAotLS0gYS9KYXZhU2NyaXB0Q29yZS93dGYvQXNzZXJ0 aW9ucy5oCisrKyBiL0phdmFTY3JpcHRDb3JlL3d0Zi9Bc3NlcnRpb25zLmgKQEAgLTQ0LDYgKzQ0 LDggQEAKIAogI2luY2x1ZGUgIlBsYXRmb3JtLmgiCiAKKyNpbmNsdWRlIDxzdGRib29sLmg+CisK ICNpZiBDT01QSUxFUihNU1ZDKQogI2luY2x1ZGUgPHN0ZGRlZi5oPgogI2Vsc2UKQEAgLTkxLDYg KzkzLDEzIEBACiAjZGVmaW5lIFdURl9BVFRSSUJVVEVfUFJJTlRGKGZvcm1hdFN0cmluZ0FyZ3Vt ZW50LCBleHRyYUFyZ3VtZW50cykgCiAjZW5kaWYKIAorLyogVGhpcyBtYWNybyBpcyBuZWVkZWQg dG8gcHJldmVudCB0aGUgY2xhbmcgc3RhdGljIGFuYWx5emVyIGZyb20gZ2VuZXJhdGluZyBmYWxz ZS1wb3NpdGl2ZSByZXBvcnRzIGluIEFTU0VSVCgpIG1hY3Jvcy4gKi8KKyNpZmRlZiBfX2NsYW5n X18KKyNkZWZpbmUgQ0xBTkdfQU5BTFlaRVJfTk9SRVRVUk4gX19hdHRyaWJ1dGVfXygoYW5hbHl6 ZXJfbm9yZXR1cm4pKQorI2Vsc2UKKyNkZWZpbmUgQ0xBTkdfQU5BTFlaRVJfTk9SRVRVUk4KKyNl bmRpZgorCiAvKiBUaGVzZSBoZWxwZXIgZnVuY3Rpb25zIGFyZSBhbHdheXMgZGVjbGFyZWQsIGJ1 dCBub3QgbmVjZXNzYXJpbHkgYWx3YXlzIGRlZmluZWQgaWYgdGhlIGNvcnJlc3BvbmRpbmcgZnVu Y3Rpb24gaXMgZGlzYWJsZWQuICovCiAKICNpZmRlZiBfX2NwbHVzcGx1cwpAQCAtMTA1LDEwICsx MTQsMTAgQEAgdHlwZWRlZiBzdHJ1Y3QgewogICAgIFdURkxvZ0NoYW5uZWxTdGF0ZSBzdGF0ZTsK IH0gV1RGTG9nQ2hhbm5lbDsKIAotdm9pZCBXVEZSZXBvcnRBc3NlcnRpb25GYWlsdXJlKGNvbnN0 IGNoYXIqIGZpbGUsIGludCBsaW5lLCBjb25zdCBjaGFyKiBmdW5jdGlvbiwgY29uc3QgY2hhciog YXNzZXJ0aW9uKTsKLXZvaWQgV1RGUmVwb3J0QXNzZXJ0aW9uRmFpbHVyZVdpdGhNZXNzYWdlKGNv bnN0IGNoYXIqIGZpbGUsIGludCBsaW5lLCBjb25zdCBjaGFyKiBmdW5jdGlvbiwgY29uc3QgY2hh ciogYXNzZXJ0aW9uLCBjb25zdCBjaGFyKiBmb3JtYXQsIC4uLikgV1RGX0FUVFJJQlVURV9QUklO VEYoNSwgNik7Ci12b2lkIFdURlJlcG9ydEFyZ3VtZW50QXNzZXJ0aW9uRmFpbHVyZShjb25zdCBj aGFyKiBmaWxlLCBpbnQgbGluZSwgY29uc3QgY2hhciogZnVuY3Rpb24sIGNvbnN0IGNoYXIqIGFy Z05hbWUsIGNvbnN0IGNoYXIqIGFzc2VydGlvbik7Ci12b2lkIFdURlJlcG9ydEZhdGFsRXJyb3Io Y29uc3QgY2hhciogZmlsZSwgaW50IGxpbmUsIGNvbnN0IGNoYXIqIGZ1bmN0aW9uLCBjb25zdCBj aGFyKiBmb3JtYXQsIC4uLikgV1RGX0FUVFJJQlVURV9QUklOVEYoNCwgNSk7Cit2b2lkIFdURlJl cG9ydEFzc2VydGlvbkZhaWx1cmUoY29uc3QgY2hhciogZmlsZSwgaW50IGxpbmUsIGNvbnN0IGNo YXIqIGZ1bmN0aW9uLCBjb25zdCBjaGFyKiBhc3NlcnRpb24pIENMQU5HX0FOQUxZWkVSX05PUkVU VVJOOwordm9pZCBXVEZSZXBvcnRBc3NlcnRpb25GYWlsdXJlV2l0aE1lc3NhZ2UoY29uc3QgY2hh ciogZmlsZSwgaW50IGxpbmUsIGNvbnN0IGNoYXIqIGZ1bmN0aW9uLCBjb25zdCBjaGFyKiBhc3Nl cnRpb24sIGNvbnN0IGNoYXIqIGZvcm1hdCwgLi4uKSBDTEFOR19BTkFMWVpFUl9OT1JFVFVSTiBX VEZfQVRUUklCVVRFX1BSSU5URig1LCA2KTsKK3ZvaWQgV1RGUmVwb3J0QXJndW1lbnRBc3NlcnRp b25GYWlsdXJlKGNvbnN0IGNoYXIqIGZpbGUsIGludCBsaW5lLCBjb25zdCBjaGFyKiBmdW5jdGlv biwgY29uc3QgY2hhciogYXJnTmFtZSwgY29uc3QgY2hhciogYXNzZXJ0aW9uKSBDTEFOR19BTkFM WVpFUl9OT1JFVFVSTjsKK3ZvaWQgV1RGUmVwb3J0RmF0YWxFcnJvcihjb25zdCBjaGFyKiBmaWxl LCBpbnQgbGluZSwgY29uc3QgY2hhciogZnVuY3Rpb24sIGNvbnN0IGNoYXIqIGZvcm1hdCwgLi4u KSBDTEFOR19BTkFMWVpFUl9OT1JFVFVSTiBXVEZfQVRUUklCVVRFX1BSSU5URig0LCA1KTsKIHZv aWQgV1RGUmVwb3J0RXJyb3IoY29uc3QgY2hhciogZmlsZSwgaW50IGxpbmUsIGNvbnN0IGNoYXIq IGZ1bmN0aW9uLCBjb25zdCBjaGFyKiBmb3JtYXQsIC4uLikgV1RGX0FUVFJJQlVURV9QUklOVEYo NCwgNSk7CiB2b2lkIFdURkxvZyhXVEZMb2dDaGFubmVsKiBjaGFubmVsLCBjb25zdCBjaGFyKiBm b3JtYXQsIC4uLikgV1RGX0FUVFJJQlVURV9QUklOVEYoMiwgMyk7CiB2b2lkIFdURkxvZ1ZlcmJv c2UoY29uc3QgY2hhciogZmlsZSwgaW50IGxpbmUsIGNvbnN0IGNoYXIqIGZ1bmN0aW9uLCBXVEZM b2dDaGFubmVsKiBjaGFubmVsLCBjb25zdCBjaGFyKiBmb3JtYXQsIC4uLikgV1RGX0FUVFJJQlVU RV9QUklOVEYoNSwgNik7Cg==