[GSoC] A bunch of questions about sm-malloc behavior, and proposition of a GSoC subject.

[GSoC] A bunch of questions about sm-malloc behavior, and proposition of a GSoC subject.

[Benjamin Priour]

[-- Attachment #1: Type: text/plain, Size: 4404 bytes --]

Hi David,

I've been playing around with sm-malloc on C++ samples.
I've noticed double delete don't go under -Wanalyzer-double-free but within
-Wanalyzer-use-after-free scope.

With the reproducer ...
> struct A {};
>
> int main() {
>     A* a = new A();
>     delete a;
>     delete a;
>     return 0;
> }

... the analyzer diagnoses:
build/gcc/xg++ -Bbuild/gcc -S -fanalyzer
-Wanalyzer-mismatching-deallocation -Wanalyzer-use-after-free
-Wanalyzer-double-free -Wanalyzer-malloc-leak  double_delete_test.cpp
double_delete_test.cpp: In function ‘int main()’:
double_delete_test.cpp:7:1: warning: use after ‘delete’ of ‘a’ [CWE-416]
[-Wanalyzer-use-after-free]
    7 | }
      | ^
  ‘int main()’: events 1-8
    |
    |    3 |     A* a = new A();
    |      |                  ^
    |      |                  |
    |      |                  (1) allocated here
    |      |                  (2) assuming ‘operator new(8)’ is non-NULL
    |    4 |     delete a;
    |      |     ~~~~~~~~
    |      |     |      |
    |      |     |      (4) ...to here
    |      |     |      (5) deleted here
    |      |     (3) following ‘true’ branch...
    |    5 |     delete a;
    |      |     ~~~~~~~~
    |      |     |      |
    |      |     |      (7) ...to here
    |      |     (6) following ‘true’ branch...
    |    6 |     return 0;
    |    7 | }
    |      | ~
    |      | |
    |      | (8) use after ‘delete’ of ‘a’; deleted at (5)
    |
double_delete_test.cpp:3:18: warning: dereference of possibly-NULL
‘operator new(8)’ [CWE-690] [-Wanalyzer-possible-null-dereference]
    3 |     A* a = new A();
      |                  ^
  ‘int main()’: events 1-2
    |
    |    3 |     A* a = new A();
    |      |                  ^
    |      |                  |
    |      |                  (1) this call could return NULL
    |      |                  (2) ‘operator new(8)’ could be NULL:
unchecked value from (1)
    |


Debugging read out two things:
- During second call of 'on_deallocator_call' on delete, the state would be
'stop' instead of the expected 'freed'
- The call to set_next_state transitions malloc instead of delete from
'nonnull' to 'freed'.
I'm still trying to come up with why these two behaviors happens.

By the way, in the first call to 'on_deallocator_call' on delete, the state
is set to 'nonnull',
which conforms to C++ behavior for new. However, a
-Wanalyzer-possible-null-dereference is emitted at the expression 'new'.
I haven't yet figured out why, but I'm looking into it.


Another observation was in the distinction between delete and free in the
case of mixing them.
With 'a' being allocated with malloc:
> A* a = (A*) malloc(sizeof(A));
> free(a);
> delete a; // -Wanalyzer-use-after-free, OK, might expect warning for
double free though ?

but with allocation through new and inversion of free/delete
> A* a = new A();
> delete a;
> free(a); // No diagnostics at all from the analyzer, got
-Wmismatched-new-delete from the front-end though.

I believe this might come from a similar issue as above, but I still have
to investigate on that front.

I just noticed another unexpected behavior. Let's consider
> struct A {int x; int y;};
> void* foo() { A* a = (A*) __builtin_malloc(sizeof(A)); return a; }
> int main() {
>     A* a2 = (A*) __builtin_malloc(sizeof(A));
>     foo();
>     return 0;
> }

Then a -Wanalyzer-malloc-leak is correctly ensued for allocation in foo(),
but none is emitted for the leak in main(), although I checked the exploded
graph it is correctly marked as unchecked(free).

Should I file those on bugzilla ?


Also I have taken interest in PR106388 - Support for use-after-move in
-fanalyzer -.
The prerequisite PR106386 - Reuse libstdc++ assertions - would also be of
great help in extending the support of -Wanalyzer-out-of-bounds,
as we could detect out-of-bounds on vectors through
__glibcxx_requires_subscript used in the definition of operator[].

I already thought about a few ideas to implement that, but I'll develop
them more and try to come up with some proof of concept before sending them
to you.
Hopefully by tomorrow I'll update on this, I'll preferably hop to bed now
haha.
Do you think this could make a suitable GSoC subject ?


Best,
Benjamin.

Re: [GSoC] A bunch of questions about sm-malloc behavior, and proposition of a GSoC subject.

[David Malcolm]

On Thu, 2023-03-30 at 00:50 +0200, Benjamin Priour wrote:
> Hi David,
> 
> I've been playing around with sm-malloc on C++ samples.

Note that the analyzer doesn't properly work yet on C++; see:
  https://gcc.gnu.org/bugzilla/showdependencytree.cgi?id=97110
I'm hoping to address much of this in GCC 14.

Other notes inline below...

> I've noticed double delete don't go under -Wanalyzer-double-free but
> within
> -Wanalyzer-use-after-free scope.
> 
> With the reproducer ...
> > struct A {};
> > 
> > int main() {
> >     A* a = new A();
> >     delete a;
> >     delete a;
> >     return 0;
> > }
> 
> ... the analyzer diagnoses:
> build/gcc/xg++ -Bbuild/gcc -S -fanalyzer
> -Wanalyzer-mismatching-deallocation -Wanalyzer-use-after-free
> -Wanalyzer-double-free -Wanalyzer-malloc-leak  double_delete_test.cpp
> double_delete_test.cpp: In function ‘int main()’:
> double_delete_test.cpp:7:1: warning: use after ‘delete’ of ‘a’ [CWE-
> 416]
> [-Wanalyzer-use-after-free]
>     7 | }
>       | ^
>   ‘int main()’: events 1-8
>     |
>     |    3 |     A* a = new A();
>     |      |                  ^
>     |      |                  |
>     |      |                  (1) allocated here
>     |      |                  (2) assuming ‘operator new(8)’ is non-
> NULL
>     |    4 |     delete a;
>     |      |     ~~~~~~~~
>     |      |     |      |
>     |      |     |      (4) ...to here
>     |      |     |      (5) deleted here
>     |      |     (3) following ‘true’ branch...
>     |    5 |     delete a;
>     |      |     ~~~~~~~~
>     |      |     |      |
>     |      |     |      (7) ...to here
>     |      |     (6) following ‘true’ branch...
>     |    6 |     return 0;
>     |    7 | }
>     |      | ~
>     |      | |
>     |      | (8) use after ‘delete’ of ‘a’; deleted at (5)
>     |
> double_delete_test.cpp:3:18: warning: dereference of possibly-NULL
> ‘operator new(8)’ [CWE-690] [-Wanalyzer-possible-null-dereference]
>     3 |     A* a = new A();
>       |                  ^
>   ‘int main()’: events 1-2
>     |
>     |    3 |     A* a = new A();
>     |      |                  ^
>     |      |                  |
>     |      |                  (1) this call could return NULL
>     |      |                  (2) ‘operator new(8)’ could be NULL:
> unchecked value from (1)
>     |
> 
> 
> Debugging read out two things:
> - During second call of 'on_deallocator_call' on delete, the state
> would be
> 'stop' instead of the expected 'freed'
> - The call to set_next_state transitions malloc instead of delete
> from
> 'nonnull' to 'freed'.
> I'm still trying to come up with why these two behaviors happens.
> 
> By the way, in the first call to 'on_deallocator_call' on delete, the
> state
> is set to 'nonnull',
> which conforms to C++ behavior for new. However, a
> -Wanalyzer-possible-null-dereference is emitted at the expression
> 'new'.
> I haven't yet figured out why, but I'm looking into it.

I'm guessing that the CFG and thus the supergraph contains edges for
handling exceptions, and the analyzer currently doesn't know anything
about these, and mishandled them:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97111

You might want to have a look at the supergraph (via -fdump-analyzer-
supergraph); I recently added more debugging notes here:
 https://gcc.gnu.org/onlinedocs/gccint/Debugging-the-Analyzer.html

If that's the case, then -fno-exceptions might affect the behavior.

> 
> 
> Another observation was in the distinction between delete and free in
> the
> case of mixing them.
> With 'a' being allocated with malloc:
> > A* a = (A*) malloc(sizeof(A));
> > free(a);
> > delete a; // -Wanalyzer-use-after-free, OK, might expect warning
> > for
> double free though ?
> 
> but with allocation through new and inversion of free/delete
> > A* a = new A();
> > delete a;
> > free(a); // No diagnostics at all from the analyzer, got
> -Wmismatched-new-delete from the front-end though.
> 
> I believe this might come from a similar issue as above, but I still
> have
> to investigate on that front.
> 
> I just noticed another unexpected behavior. Let's consider
> > struct A {int x; int y;};
> > void* foo() { A* a = (A*) __builtin_malloc(sizeof(A)); return a; }
> > int main() {
> >     A* a2 = (A*) __builtin_malloc(sizeof(A));
> >     foo();
> >     return 0;
> > }
> 
> Then a -Wanalyzer-malloc-leak is correctly ensued for allocation in
> foo(),
> but none is emitted for the leak in main(), although I checked the
> exploded
> graph it is correctly marked as unchecked(free).
> 
> Should I file those on bugzilla ?

We already know that "C++ doesn't work yet".  Minimal examples of
problems with C++ support might be worth filing, to isolate specific
issues.  If you do, please add them to the tracker bug (by adding
"analyzer-c++" to the "Blocks" field of the new bug(s))

> 
> 
> Also I have taken interest in PR106388 - Support for use-after-move
> in
> -fanalyzer -.
> The prerequisite PR106386 - Reuse libstdc++ assertions - would also
> be of
> great help in extending the support of -Wanalyzer-out-of-bounds,
> as we could detect out-of-bounds on vectors through
> __glibcxx_requires_subscript used in the definition of operator[].

There's a bunch of other C++-enablement work (as per the tracker bug
above) that I think would need fixing before PR106388 is reasonable to
tackle.

> 
> I already thought about a few ideas to implement that, but I'll
> develop
> them more and try to come up with some proof of concept before
> sending them
> to you.
> Hopefully by tomorrow I'll update on this, I'll preferably hop to bed
> now
> haha.
> Do you think this could make a suitable GSoC subject ?

I think working on the C++ enablement prerequisites in the analyzer
would make more sense.  I'd planned to do this myself for GCC 14, but
there are plenty of other tasks I could work on if you want to tackle
C++ support as a GSoC project for GCC 14.

A good C++ project might be: enable enough C++ support in the analyzer
for it to be able to analyze itself.  This could be quite a large,
difficult project, though it sidesteps having to support exception-
handling, since we build ourselves with exception-handling disabled.

Hope this is helpful
Dave