[Bug tree-optimization/104215] New: bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[Bug tree-optimization/104215] New: bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[msebor at gcc dot gnu.org]

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

            Bug ID: 104215
           Summary: bogus -Wuse-after-free=3 due to forwprop moving a
                    pointer test after realloc
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: msebor at gcc dot gnu.org
  Target Milestone: ---

This is to make a record of the false positive.  I don't expect GCC to avoid
reordering the statements.  I don't see a way to avoid the warning except to
suppress it when it's moved (the statement is moved in forwprop1).

As discussed in the libc-alpha thread at
https://sourceware.org/pipermail/libc-alpha/2022-January/135586.html, at level
3 the new -Wuse-after-free warning complains about the integer variable in the
controlling expression of the if statement below,  The IL explains why: GCC
moves the variable's definition after the realloc call.  Since the defintion
uses the pointer made invalid by the call the warning triggers as designed.

$ cat a.c && gcc -O1 -S -Wall -fdump-tree-waccess3=/dev/stdout
-Wuse-after-free=3 a.c
void *p, *q;

void f (void)
{
  int c = p == q;
  void *r = __builtin_realloc (q, 7);
  if (!r)
    return;

  if (!c)   // <<< -Wuse-after-free=3
    __builtin_memcpy (r, p, 3);

  q = p = r;
}

;; Function f (f, funcdef_no=0, decl_uid=1981, cgraph_uid=1, symbol_order=2)

a.c: In function ‘f’:
a.c:10:6: warning: pointer may be used after ‘__builtin_realloc’
[-Wuse-after-free]
   10 |   if (!c)   // <<< -Wuse-after-free=3
      |      ^
a.c:6:13: note: call to ‘__builtin_realloc’ here
    6 |   void *r = __builtin_realloc (q, 7);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~
pointer_query counters:
  index cache size:   17
  index entries:      1
  access cache size:  2
  access entries:     1
  hits:               0
  misses:             11
  failures:           0
  max_depth:          1
void f ()
{
  void * r;
  void * p.0_1;
  void * q.1_2;
  void * p.3_3;

  <bb 2> [local count: 1073741824]:
  p.0_1 = p;
  q.1_2 = q;
  r_8 = __builtin_realloc (q.1_2, 7);
  if (r_8 == 0B)
    goto <bb 6>; [0.04%]
  else
    goto <bb 3>; [99.96%]

  <bb 3> [local count: 1073312329]:
  if (p.0_1 != q.1_2)                        <<< -Wuse-after-free=3
    goto <bb 4>; [53.47%]
  else
    goto <bb 5>; [46.53%]

  <bb 4> [local count: 573900101]:
  p.3_3 = p;
  __builtin_memcpy (r_8, p.3_3, 3);

  <bb 5> [local count: 1073312329]:
  p = r_8;
  q = r_8;

  <bb 6> [local count: 1073741824]:
  return;

}

[Bug tree-optimization/104215] bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

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

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
A use of the pointer value (not dereferencing it) after releasing the storage
it points to is perfectly valid, so I don't see how that could be considered an
error.  Esp. for equality compares which can compare pointers to distinct
objects.  Esp. also because 'q' may be equal to 'r', so

 void *r = __builtin_realloc (q, 7);
 if (r == q)
   {
...
   }

would also be diagnosed?  IMHO -Wuse-after-free needs to be fixed here.

[Bug tree-optimization/104215] bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[msebor at gcc dot gnu.org]

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

--- Comment #2 from Martin Sebor <msebor at gcc dot gnu.org> ---
The use in your example is undefined in C (as is any other use of an
indeterminate pointer value).  C++ made using pointers made it
implementation-defined a few years ago while still allowing for it to crash,
which is still effectively undefined.  The warning for your example (equality)
only triggers at level 3 (which is not the default in -Wall) to accommodate the
C++ decision.

[Bug tree-optimization/104215] bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[msebor at gcc dot gnu.org]

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

--- Comment #3 from Martin Sebor <msebor at gcc dot gnu.org> ---
I meant to say "C++ made it implementation-defined to use a pointer made
indeterminate by the pointee's lifetime having ended."

[Bug tree-optimization/104215] bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[rguenther at suse dot de]

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

--- Comment #4 from rguenther at suse dot de <rguenther at suse dot de> ---
> Am 25.01.2022 um 19:20 schrieb msebor at gcc dot gnu.org <gcc-bugzilla@gcc.gnu.org>:
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104215
> 
> --- Comment #3 from Martin Sebor <msebor at gcc dot gnu.org> ---
> I meant to say "C++ made it implementation-defined to use a pointer made
> indeterminate by the pointee's lifetime having ended."

I’m sure „use“ is too weak defined here.  Also reallocate only conditionally
ends the pointees lifetime since it can return NULL or the same pointer.

> -- 
> You are receiving this mail because:
> You are on the CC list for the bug.

[Bug tree-optimization/104215] bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[msebor at gcc dot gnu.org]

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

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
To "use" means to evaluate.  The strict C semantics are that the realloc()
argument becomes indeterminate after the function has returned non-null,
whether or not the returned pointer is the same as the argument.  The argument
is only safe to use after realloc() has returned null.  In other words,
strictly conforming programs must treat every successful call to realloc() as
if it freed the original object.

The warning is based on these strict semantics.  It handles the realloc failure
case, and it tries to accommodate the use case of detecting whether realloc()
has moved the block to a different address by only warning on equality
expressions involving the original argument at level 3.

The warning cannot very well avoid triggering on this case if we want it to
continue to work as designed (I of course do).  The solution I'd like to see is
the forwprop pass checking for the uses of the pointers in the propagated
condition in deallocation calls and suppressing the warning for the propagated
condition statement by calling suppress_warning().  This could be made reliable
by having forwprop and the warning share the same code (a common function to do
the same analysis to determine whether to suppress the warning in forwprop or
whether to trigger in gimple-ssa-warn-access).

[Bug tree-optimization/104215] bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[rguenther at suse dot de]

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

--- Comment #6 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 25 Jan 2022, msebor at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104215
> 
> --- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
> To "use" means to evaluate.  The strict C semantics are that the realloc()
> argument becomes indeterminate after the function has returned non-null,
> whether or not the returned pointer is the same as the argument.  The argument
> is only safe to use after realloc() has returned null.  In other words,
> strictly conforming programs must treat every successful call to realloc() as
> if it freed the original object.
> 
> The warning is based on these strict semantics.  It handles the realloc failure
> case, and it tries to accommodate the use case of detecting whether realloc()
> has moved the block to a different address by only warning on equality
> expressions involving the original argument at level 3.
> 
> The warning cannot very well avoid triggering on this case if we want it to
> continue to work as designed (I of course do).  The solution I'd like to see is
> the forwprop pass checking for the uses of the pointers in the propagated
> condition in deallocation calls and suppressing the warning for the propagated
> condition statement by calling suppress_warning().  This could be made reliable
> by having forwprop and the warning share the same code (a common function to do
> the same analysis to determine whether to suppress the warning in forwprop or
> whether to trigger in gimple-ssa-warn-access).

Note it's not only forwprop but any pass that eventually uses fold_stmt
or moves code (sink for example) thereby honoring dataflow which
does not represent that realloc() "clobbers" its first argument
(on the !NULL return path).

So I don't think it's feasible to fix this which means I don't think
this warning will do any good to people.  For example I see
in a kernel build

[   59s] In file included from help.c:12:
[   59s] In function 'xrealloc',
[   59s]     inlined from 'add_cmdname' at help.c:24:2:
[   59s] subcmd-util.h:56:23: error: pointer may be used after 'realloc' 
[-Werror=use-after-free]
[   59s]    56 |                 ret = realloc(ptr, size);
[   59s]       |                       ^~~~~~~~~~~~~~~~~~
[   59s] subcmd-util.h:52:21: note: call to 'realloc' here
[   59s]    52 |         void *ret = realloc(ptr, size);
[   59s]       |                     ^~~~~~~~~~~~~~~~~~
[   59s] subcmd-util.h:58:31: error: pointer may be used after 'realloc' 
[-Werror=use-after-free]
[   59s]    58 |                         ret = realloc(ptr, 1);
[   59s]       |                               ^~~~~~~~~~~~~~~
[   59s] subcmd-util.h:52:21: note: call to 'realloc' here
[   59s]    52 |         void *ret = realloc(ptr, size);
[   59s]       |                     ^~~~~~~~~~~~~~~~~~

which eventually looks like

static inline void *xrealloc(void *ptr, size_t size)
{
        void *ret = realloc(ptr, size);
        if (!ret && !size)
                ret = realloc(ptr, 1);
        if (!ret) {
                ret = realloc(ptr, size);
                if (!ret && !size)
                        ret = realloc(ptr, 1);
                if (!ret)
                        die("Out of memory, realloc failed");
        }
        return ret;
}

so it seems there'll be false positives even with the strict
reading of the standard.

[Bug tree-optimization/104215] bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[msebor at gcc dot gnu.org]

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

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
The kernel false positive is discussed in bug 104069 comment 13.  I don't think
it's related to this issue but you're of course right that this warning isn't
immune to false positives.  That's true for all warnings in GCC but especially
so for the flow-dependent ones that depend on optimizers.  They're inherent in
their design.  The best we can do is try to minimize them but we'll never be
able to avoid them all.  But I'm tired of having this argument each time a
false positive pops up that can't be easily avoided so I won't be working on
any more warnings in the future.

[Bug tree-optimization/104215] bogus -Wuse-after-free=3 due to forwprop moving a pointer test after realloc

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-01-29

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.