Double delete does not result in a -Wanalyzer-double-free warning as expected, but rather into -Wanalyzer-use-after-free warning. Using the following reproducer: // file ../../double_delete_test.cpp struct A {int a; int b;}; int main () { A* a = new A(); delete a; delete a; return 0; } Then compiling with ./xg++ -B. -S -fanalyzer -Wanalyzer-double-free ../../double_delete_test.cpp ../../double_delete_test.cpp: In function ‘int main()’: ../../double_delete_test.cpp:9:1: warning: use after ‘delete’ of ‘a’ [CWE-416] [-Wanalyzer-use-after-free] 9 | } | ^ ‘int main()’: events 1-7 | | 5 | A* a = new A(); | | ^ | | | | | (1) state of ‘&HEAP_ALLOCATED_REGION(10)’: ‘start’ -> ‘nonnull’ (NULL origin) | 6 | delete a; | | ~~~~~~~~ | | | | | | | (3) ...to here | | | (4) deleted here | | (2) following ‘true’ branch... | 7 | delete a; | | ~~~~~~~~ | | | | | | | (6) ...to here | | (5) following ‘true’ branch... | 8 | return 0; | 9 | } | | ~ | | | | | (7) use after ‘delete’ of ‘a’; deleted at (4) | I also attempted with -fno-exception, but no impact was observer on the graphs nor the output. With the addition of -fanalyzer-fine-grained, I observed than each delete statement is actually split into two: delete a; becomes in the ipa form <bb 3> : *a.0_9 ={v} {CLOBBER}; operator delete (a.0_9, 8); The exploded-graph shows that the second '*a.1_12 ={v} {CLOBBER};' dereference is responsible for the -Wanalyzer-use-after-free, and changes the state of the allocated region from 'freed' to 'stop', which causes the actual following 'operator delete' to not be detected as a double free. I am still familiarizing myself with the gimplification and ssa passes, so I'm yet unsure as to how to tackle this. I'm still looking into this though, and would gladly receive your pointers. (Note: sorry David, I've binged through bugzilla doc and gcc bugs page yet I cannot seem to find the way to add this to the 'analyzer-c++' block, nor do I see the option in the advanced fields.)
(In reply to Benjamin Priour from comment #0) [...] > (Note: sorry David, I've binged through bugzilla doc and gcc bugs page yet I > cannot seem to find the way to add this to the 'analyzer-c++' block, nor do > I see the option in the advanced fields.) I've added it for you; see the "Blocks: " field of show_bug.cgi
(In reply to David Malcolm from comment #1) > (In reply to Benjamin Priour from comment #0) > [...] > > (Note: sorry David, I've binged through bugzilla doc and gcc bugs page yet I > > cannot seem to find the way to add this to the 'analyzer-c++' block, nor do > > I see the option in the advanced fields.) > > I've added it for you; see the "Blocks: " field of show_bug.cgi Ahah yes thank you, you sure were fast, I only had time to notice the field actually appearing only after I submitted the bug, that you had already updated it.
Changing the second delete expression into a direct call to "operator delete" results in the correct warning Wanalyzer-double-free being emitted. This is due to delete expression first calling the destructor and performing extra operations, whose one of them is a dereference. "delete expression" compiled with -O0 results on my x86_64-linux-gnu to analyzer ipa: if (a.0_11 != 0B) goto <bb 3>; [INV] else goto <bb 4>; [INV] <bb 3> : *a.0_11 ={v} {CLOBBER}; operator delete (a.0_11, 8); <bb 4> : _14 = 0; Entry statement of bb3 is the one actually detected as -Wanalyzer-double-free.
(In reply to Benjamin Priour from comment #3) [...snip...] > > > <bb 3> : > *a.0_11 ={v} {CLOBBER}; > operator delete (a.0_11, 8); > [...snip...] > > Entry statement of bb3 is the one actually detected as > -Wanalyzer-double-free. Given the above IPA, we cannot just ignore the assignment statement, as it could really be an injurious statement, not just a pre-deallocation statement at it is now. Consider the code: A* a; ... delete a; a->x = 7; // (1) operator delete (a); (2) On my box, (1) and (2) generated the IPA <bb 4> : a_10->a = 7; operator delete (a_10); Thus, I'd first only consider types where a destructor is provided (by the user or generated). Indeed, when a destructor is supplied for a type, <bb 3> becomes something akin to : struct A { ... ~A() {} } ... <bb 3> : A::~A (a.0_12); operator delete (a.0_12, 8); The warnings stay the same, though it is now more reliable to check for a destructor call, instead any random single assignment. I'm considering adding a new state to sm-malloc, RS_DESTROYED, that would also help flag use after standalone destruction (without a succeeding deallocation).
(In reply to Benjamin Priour from comment #4) > (In reply to Benjamin Priour from comment #3) Here's a link to the reproducer: https://godbolt.org/z/Wa3fqjrTK with the fields renamed to avoid reusing the name "a". > [...snip...] > > > > > > <bb 3> : > > *a.0_11 ={v} {CLOBBER}; > > operator delete (a.0_11, 8); > > > [...snip...] > > > > Entry statement of bb3 is the one actually detected as > > -Wanalyzer-double-free. > > Given the above IPA, we cannot just ignore the assignment statement, as it > could really be an injurious statement, not just a pre-deallocation > statement at it is now. Ths assignment statement: *a.0_11 ={v} {CLOBBER}; is a "clobber", which is a special-case of assignment, generated by the frontends when something is going out of scope, or becoming invalid. We could potentially just special-case such ass > > Consider the code: > A* a; > ... > delete a; > a->x = 7; // (1) > operator delete (a); (2) > > On my box, (1) and (2) generated the IPA > <bb 4> : > a_10->a = 7; > operator delete (a_10); > > Thus, I'd first only consider types where a destructor is provided (by the > user or generated). > Indeed, when a destructor is supplied for a type, <bb 3> becomes something > akin to : > > struct A > { > ... > ~A() {} > } > > ... > > <bb 3> : > A::~A (a.0_12); > operator delete (a.0_12, 8); > > > The warnings stay the same, though it is now more reliable to check for a > destructor call, instead any random single assignment. There's a sense in which it does make sense to complain about use-after-delete in the destructor (when the destructor is non-empty): the memory is being accessed after deletion. Maybe this case would make more sense to the user? (albeit being rather verbose) > I'm considering > adding a new state to sm-malloc, RS_DESTROYED, that would also help flag use > after standalone destruction (without a succeeding deallocation).
(In reply to David Malcolm from comment #5) > (In reply to Benjamin Priour from comment #4) > > (In reply to Benjamin Priour from comment #3) > > Here's a link to the reproducer: > https://godbolt.org/z/Wa3fqjrTK > with the fields renamed to avoid reusing the name "a". > > > > [...snip...] > > > > > > > > > <bb 3> : > > > *a.0_11 ={v} {CLOBBER}; > > > operator delete (a.0_11, 8); > > > > > [...snip...] > > > > > > Entry statement of bb3 is the one actually detected as > > > -Wanalyzer-double-free. > > > > Given the above IPA, we cannot just ignore the assignment statement, as it > > could really be an injurious statement, not just a pre-deallocation > > statement at it is now. > > Ths assignment statement: > *a.0_11 ={v} {CLOBBER}; > is a "clobber", which is a special-case of assignment, generated by the > frontends when something is going out of scope, or becoming invalid. > > We could potentially just special-case such ass > Wouldn't considering "clobber" as a trigger for double-delete also lead to many false positives ? I'm not yet 100% familiar with and when this "clobber" appears. [...snip...] > > > > struct A > > { > > ... > > ~A() {} > > } > > > > ... > > > > <bb 3> : > > A::~A (a.0_12); > > operator delete (a.0_12, 8); > > > > > > The warnings stay the same, though it is now more reliable to check for a > > destructor call, instead any random single assignment. > > There's a sense in which it does make sense to complain about > use-after-delete in the destructor (when the destructor is non-empty): the > memory is being accessed after deletion. Maybe this case would make more > sense to the user? (albeit being rather verbose) I believe in the case of a non-empty destructor, "double-delete" would be more on point than "use-after-delete", as ultimately the issue is the second call to delete. "double-delete" immediately warns about the actual issue, whereas if the first "delete" is not as obvious as in the above test case, a "use-after-delete" might confuse the user. "use-after-delete" could mislead the user to believe there is something wrong with their destructor, although only their double delete is.