* [Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free
2023-03-31 20:31 [Bug analyzer/109365] New: Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free priour.be at gmail dot com
@ 2023-03-31 20:34 ` dmalcolm at gcc dot gnu.org
2023-03-31 20:37 ` priour.be at gmail dot com
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2023-03-31 20:34 UTC (permalink / raw)
To: gcc-bugs
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free
2023-03-31 20:31 [Bug analyzer/109365] New: Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free priour.be at gmail dot com
2023-03-31 20:34 ` [Bug analyzer/109365] " dmalcolm at gcc dot gnu.org
@ 2023-03-31 20:37 ` priour.be at gmail dot com
2023-07-05 11:31 ` vultkayn at gcc dot gnu.org
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: priour.be at gmail dot com @ 2023-03-31 20:37 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free
2023-03-31 20:31 [Bug analyzer/109365] New: Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free priour.be at gmail dot com
2023-03-31 20:34 ` [Bug analyzer/109365] " dmalcolm at gcc dot gnu.org
2023-03-31 20:37 ` priour.be at gmail dot com
@ 2023-07-05 11:31 ` vultkayn at gcc dot gnu.org
2023-07-07 11:28 ` vultkayn at gcc dot gnu.org
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: vultkayn at gcc dot gnu.org @ 2023-07-05 11:31 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free
2023-03-31 20:31 [Bug analyzer/109365] New: Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free priour.be at gmail dot com
` (2 preceding siblings ...)
2023-07-05 11:31 ` vultkayn at gcc dot gnu.org
@ 2023-07-07 11:28 ` vultkayn at gcc dot gnu.org
2023-07-21 22:22 ` dmalcolm at gcc dot gnu.org
2023-07-26 17:10 ` vultkayn at gcc dot gnu.org
5 siblings, 0 replies; 7+ messages in thread
From: vultkayn at gcc dot gnu.org @ 2023-07-07 11:28 UTC (permalink / raw)
To: gcc-bugs
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).
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free
2023-03-31 20:31 [Bug analyzer/109365] New: Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free priour.be at gmail dot com
` (3 preceding siblings ...)
2023-07-07 11:28 ` vultkayn at gcc dot gnu.org
@ 2023-07-21 22:22 ` dmalcolm at gcc dot gnu.org
2023-07-26 17:10 ` vultkayn at gcc dot gnu.org
5 siblings, 0 replies; 7+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2023-07-21 22:22 UTC (permalink / raw)
To: gcc-bugs
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).
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Bug analyzer/109365] Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free
2023-03-31 20:31 [Bug analyzer/109365] New: Double delete yields -Wanalyzer-use-after-free instead of -Wanalyzer-double-free priour.be at gmail dot com
` (4 preceding siblings ...)
2023-07-21 22:22 ` dmalcolm at gcc dot gnu.org
@ 2023-07-26 17:10 ` vultkayn at gcc dot gnu.org
5 siblings, 0 replies; 7+ messages in thread
From: vultkayn at gcc dot gnu.org @ 2023-07-26 17:10 UTC (permalink / raw)
To: gcc-bugs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread