public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug analyzer/94355] New: support for C++ new expression
@ 2020-03-27 11:36 vanyacpp at gmail dot com
2020-03-27 11:59 ` [Bug analyzer/94355] " redi at gcc dot gnu.org
` (15 more replies)
0 siblings, 16 replies; 17+ messages in thread
From: vanyacpp at gmail dot com @ 2020-03-27 11:36 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
Bug ID: 94355
Summary: support for C++ new expression
Product: gcc
Version: 10.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: analyzer
Assignee: dmalcolm at gcc dot gnu.org
Reporter: vanyacpp at gmail dot com
Target Milestone: ---
At the moment static analyzer warns about leaked malloc. It would be great if
C++ new expression were also supported.
Example:
void f()
{
char* p = new char;
}
Expected diagnostic:
warning: leak of 'p' [CWE-401] [-Wanalyzer-malloc-leak]
3 | char* p = new char;
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
@ 2020-03-27 11:59 ` redi at gcc dot gnu.org
2020-09-09 21:24 ` cvs-commit at gcc dot gnu.org
` (14 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: redi at gcc dot gnu.org @ 2020-03-27 11:59 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This is already being actively discussed, and will probably be a GSoC project.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
2020-03-27 11:59 ` [Bug analyzer/94355] " redi at gcc dot gnu.org
@ 2020-09-09 21:24 ` cvs-commit at gcc dot gnu.org
2020-09-09 21:44 ` dmalcolm at gcc dot gnu.org
` (13 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2020-09-09 21:24 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #2 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:
https://gcc.gnu.org/g:1690a839cff2e0276017a013419d81d675bbf69d
commit r11-3090-g1690a839cff2e0276017a013419d81d675bbf69d
Author: David Malcolm <dmalcolm@redhat.com>
Date: Fri Aug 28 13:43:56 2020 -0400
analyzer: generalize sm-malloc to new/delete [PR94355]
This patch generalizes the state machine in sm-malloc.c to support
multiple allocator APIs, and adds just enough support for C++ new and
delete to demonstrate the feature, allowing for detection of code
paths where the result of new in C++ can leak - for some crude examples,
at least (bearing in mind that the analyzer doesn't yet know about
e.g. vfuncs, exceptions, inheritance, RTTI, etc)
It also implements a new warning: -Wanalyzer-mismatching-deallocation.
For example:
demo.cc: In function 'void test()':
demo.cc:8:8: warning: 'f' should have been deallocated with 'delete'
but was deallocated with 'free' [CWE-762]
[-Wanalyzer-mismatching-deallocation]
8 | free (f);
| ~~~~~^~~
'void test()': events 1-2
|
| 7 | foo *f = new foo;
| | ^~~
| | |
| | (1) allocated here (expects deallocation with
'delete')
| 8 | free (f);
| | ~~~~~~~~
| | |
| | (2) deallocated with 'free' here; allocation at (1)
expects deallocation with 'delete'
|
The patch also adds just enough knowledge of exception-handling to
suppress a false positive from -Wanalyzer-malloc-leak on
g++.dg/analyzer/pr96723.C on the exception-handling CFG edge after
operator new. It does this by adding a constraint that the result is
NULL if an exception was thrown from operator new, since the result from
operator new is lost when following that exception-handling CFG edge.
gcc/analyzer/ChangeLog:
PR analyzer/94355
* analyzer.opt (Wanalyzer-mismatching-deallocation): New warning.
* region-model-impl-calls.cc
(region_model::impl_call_operator_new): New.
(region_model::impl_call_operator_delete): New.
* region-model.cc (region_model::on_call_pre): Detect operator new
and operator delete.
(region_model::on_call_post): Likewise.
(region_model::maybe_update_for_edge): Detect EH edges and call...
(region_model::apply_constraints_for_exception): New function.
* region-model.h (region_model::impl_call_operator_new): New decl.
(region_model::impl_call_operator_delete): New decl.
(region_model::apply_constraints_for_exception): New decl.
* sm-malloc.cc (enum resource_state): New.
(struct allocation_state): New state subclass.
(enum wording): New.
(struct api): New.
(malloc_state_machine::custom_data_t): New typedef.
(malloc_state_machine::add_state): New decl.
(malloc_state_machine::m_unchecked)
(malloc_state_machine::m_nonnull)
(malloc_state_machine::m_freed): Delete these states in favor
of...
(malloc_state_machine::m_malloc)
(malloc_state_machine::m_scalar_new)
(malloc_state_machine::m_vector_new): ...this new api instances,
which own their own versions of these states.
(malloc_state_machine::on_allocator_call): New decl.
(malloc_state_machine::on_deallocator_call): New decl.
(api::api): New ctor.
(dyn_cast_allocation_state): New.
(as_a_allocation_state): New.
(get_rs): New.
(unchecked_p): New.
(nonnull_p): New.
(freed_p): New.
(malloc_diagnostic::describe_state_change): Use unchecked_p and
nonnull_p.
(class mismatching_deallocation): New.
(double_free::double_free): Add funcname param for initializing
m_funcname.
(double_free::emit): Use m_funcname in warning message rather
than hardcoding "free".
(double_free::describe_state_change): Likewise. Use freed_p.
(double_free::describe_call_with_state): Use freed_p.
(double_free::describe_final_event): Use m_funcname in message
rather than hardcoding "free".
(double_free::m_funcname): New field.
(possible_null::describe_state_change): Use unchecked_p.
(possible_null::describe_return_of_state): Likewise.
(use_after_free::use_after_free): Add param for initializing m_api.
(use_after_free::emit): Use m_api->m_dealloc_funcname in message
rather than hardcoding "free".
(use_after_free::describe_state_change): Use freed_p. Change the
wording of the message based on the API.
(use_after_free::describe_final_event): Use
m_api->m_dealloc_funcname in message rather than hardcoding
"free". Change the wording of the message based on the API.
(use_after_free::m_api): New field.
(malloc_leak::describe_state_change): Use unchecked_p. Update
for renaming of m_malloc_event to m_alloc_event.
(malloc_leak::describe_final_event): Update for renaming of
m_malloc_event to m_alloc_event.
(malloc_leak::m_malloc_event): Rename...
(malloc_leak::m_alloc_event): ...to this.
(free_of_non_heap::free_of_non_heap): Add param for initializing
m_funcname.
(free_of_non_heap::emit): Use m_funcname in message rather than
hardcoding "free".
(free_of_non_heap::describe_final_event): Likewise.
(free_of_non_heap::m_funcname): New field.
(allocation_state::dump_to_pp): New.
(allocation_state::get_nonnull): New.
(malloc_state_machine::malloc_state_machine): Update for changes
to state fields and new api fields.
(malloc_state_machine::add_state): New.
(malloc_state_machine::on_stmt): Move malloc/calloc handling to
on_allocator_call and call it, passing in the API pointer.
Likewise for free, moving it to on_deallocator_call. Handle calls
to operator new and delete in an analogous way. Use unchecked_p
when testing for possibly-null-arg and possibly-null-deref, and
transition to the non-null for the correct API. Remove redundant
node param from call to on_zero_assignment. Use freed_p for
use-after-free check, and pass in API.
(malloc_state_machine::on_allocator_call): New, based on code in
on_stmt.
(malloc_state_machine::on_deallocator_call): Likewise.
(malloc_state_machine::on_phi): Mark node param with
ATTRIBUTE_UNUSED; don't pass it to on_zero_assignment.
(malloc_state_machine::on_condition): Mark node param with
ATTRIBUTE_UNUSED. Replace on_transition calls with get_state and
set_next_state pairs, transitioning to the non-null state for the
appropriate API.
(malloc_state_machine::can_purge_p): Port to new state approach.
(malloc_state_machine::on_zero_assignment): Replace on_transition
calls with get_state and set_next_state pairs. Drop redundant
node param.
* sm.h (state_machine::add_custom_state): New.
gcc/ChangeLog:
PR analyzer/94355
* doc/invoke.texi: Document -Wanalyzer-mismatching-deallocation.
gcc/testsuite/ChangeLog:
PR analyzer/94355
* g++.dg/analyzer/new-1.C: New test.
* g++.dg/analyzer/new-vs-malloc.C: New test.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
2020-03-27 11:59 ` [Bug analyzer/94355] " redi at gcc dot gnu.org
2020-09-09 21:24 ` cvs-commit at gcc dot gnu.org
@ 2020-09-09 21:44 ` dmalcolm at gcc dot gnu.org
2020-09-09 21:50 ` dmalcolm at gcc dot gnu.org
` (12 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2020-09-09 21:44 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
David Malcolm <dmalcolm at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |ASSIGNED
Ever confirmed|0 |1
Last reconfirmed| |2020-09-09
--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
The above patch is an initial implementation of new/delete support; it works
for the simplest cases.
As noted in the commit message this patch was mostly about generalizing the
sm-malloc.cc code; the implementation of new/delete will no doubt need much
further refinement (and beyond that, the analyzer doesn't yet know about
vfuncs, exceptions, inheritance, RTTI, etc)
Specific things still to be investigated relating to new/delete (if only to add
test coverage):
* all of the various cases seen at
https://en.cppreference.com/w/cpp/memory/new/operator_new
* placement new
* global and class-specific replacements for operator new/new[]
* exceptions vs non-throwing allocators vs -fno-exceptions
* verify that ctors/dtors are simulated appropriately for both scalar and
vector new/delete
Anything else?
Note to self: see also https://en.cppreference.com/w/cpp/language/new
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (2 preceding siblings ...)
2020-09-09 21:44 ` dmalcolm at gcc dot gnu.org
@ 2020-09-09 21:50 ` dmalcolm at gcc dot gnu.org
2020-09-26 1:35 ` cvs-commit at gcc dot gnu.org
` (11 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2020-09-09 21:50 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to Ivan Sorokin from comment #0)
> At the moment static analyzer warns about leaked malloc. It would be great
> if C++ new expression were also supported.
>
> Example:
>
> void f()
> {
> char* p = new char;
> }
>
> Expected diagnostic:
>
> warning: leak of 'p' [CWE-401] [-Wanalyzer-malloc-leak]
>
> 3 | char* p = new char;
Actual diagnostic with commit
r11-3090-g1690a839cff2e0276017a013419d81d675bbf69d:
demo.cc: In function ‘void f()’:
demo.cc:4:1: warning: leak of ‘p’ [CWE-401] [-Wanalyzer-malloc-leak]
4 | }
| ^
‘void f()’: events 1-2
|
| 3 | char* p = new char;
| | ^~~~
| | |
| | (1) allocated here
| 4 | }
| | ~
| | |
| | (2) ‘p’ leaks here; was allocated at (1)
|
where the diagnostic's location is at the point where "p" goes out of scope.
There's an argument that it would be better emitting it at the "new".
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (3 preceding siblings ...)
2020-09-09 21:50 ` dmalcolm at gcc dot gnu.org
@ 2020-09-26 1:35 ` cvs-commit at gcc dot gnu.org
2021-04-12 7:46 ` vanyacpp at gmail dot com
` (10 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2020-09-26 1:35 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:
https://gcc.gnu.org/g:d4a906e7b51f3fc31f3328810f45ae4cf2e7bbc3
commit r11-3472-gd4a906e7b51f3fc31f3328810f45ae4cf2e7bbc3
Author: David Malcolm <dmalcolm@redhat.com>
Date: Fri Sep 25 14:31:46 2020 -0400
analyzer: add test for placement new
gcc/testsuite/ChangeLog:
PR analyzer/94355
* g++.dg/analyzer/placement-new.C: New test.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (4 preceding siblings ...)
2020-09-26 1:35 ` cvs-commit at gcc dot gnu.org
@ 2021-04-12 7:46 ` vanyacpp at gmail dot com
2021-04-12 8:04 ` vanyacpp at gmail dot com
` (9 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: vanyacpp at gmail dot com @ 2021-04-12 7:46 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #6 from Ivan Sorokin <vanyacpp at gmail dot com> ---
I played with -fanalyzer on godbolt (GCC trunk). I noticed that -fanalyzer
doesn't report double free in this (convoluted) case:
#include <cstdlib>
int main()
{
int* p = new int;
delete p;
free(p);
}
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (5 preceding siblings ...)
2021-04-12 7:46 ` vanyacpp at gmail dot com
@ 2021-04-12 8:04 ` vanyacpp at gmail dot com
2021-07-22 21:14 ` navarre.gcc.bugs at gmail dot com
` (8 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: vanyacpp at gmail dot com @ 2021-04-12 8:04 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #7 from Ivan Sorokin <vanyacpp at gmail dot com> ---
For me the support for operator new works well for trivially constructible
types. For a non-trivially constructible type I got a false positive:
struct foo { foo(); };
int main()
{
delete new foo();
}
In function 'int main()':
cc1plus: warning: use of possibly-NULL 'operator new(1)' where non-null
expected [CWE-690] [-Wanalyzer-possible-null-argument]
'int main()': event 1
|
|<source>:5:20:
| 5 | delete new foo();
| | ^
| | |
| | (1) this call could return NULL
|
'int main()': event 2
|
|cc1plus:
| (2): argument 'this' ('operator new(1)') from (1) could be NULL where
non-null expected
|
<source>:1:14: note: argument 'this' of 'foo::foo()' must be non-null
1 | struct foo { foo(); };
| ^~~
Compiler returned: 0
https://godbolt.org/z/nPff9EGsY
Also the error location seems to be wrong. Removing "()" from "delete new
foo()" fixes the error location.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (6 preceding siblings ...)
2021-04-12 8:04 ` vanyacpp at gmail dot com
@ 2021-07-22 21:14 ` navarre.gcc.bugs at gmail dot com
2021-07-22 21:35 ` redi at gcc dot gnu.org
` (7 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: navarre.gcc.bugs at gmail dot com @ 2021-07-22 21:14 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
William Navarre <navarre.gcc.bugs at gmail dot com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |navarre.gcc.bugs at gmail dot com
--- Comment #8 from William Navarre <navarre.gcc.bugs at gmail dot com> ---
It seems that `operator new` is generally not supposed to return NULL --
std::bad_alloc() is supposed to be thrown instead.
I made that change on my build (see below). I think that treating new's result
as never-null is probably the correct thing to do most of the time, but two
considerations:
1) The case of allocating a zero-length array.
2) The case that a project has replaced `operator new.` (See "global
replacements" at https://en.cppreference.com/w/cpp/memory/new/operator_new).
Apparently projects can replace `operator new` (see "global replacements" at
https://en.cppreference.com/w/cpp/memory/new/operator_new). It's not clear
--- a/gcc-11.1.0/gcc/analyzer/sm-malloc.cc
+++ b/gcc-11.1.0.navarre/gcc/analyzer/sm-malloc.cc
@@ -1527,9 +1527,9 @@ malloc_state_machine::on_stmt (sm_context *sm_ctxt,
}
if (is_named_call_p (callee_fndecl, "operator new", call, 1))
- on_allocator_call (sm_ctxt, call, &m_scalar_delete);
+ on_allocator_call (sm_ctxt, call, &m_scalar_delete, true);
else if (is_named_call_p (callee_fndecl, "operator new []", call, 1))
- on_allocator_call (sm_ctxt, call, &m_vector_delete);
+ on_allocator_call (sm_ctxt, call, &m_vector_delete, true);
else if (is_named_call_p (callee_fndecl, "operator delete", call, 1)
|| is_named_call_p (callee_fndecl, "operator delete", call,
2))
{
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (7 preceding siblings ...)
2021-07-22 21:14 ` navarre.gcc.bugs at gmail dot com
@ 2021-07-22 21:35 ` redi at gcc dot gnu.org
2022-11-05 9:21 ` redi at gcc dot gnu.org
` (6 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: redi at gcc dot gnu.org @ 2021-07-22 21:35 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to William Navarre from comment #8)
> It seems that `operator new` is generally not supposed to return NULL --
> std::bad_alloc() is supposed to be thrown instead.
If an operator new overload is declared noexcept, then it can return null on
failure. If it is not noexcept then it throws bad_alloc (or something derived
from it) and must never return null.
>
> I made that change on my build (see below). I think that treating new's
> result as never-null is probably the correct thing to do most of the time,
> but two considerations:
>
> 1) The case of allocating a zero-length array.
That still can't return null. It must return a valid non-null pointer, that
cannot be derefernced.
> 2) The case that a project has replaced `operator new.` (See "global
> replacements" at https://en.cppreference.com/w/cpp/memory/new/operator_new).
>
> Apparently projects can replace `operator new` (see "global replacements" at
> https://en.cppreference.com/w/cpp/memory/new/operator_new). It's not clear
Exactly the same rules apply. If the operator new function is noexcept it
returns null to indicate allocation failure, otherwise it must throw and cannot
return null, ever.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (8 preceding siblings ...)
2021-07-22 21:35 ` redi at gcc dot gnu.org
@ 2022-11-05 9:21 ` redi at gcc dot gnu.org
2022-11-05 9:28 ` redi at gcc dot gnu.org
` (5 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: redi at gcc dot gnu.org @ 2022-11-05 9:21 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
struct S {
S() { p = new int(); }
~S() { delete p; }
int* p = nullptr;
};
int main() {
S s;
}
Everything the analyzer says is wrong :-)
dest.C: In function ‘int main()’:
dest.C:9:1: warning: leak of ‘s.S::p’ [CWE-401] [-Wanalyzer-malloc-leak]
9 | }
| ^
‘int main()’: events 1-2
|
| 7 | int main() {
| | ^~~~
| | |
| | (1) entry to ‘main’
| 8 | S s;
| | ~
| | |
| | (2) calling ‘S::S’ from ‘main’
|
+--> ‘S::S()’: events 3-5
|
| 2 | S() { p = new int(); }
| | ^ ~~~~~~~~~~~~~
| | | | |
| | | | (4) allocated here
| | | (5) assuming ‘operator new(4)’ is non-NULL
| | (3) entry to ‘S::S’
|
<------+
|
‘int main()’: events 6-7
|
| 8 | S s;
| | ^
| | |
| | (6) returning to ‘main’ from ‘S::S’
| 9 | }
| | ~
| | |
| | (7) ‘s.S::p’ leaks here; was allocated at (4)
|
dest.C: In constructor ‘S::S()’:
dest.C:2:13: warning: dereference of possibly-NULL ‘operator new(4)’ [CWE-690]
[-Wanalyzer-possible-null-dereference]
2 | S() { p = new int(); }
| ~~^~~~~~~~~~~
‘S::S()’: events 1-2
|
| 2 | S() { p = new int(); }
| | ~~~~~~~~~~~~^
| | | |
| | | (1) this call could return NULL
| | (2) ‘operator new(4)’ could be NULL: unchecked value
from (1)
|
As already noted above, new can't return null here, and there is no dereference
anyway. And the pointer isn't leaked, but it seems maybe the analyzer doesn't
know about destructors?
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (9 preceding siblings ...)
2022-11-05 9:21 ` redi at gcc dot gnu.org
@ 2022-11-05 9:28 ` redi at gcc dot gnu.org
2022-11-05 9:59 ` redi at gcc dot gnu.org
` (4 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: redi at gcc dot gnu.org @ 2022-11-05 9:28 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
If we add a real bug (making a shallow copy of the object, which shares the
same heap pointer and then both destructors free the same one):
struct S {
S() { p = new int(); }
~S() { delete p; }
int* p = nullptr;
};
int main() {
S s;
S ss = s;
}
Then the analyzer still tells us the pointer is leaked:
dest.C: In function ‘int main()’:
dest.C:10:1: warning: leak of ‘s.S::p’ [CWE-401] [-Wanalyzer-malloc-leak]
10 | }
| ^
‘int main()’: events 1-2
|
| 7 | int main() {
| | ^~~~
| | |
| | (1) entry to ‘main’
| 8 | S s;
| | ~
| | |
| | (2) calling ‘S::S’ from ‘main’
|
+--> ‘S::S()’: events 3-5
|
| 2 | S() { p = new int(); }
| | ^ ~~~~~~~~~~~~~
| | | | |
| | | | (4) allocated here
| | | (5) assuming ‘operator new(4)’ is non-NULL
| | (3) entry to ‘S::S’
|
<------+
|
‘int main()’: events 6-7
|
| 8 | S s;
| | ^
| | |
| | (6) returning to ‘main’ from ‘S::S’
| 9 | S ss = s;
| 10 | }
| | ~
| | |
| | (7) ‘s.S::p’ leaks here; was allocated at (4)
|
But strangely it also tells use there's a use-after-free, which is odd if it
thinks the pointer was nver freed:
dest.C: In destructor ‘S::~S()’:
dest.C:3:22: warning: use after ‘delete’ of ‘s.S::p’ [CWE-416]
[-Wanalyzer-use-after-free]
3 | ~S() { delete p; }
| ^
‘int main()’: events 1-2
|
| 7 | int main() {
| | ^~~~
| | |
| | (1) entry to ‘main’
| 8 | S s;
| | ~
| | |
| | (2) calling ‘S::S’ from ‘main’
|
+--> ‘S::S()’: events 3-5
|
| 2 | S() { p = new int(); }
| | ^ ~~~~~~~~~~~~~
| | | | |
| | | | (4) allocated here
| | | (5) assuming ‘operator new(4)’ is non-NULL
| | (3) entry to ‘S::S’
|
<------+
|
‘int main()’: events 6-7
|
| 8 | S s;
| | ^
| | |
| | (6) returning to ‘main’ from ‘S::S’
| 9 | S ss = s;
| 10 | }
| | ~
| | |
| | (7) calling ‘S::~S’ from ‘main’
|
+--> ‘S::~S()’: events 8-11
|
| 3 | ~S() { delete p; }
| | ^ ~~~~~~~~
| | | | |
| | | | (10) ...to here
| | | | (11) deleted here
| | | (9) following ‘true’ branch...
| | (8) entry to ‘S::~S’
|
<------+
|
‘int main()’: events 12-13
|
| 10 | }
| | ^
| | |
| | (12) returning to ‘main’ from ‘S::~S’
| | (13) calling ‘S::~S’ from ‘main’
|
+--> ‘S::~S()’: events 14-17
|
| 3 | ~S() { delete p; }
| | ^ ~~~~~~~~ ~
| | | | | |
| | | | | (17) use after ‘delete’ of
‘*this.S::p’; deleted at (11)
| | | | (16) ...to here
| | | (15) following ‘true’ branch...
| | (14) entry to ‘S::~S’
|
I think it's (correctly) telling us that the destructor ss.~S() uses the value
after s.~S() already freed it. But it would be nice if it mentioned that the
second destructor is on ss, not s.
The earlier warning tells us the object: "warning: use after ‘delete’ of
‘s.S::p’"
So it would be nice if the use-after-free was identified as being ss.S::p
(which has the same value as s.S::p).
And it's not actually a use-after-free (and certainly not at the closing brace
of the destructor where the location points to), it's a double-free.
Ideally it would have complained when we copied the pointer value in the
trivial copy constructor and didn't zero out the original. That shallow copy
was the actual bug, the double-free is a symptom of it.
dest.C: In constructor ‘S::S()’:
dest.C:2:13: warning: dereference of possibly-NULL ‘operator new(4)’ [CWE-690]
[-Wanalyzer-possible-null-dereference]
2 | S() { p = new int(); }
| ~~^~~~~~~~~~~
‘S::S()’: events 1-2
|
| 2 | S() { p = new int(); }
| | ~~~~~~~~~~~~^
| | | |
| | | (1) this call could return NULL
| | (2) ‘operator new(4)’ could be NULL: unchecked value
from (1)
|
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (10 preceding siblings ...)
2022-11-05 9:28 ` redi at gcc dot gnu.org
@ 2022-11-05 9:59 ` redi at gcc dot gnu.org
2022-11-05 16:44 ` dmalcolm at gcc dot gnu.org
` (3 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: redi at gcc dot gnu.org @ 2022-11-05 9:59 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #12 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #11)
> Ideally it would have complained when we copied the pointer value in the
> trivial copy constructor and didn't zero out the original. That shallow copy
> was the actual bug, the double-free is a symptom of it.
I've created PR 107534 for that.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (11 preceding siblings ...)
2022-11-05 9:59 ` redi at gcc dot gnu.org
@ 2022-11-05 16:44 ` dmalcolm at gcc dot gnu.org
2022-11-05 21:31 ` redi at gcc dot gnu.org
` (2 subsequent siblings)
15 siblings, 0 replies; 17+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2022-11-05 16:44 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #13 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #10)
[...snip...]
> As already noted above, new can't return null here, and there is no
> dereference anyway. And the pointer isn't leaked, but it seems maybe the
> analyzer doesn't know about destructors?
FWIW the analyzer (presumably incorrectly) considers the case where operator
new returns NULL, and then attempts to write 0 to it.
The leak is due to the analyzer (presumably incorrectly) following the EH cfg
edge from BB 2 to BB 5 below.
int main ()
{
struct S s;
int D.2800;
int _5;
<bb 2> :
S::S (&s);
<bb 3> :
S::~S (&s);
s ={v} {CLOBBER(eol)};
_5 = 0;
<bb 4> :
<L0>:
return _5;
<bb 5> :
<L1>:
s ={v} {CLOBBER(eol)};
resx 1
}
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (12 preceding siblings ...)
2022-11-05 16:44 ` dmalcolm at gcc dot gnu.org
@ 2022-11-05 21:31 ` redi at gcc dot gnu.org
2023-06-29 20:01 ` [Bug analyzer/94355] analyzer " vultkayn at gcc dot gnu.org
2023-09-01 20:06 ` cvs-commit at gcc dot gnu.org
15 siblings, 0 replies; 17+ messages in thread
From: redi at gcc dot gnu.org @ 2022-11-05 21:31 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to David Malcolm from comment #13)
> (In reply to Jonathan Wakely from comment #10)
>
> [...snip...]
>
> > As already noted above, new can't return null here, and there is no
> > dereference anyway. And the pointer isn't leaked, but it seems maybe the
> > analyzer doesn't know about destructors?
>
> FWIW the analyzer (presumably incorrectly) considers the case where operator
> new returns NULL, and then attempts to write 0 to it.
Yeah, that's incorrect, the write is guaranteed to be OK because if new fails
it throws an exception before the write ever happens.
If a non-throwing form of new is used, e.g. p = new(std::nothrow) int(), then
the compiler *does* insert a null check before writing to it. So in the case
where a null check is necessary, the compiler inserts one.
See the -fcheck-new option:
Check that the pointer returned by "operator new" is non-null before
attempting to modify the storage allocated. This check is normally unnecessary
because the C++ standard specifies that "operator new" only returns 0 if it is
declared "throw()", in which case the compiler always checks the return value
even without this option. In all other cases, when "operator new" has a
non-empty exception specification, memory exhaustion is signalled by throwing
"std::bad_alloc". See also new (nothrow).
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] analyzer support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (13 preceding siblings ...)
2022-11-05 21:31 ` redi at gcc dot gnu.org
@ 2023-06-29 20:01 ` vultkayn at gcc dot gnu.org
2023-09-01 20:06 ` cvs-commit at gcc dot gnu.org
15 siblings, 0 replies; 17+ messages in thread
From: vultkayn at gcc dot gnu.org @ 2023-06-29 20:01 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #15 from Benjamin Priour <vultkayn at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #14)
> [...snip...]
>
> See the -fcheck-new option:
>
> Check that the pointer returned by "operator new" is non-null before
> attempting to modify the storage allocated. This check is normally
> unnecessary because the C++ standard specifies that "operator new" only
> returns 0 if it is declared "throw()", in which case the compiler always
> checks the return value even without this option. In all other cases, when
> "operator new" has a non-empty exception specification, memory exhaustion is
> signalled by throwing "std::bad_alloc". See also new (nothrow).
Should we use the above flag's value to also optionally disable the analyzer
warnings on operator new possibly returning NULL?
Or maybe there could be an additional flag -fanalyzer-new-returns-null, turned
'on' by default.
Having such capability would be useful to run the analyzer against itself, as
GCC is built without exceptions (thus every operator new possibly returns
NULL).
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Bug analyzer/94355] analyzer support for C++ new expression
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
` (14 preceding siblings ...)
2023-06-29 20:01 ` [Bug analyzer/94355] analyzer " vultkayn at gcc dot gnu.org
@ 2023-09-01 20:06 ` cvs-commit at gcc dot gnu.org
15 siblings, 0 replies; 17+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2023-09-01 20:06 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94355
--- Comment #16 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Benjamin Priour <vultkayn@gcc.gnu.org>:
https://gcc.gnu.org/g:e7b267444045c507654a2a3f758efee5d5b550df
commit r14-3632-ge7b267444045c507654a2a3f758efee5d5b550df
Author: benjamin priour <priour.be@gmail.com>
Date: Fri Sep 1 00:01:29 2023 +0200
analyzer: Add support of placement new and improved operator new
[PR105948,PR94355]
Fixed spurious possibly-NULL warning always tagging along throwing
operator new despite it never returning NULL.
Now operator new is correctly recognized as possibly returning NULL
if and only if it is non-throwing or exceptions have been disabled.
Different standard signatures of operator new are now properly
recognized.
Added support of placement new, so that it is now properly recognized,
and a 'heap_allocated' region is no longer created for it.
Placement new size is also checked and a 'Wanalyzer-allocation-size'
is emitted when relevant, as well as always a 'Wanalyzer-out-of-bounds'.
'operator new' non-throwing variants are detected y checking the types
of the parameters.
Indeed, in a call to new (std::nothrow) () the chosen overload
has signature 'operator new (void*, std::nothrow_t&)', where the second
parameter is a reference. In a placement new, the second parameter will
always be a void pointer.
Prior to this patch, some buffers first allocated with 'new', then deleted
an thereafter used would result in a 'Wanalyzer-user-after-free'
warning. However the wording was "use after 'free'" instead of the
expected "use after 'delete'".
This patch fixes this by introducing a new kind of poisoned value,
namely POISON_KIND_DELETED.
Due to how the analyzer sees calls to non-throwing variants of
operator new, dereferencing a pointer freshly allocated in this fashion
caused both a 'Wanalyzer-use-of-uninitialized-value' and a
'Wanalyzer-null-dereference' to be emitted, while only the latter was
relevant. As a result, 'null-dereference' now supersedes
'use-of-uninitialized'.
Signed-off-by: benjamin priour <vultkayn@gcc.gnu.org>
gcc/analyzer/ChangeLog:
PR analyzer/105948
PR analyzer/94355
* analyzer.h (is_placement_new_p): New declaration.
* call-details.cc
(call_details::deref_ptr_arg): New function.
Dereference the argument at given index if possible.
* call-details.h: Declaration of the above function.
* kf-lang-cp.cc (is_placement_new_p): Returns true if the gcall
is recognized as a placement new.
(kf_operator_delete::impl_call_post): Unbinding a region and its
descendents now poisons with POISON_KIND_DELETED.
(register_known_functions_lang_cp): Known function "operator
delete" is now registered only once independently of its number of
arguments.
* region-model.cc (region_model::eval_condition): Now
recursively calls itself if any of the operand is wrapped in a
cast.
* sm-malloc.cc (malloc_state_machine::on_stmt):
Add placement new recognition.
* svalue.cc (poison_kind_to_str): Wording for the new PK.
* svalue.h (enum poison_kind): Add value POISON_KIND_DELETED.
gcc/testsuite/ChangeLog:
PR analyzer/105948
PR analyzer/94355
* g++.dg/analyzer/out-of-bounds-placement-new.C: Added a directive.
* g++.dg/analyzer/placement-new.C: Added tests.
* g++.dg/analyzer/new-2.C: New test.
* g++.dg/analyzer/noexcept-new.C: New test.
* g++.dg/analyzer/placement-new-size.C: New test.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-09-01 20:06 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 11:36 [Bug analyzer/94355] New: support for C++ new expression vanyacpp at gmail dot com
2020-03-27 11:59 ` [Bug analyzer/94355] " redi at gcc dot gnu.org
2020-09-09 21:24 ` cvs-commit at gcc dot gnu.org
2020-09-09 21:44 ` dmalcolm at gcc dot gnu.org
2020-09-09 21:50 ` dmalcolm at gcc dot gnu.org
2020-09-26 1:35 ` cvs-commit at gcc dot gnu.org
2021-04-12 7:46 ` vanyacpp at gmail dot com
2021-04-12 8:04 ` vanyacpp at gmail dot com
2021-07-22 21:14 ` navarre.gcc.bugs at gmail dot com
2021-07-22 21:35 ` redi at gcc dot gnu.org
2022-11-05 9:21 ` redi at gcc dot gnu.org
2022-11-05 9:28 ` redi at gcc dot gnu.org
2022-11-05 9:59 ` redi at gcc dot gnu.org
2022-11-05 16:44 ` dmalcolm at gcc dot gnu.org
2022-11-05 21:31 ` redi at gcc dot gnu.org
2023-06-29 20:01 ` [Bug analyzer/94355] analyzer " vultkayn at gcc dot gnu.org
2023-09-01 20:06 ` cvs-commit at gcc dot gnu.org
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).