[PATCH] analyzer: Add support of placement new and improved operator new [PR105948]

[Benjamin Priour]

Hi Dave,

On Fri, Jul 21, 2023 at 10:10 PM David Malcolm <dmalcolm@redhat.com> wrote:
 [...]

It looks like something's gone wrong with the indentation in the above:
> previously we had tab characters, but now I'm seeing a pair of spaces,
> which means this wouldn't line up properly.  This might be a glitch
> somewhere in our email workflow, but please check it in your editor
> (with visual whitespace enabled).
>

I'll double check that before submitting.


> [...snip...]
>
> Some comments on the test cases:
>
> > diff --git a/gcc/testsuite/g++.dg/analyzer/new-2.C
> b/gcc/testsuite/g++.dg/analyzer/new-2.C
> > new file mode 100644
> > index 00000000000..4e696040a54
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/analyzer/new-2.C
> > @@ -0,0 +1,50 @@
> > +// { dg-additional-options "-O0" }
> > +
> > +struct A
> > +{
> > +  int x;
> > +  int y;
> > +};
> > +
> > +void test_spurious_null_warning_throwing ()
> > +{
> > +  int *x = new int; /* { dg-bogus "dereference of possibly-NULL" } */
> > +  int *y = new int (); /* { dg-bogus "dereference of possibly-NULL"
> "non-throwing" } */
> > +  int *arr = new int[3]; /* { dg-bogus "dereference of possibly-NULL" }
> */
> > +  A *a = new A (); /* { dg-bogus "dereference of possibly-NULL"
> "throwing new cannot be null" } */
> > +
> > +  int z = *y + 2;
> > +  z = *x + 4; /* { dg-bogus "dereference of possibly-NULL 'x'" } */
> > +  /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-*
> } .-1 } */
> > +  z = arr[0] + 4; /* { dg-bogus "dereference of possibly-NULL" } */
> > +
> > +  delete a;
> > +  delete y;
> > +  delete x;
> > +  delete[] arr;
> > +}
> > +
> > +void test_default_initialization ()
> > +{
> > +    int *y = ::new int;
> > +    int *x = ::new int (); /* { dg-bogus "dereference of possibly-NULL
> 'operator new" } */
> > +
> > +    int b = *x + 3; /* { dg-bogus "dereference of possibly-NULL" } */
> > +    /* { dg-bogus "use of uninitialized ‘*x’" "" { target *-*-* } .-1 }
> */
> > +    int a = *y + 2; /* { dg-bogus "dereference of possibly-NULL 'y'" }
> */
> > +    /* { dg-warning "use of uninitialized value '\\*y'" "no default
> init" { target *-*-* } .-1 } */
> > +
> > +    delete x;
> > +    delete y;
> > +}
> > +
> > +/* From clang core.uninitialized.NewArraySize
> > +new[] should not be called with an undefined size argument */
> > +
> > +void test_garbage_new_array ()
> > +{
> > +  int n;
> > +  int *arr = ::new int[n]; /* { dg-warning "use of uninitialized value
> 'n'" } */
> > +  arr[0] = 7;
> > +  ::delete[] arr; /* no warnings emitted here either */
> > +}
> > diff --git a/gcc/testsuite/g++.dg/analyzer/noexcept-new.C
> b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C
> > new file mode 100644
> > index 00000000000..7699cd99cff
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/analyzer/noexcept-new.C
> > @@ -0,0 +1,48 @@
> > +/* { dg-additional-options "-O0 -fno-exceptions
> -fno-analyzer-suppress-followups" } */
> > +#include <new>
> > +
> > +/* Test non-throwing variants of operator new */
> > +
> > +struct A
> > +{
> > +  int x;
> > +  int y;
> > +};
> > +
> > +void test_throwing ()
> > +{
> > +  int* x = new int;
> > +  int* y = new int(); /* { dg-warning "dereference of possibly-NULL" }
> */
> > +  int* arr = new int[10];
> > +  A *a = new A(); /* { dg-warning "dereference of possibly-NULL" } */
> > +
> > +  int z = *y + 2;
> > +  z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */
> > +  /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-*
> } .-1 } */
> > +  z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'"
> } */
> > +  /* { dg-warning "use of uninitialized value '\\*arr'" "" { target
> *-*-* } .-1 } */
> > +  a->y = a->x + 3;
> > +
> > +  delete a;
> > +  delete y;
> > +  delete x;
> > +  delete[] arr;
> > +}
> > +
> > +void test_nonthrowing ()
> > +{
> > +  int* x = new(std::nothrow) int;
> > +  int* y = new(std::nothrow) int();
> > +  int* arr = new(std::nothrow) int[10];
> > +
> > +  int z = *y + 2; /* { dg-warning "dereference of NULL 'y'" } */
> > +  /* { dg-warning "use of uninitialized value '\\*y'" "" { target *-*-*
> } .-1 } */
> > +  z = *x + 4; /* { dg-warning "dereference of possibly-NULL 'x'" } */
> > +  /* { dg-warning "use of uninitialized value '\\*x'" "" { target *-*-*
> } .-1 } */
> > +  z = arr[0] + 4; /* { dg-warning "dereference of possibly-NULL 'arr'"
> } */
> > +  /* { dg-warning "use of uninitialized value '\\*arr'" "" { target
> *-*-* } .-1 } */
> > +
> > +  delete y;
> > +  delete x;
> > +  delete[] arr;
> > +}
>
> I see that we have test coverage for:
>   noexcept-new.C: -fno-exceptions with new vs nothrow-new
> whereas:
>   new-2.C has (implicitly) -fexceptions with new
>
> It seems that of the four combinations for:
>   - exceptions enabled or disabled
> and:
>   - throwing versus non-throwing new
> this is covering three of the cases but is missing the case of nothrow-
> new when exceptions are enabled.
> Presumably new-2.C should gain test coverage for this case.  Or am I
> missing something here?  Am I right in thinking that it's possible for
> the user to use nothrow new when exceptions are enabled to get a new
> that can fail and return nullptr?  Or is that not possible?
>
>
Thanks a lot for spotting that, the new test pointed out an issue with the
detection of nothrow.
It has been fixed and now both test cases behave similarly.
However, this highlighted a faulty test case I had written.

int* y = new(std::nothrow) int();
int z = *y + 2; /* { dg-warning "dereference of NULL 'y'" } */
/* { dg-warning "use of uninitialized value '\\*y'" "" { xfail *-*-* } .-1
} */ // (#) should be a bogus
delete y;

The test labelled (#) is wrong and should be a bogus instead.
If "y" is null then the allocation failed and dereferencing "y" will cause
a segfault, not a "use-of-uninitialized-value".
Thus we should stick to 'dereference of NULL 'y'" only.
If "y" is non-null then the allocation succeeded and "*y" is initialized
since we are calling a default initialization with the empty parenthesis.

This led me to consider having "null-dereference" supersedes
"use-of-uninitialized-value", but
new PR 110830 made me reexamine it.

I believe fixing PR 110830 is thus required before submitting this patch,
or we would have some extra irrelevant warnings.

I've also fixed the wording of "use-after-free" when using a placement
pointer after deallocation of its underlying buffer.

A *a = ::new A ();
int *lp = ::new (&a->y) int;
delete a;
int m = *lp; /* { dg-bogus "use after 'free' of 'lp'" "PREVIOUSLY" } */
int m = *lp; /* { dg-warning "use after 'delete' of 'lp'" "CURRENTLY" } */

Thanks
Benjamin