[Bug analyzer/109365] New: Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free

[Bug analyzer/109365] New: Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free

[priour.be at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109365

            Bug ID: 109365
           Summary: Double delete yields -Wanalyzer-use-after-free instead
                    of -Wanalyzer-double-free
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: priour.be at gmail dot com
  Target Milestone: ---
            Target: x86_64-pc-linux-gnu
             Build: 13.0.1 20230328 (experimental)

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.)

[Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109365

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(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

[Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free

[priour.be at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109365

Benjamin Priour <priour.be at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |priour.be at gmail dot com

--- Comment #2 from Benjamin Priour <priour.be at gmail dot com> ---
(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.

[Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free

[vultkayn at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109365

Benjamin Priour <vultkayn at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vultkayn at gcc dot gnu.org

--- Comment #3 from Benjamin Priour <vultkayn at gcc dot gnu.org> ---
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.

[Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free

[vultkayn at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109365

Benjamin Priour <vultkayn at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|dmalcolm at gcc dot gnu.org        |vultkayn at gcc dot gnu.org

--- Comment #4 from Benjamin Priour <vultkayn at gcc dot gnu.org> ---
(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).

[Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109365

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dmalcolm at gcc dot gnu.org

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(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).

[Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free

[vultkayn at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109365

--- Comment #6 from Benjamin Priour <vultkayn at gcc dot gnu.org> ---
(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.