Bug 104069 - -Werror=use-after-free false positive on elfutils-0.186
Summary: -Werror=use-after-free false positive on elfutils-0.186
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on:
Blocks: Wuse-after-free
  Show dependency treegraph
 
Reported: 2022-01-17 17:18 UTC by Sergei Trofimovich
Modified: 2023-01-23 19:21 UTC (History)
9 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-01-17 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich 2022-01-17 17:18:52 UTC
Originally observed on elfutils-0.186 (which builds with -Werror by default).

Here is my attempt to extract simplified example:


typedef long unsigned int size_t;

extern void *realloc(void *__ptr, size_t __size)
    __attribute__((__nothrow__, __leaf__))
    __attribute__((__warn_unused_result__)) __attribute__((__alloc_size__(2)));

void * __libdw_unzstd(size_t todo) {
  void * sb = 0;

  for(;;) {
    // ran ony once
    if (!sb) {
      char * b = realloc(sb, todo);
      if (!b)
        break;

      sb = b;
    }

    todo -= 1;
    if (todo == 0)
      break;
  }

  // shrink buffer: leave only one byte for simplicity
  char * b = realloc(sb, 1);
  if (b) {
      sb = b;
  } else {
      // realloc failed mysteriously, leave 'sb' untouched.
  }

  return sb;
}

$ gcc-12.0.0 -O2 -std=gnu99 -Wall -Werror -c zstd.c.c
zstd.c.c: In function ‘__libdw_unzstd’:
zstd.c.c:35:10: error: pointer ‘sb’ may be used after ‘realloc’ [-Werror=use-after-free]
   35 |   return sb;
      |          ^~
zstd.c.c:28:14: note: call to ‘realloc’ here
   28 |   char * b = realloc(sb, 1);
      |              ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors

I think it's a proper false positive. Original code is not as contrived (but I think it's still correct): https://sourceware.org/git/?p=elfutils.git;a=blob;f=libdwfl/gzip.c;h=ba8ecfba6c316b261ee38bb288ab163664ade9e5;hb=983e86fd89e8bf02f2d27ba5dce5bf078af4ceda#l180

$ gcc-12.0.0 -v
Using built-in specs.
COLLECT_GCC=/<<NIX>>/gcc-12.0.0/bin/gcc
COLLECT_LTO_WRAPPER=/<<NIX>>/gcc-12.0.0/libexec/gcc/x86_64-unknown-linux-gnu/12.0.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with:
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 12.0.0 20220116 (experimental) (GCC)
Comment 1 Andrew Pinski 2022-01-17 17:27:41 UTC
The warning defiantly looks wrong. Realloc fails so it's argument is still valid.
Comment 2 Sergei Trofimovich 2022-01-17 17:29:11 UTC
Similar code triggers the same warning (and error due to -Werror) on current linux.git:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/subcmd/subcmd-util.h?id=0c947b893d69231a9add855939da7c66237ab44f#n50

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;
}

  CC      tools/objtool/help.o
In file included from help.c:12:
In function 'xrealloc',
    inlined from 'add_cmdname' at help.c:24:2:
subcmd-util.h:56:23: error: pointer may be used after 'realloc' [-Werror=use-after-free]
   56 |                 ret = realloc(ptr, size);
      |                       ^~~~~~~~~~~~~~~~~~
subcmd-util.h:52:21: note: call to 'realloc' here
   52 |         void *ret = realloc(ptr, size);
      |                     ^~~~~~~~~~~~~~~~~~
subcmd-util.h:58:31: error: pointer may be used after 'realloc' [-Werror=use-after-free]
   58 |                         ret = realloc(ptr, 1);
      |                               ^~~~~~~~~~~~~~~
subcmd-util.h:52:21: note: call to 'realloc' here
   52 |         void *ret = realloc(ptr, size);
      |                     ^~~~~~~~~~~~~~~~~~
Comment 3 Martin Sebor 2022-01-17 19:10:31 UTC
The warning triggers at -O0 and above for an IL that looks like this (at -O1):

  <bb 6> [local count: 59126544]:
  # sb_4 = PHI <0B(4), sb_3(5)>
  b_15 = realloc (sb_4, 1);
  if (b_15 != 0B)
    goto <bb 7>; [99.96%]
  else
    goto <bb 8>; [0.04%]

  <bb 7> [local count: 59102893]:

  <bb 8> [local count: 59126544]:
  # sb_5 = PHI <sb_4(6), b_15(7)>
  return sb_5;                         <<< -Wuse-after-free 

}

All it does is check to see if the use of sb_5 in the return statement is dominated by the realloc call without considering the data flow (i.e., that sb_5 is equal to sb_4 when b_15 is null).

The handling of the dominance is too simplistic.  Let me look into make it more intelligent.
Comment 4 Martin Sebor 2022-01-17 21:52:53 UTC
Actually, this is already supposed to be handled but the code is not effective due to a typo.  This fixes it:

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index f639807a78a..f9508a1d211 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -4082,7 +4082,9 @@ pointers_related_p (gimple *stmt, tree p, tree q, pointer_query &qry)
   access_ref pref, qref;
   if (!qry.get_ref (p, stmt, &pref, 0)
       || !qry.get_ref (q, stmt, &qref, 0))
-    return true;
+    /* GET_REF() only rarely fails.  When it does, it's likely because
+       it involves a self-referential PHI.  Return a conservative result.  */
+    return false;
 
   return pref.ref == qref.ref;
 }
Comment 5 Sergei Trofimovich 2022-01-18 09:37:22 UTC
(In reply to Martin Sebor from comment #4)
> Actually, this is already supposed to be handled but the code is not
> effective due to a typo.  This fixes it:
> 
> diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
> index f639807a78a..f9508a1d211 100644
> --- a/gcc/gimple-ssa-warn-access.cc
> +++ b/gcc/gimple-ssa-warn-access.cc
> @@ -4082,7 +4082,9 @@ pointers_related_p (gimple *stmt, tree p, tree q,
> pointer_query &qry)
>    access_ref pref, qref;
>    if (!qry.get_ref (p, stmt, &pref, 0)
>        || !qry.get_ref (q, stmt, &qref, 0))
> -    return true;
> +    /* GET_REF() only rarely fails.  When it does, it's likely because
> +       it involves a self-referential PHI.  Return a conservative result. 
> */
> +    return false;
>  
>    return pref.ref == qref.ref;
>  }

Thank you! This helped elfutils-0.186.

But linux's objtool still fails to build. Here is extracted false positive (I think it's false positive):

typedef long unsigned int size_t;

extern void *malloc(size_t __size) __attribute__((__nothrow__, __leaf__))
__attribute__((__malloc__)) __attribute__((__alloc_size__(1)));
extern void *realloc(void *__ptr, size_t __size)
    __attribute__((__nothrow__, __leaf__))
    __attribute__((__warn_unused_result__)) __attribute__((__alloc_size__(2)));
extern void die(const char *err, ...);

struct cmdnames {
  size_t alloc;
  size_t cnt;
  struct cmdname {
    size_t len;
    char name[];
  } * *names;
};

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;
}

void add_cmdname(struct cmdnames *cmds, const char *name, size_t len) {
  struct cmdname *ent = malloc(sizeof(*ent) + len + 1);
  ent->len = len;
  // memcpy(ent->name, name, len);
  ent->name[len] = 0;
  do {
    if ((cmds->cnt + 1) > cmds->alloc) {
      if ((((cmds->alloc) + 16) * 3 / 2) < (cmds->cnt + 1))
        cmds->alloc = (cmds->cnt + 1);
      else
        cmds->alloc = (((cmds->alloc) + 16) * 3 / 2);
      cmds->names =
          xrealloc((cmds->names), cmds->alloc * sizeof(*(cmds->names)));
    }
  } while (0);
  cmds->names[cmds->cnt++] = ent;
}

$ gcc-12.0.0 -Wall -Werror -c help.c.c
help.c.c: In function 'xrealloc':
help.c.c:26:13: error: pointer 'ptr' may be used after 'realloc' [-Werror=use-after-free]
   26 |       ret = realloc(ptr, 1);
      |             ^~~~~~~~~~~~~~~
help.c.c:20:15: note: call to 'realloc' here
   20 |   void *ret = realloc(ptr, size);
      |               ^~~~~~~~~~~~~~~~~~
help.c.c:24:11: error: pointer 'ptr' may be used after 'realloc' [-Werror=use-after-free]
   24 |     ret = realloc(ptr, size);
      |           ^~~~~~~~~~~~~~~~~~
help.c.c:20:15: note: call to 'realloc' here
   20 |   void *ret = realloc(ptr, size);
      |               ^~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Comment 6 Martin Sebor 2022-01-18 22:03:54 UTC
Thanks.  The new test case can be reduced to this:

void *xrealloc (void *ptr, int size)
{
  void *ret = __builtin_realloc (ptr, size);
  if (!ret && !size)
    ret = __builtin_realloc (ptr, 1);
  if (!ret) {
    ret = __builtin_realloc (ptr, size);   <<< -Wuse-after-free
    if (!ret && !size)
      ret = __builtin_realloc(ptr, 1);     <<< -Wuse-after-free
    if (!ret)
      __builtin_abort ();
  }
  return ret;
}

I don't see a bug in the warning code but let me CC Andrew for his comments on the Ranger behavior.

The warnings are false positives but I'm afraid I don't see a bug in the implementation.  Both trigger (at level 2) because the code is unable to rule out that the successfully reallocated pointer isn't used in a subsequent call to realloc().  In the unptimized IL the annotated flow for the first warning is below.  It shows that the realloc(ptr) statement in bb 8 isn't reachable with a nonnull ret_13 (i.e., after a successful call to realloc(ptr) in bb 2) but the range query for the value of ret_13 in bb 8 returns an unknown (i.e., either null or nonnull).

Andrew, this is determined by the following test art gimple-ssa-warn-access.cc:4152:

	      if (m_ptr_qry.rvals->range_of_expr (vr, realloc_lhs, use_stmt))

realloc_lhs is ret_13 and use_stmt is ret_19 = __builtin_realloc (ptr_11(D), 1).  debug_ranger() shows that bb 5 exports ret_3 == 0 but not ret_13 (which is implied by the ret_3 equality to zero).  Is this a limitation I need to be prepared for?

void * xrealloc (void * ptr, int size)
{
  void * ret;
  void * D.1996;
  long unsigned int _1;
  long unsigned int _2;
  void * _21;

  <bb 2> :
  _1 = (long unsigned int) size_9(D);
  ret_13 = __builtin_realloc (ptr_11(D), _1);
  if (ret_13 == 0B)
    goto <bb 3>; [INV]                          >>> ret_13 == 0
  else
    goto <bb 5>; [INV]                          >>> ret_13 != 0

  <bb 3> :
  if (size_9(D) == 0)
    goto <bb 4>; [INV]
  else
    goto <bb 5>; [INV]

  <bb 4> :
  ret_15 = __builtin_realloc (ptr_11(D), 1);    <<< ret_13 == 0

  <bb 5> :
  # ret_3 = PHI <ret_13(2), ret_13(3), ret_15(4)>
  if (ret_3 == 0B)
    goto <bb 6>; [INV]                          >>> ret_13 == 0 && ret_3 == 0
  else
    goto <bb 11>; [INV]

  <bb 6> :
  _2 = (long unsigned int) size_9(D);
  ret_17 = __builtin_realloc (ptr_11(D), _2);
  if (ret_17 == 0B)
    goto <bb 7>; [INV]                          >>> ret_13 == 0 && ret_3 == 0 && ret_17 == 0
  else
    goto <bb 9>; [INV]

  <bb 7> :
  if (size_9(D) == 0)
    goto <bb 8>; [INV]                          >>> ret_13 == 0 && ret_3 == 0 && ret_17 == 0 && size_9 == 0
  else
    goto <bb 9>; [INV]

  <bb 8> :                                      <<< ret_13 need not be 0 here
  ret_19 = __builtin_realloc (ptr_11(D), 1);    <<< -Wuse-after-free

  <bb 9> :
  # ret_4 = PHI <ret_17(6), ret_17(7), ret_19(8)>
  if (ret_4 == 0B)
    goto <bb 10>; [INV]
  else
    goto <bb 11>; [INV]

  <bb 10> :
  __builtin_abort ();

  <bb 11> :
  # ret_5 = PHI <ret_3(5), ret_4(9)>
  _21 = ret_5;

  <bb 12> :
<L12>:
  return _21;

}

In the optimized IL (at -O2 below), it looks like the same issue.  Again, the IL makes it clear that the call to realloc(ptr) in bb 5 where the warning triggers isn't reachable after a successful prior realloc(), 

void * xrealloc (void * ptr, int size)
{
  void * ret;
  long unsigned int _1;
  _Bool _2;
  _Bool _3;
  _Bool _4;
  _Bool _5;
  _Bool _6;

  <bb 2> [local count: 1073741824]:
  _1 = (long unsigned int) size_13(D);
  ret_17 = __builtin_realloc (ptr_15(D), _1);
  _2 = ret_17 == 0B;
  _3 = size_13(D) == 0;
  _4 = _2 & _3;
  if (_4 != 0)
    goto <bb 3>; [33.00%]                       >>> ret_17 == 0
  else
    goto <bb 4>; [67.00%]                       >>> ret_17 need not be 0

  <bb 3> [local count: 354334800]:
  ret_19 = __builtin_realloc (ptr_15(D), 1);

  <bb 4> [local count: 1073741824]:
  # ret_7 = PHI <ret_17(2), ret_19(3)>
  if (ret_7 == 0B)
    goto <bb 5>; [0.04%]                        >>> ret_7 == 0 implies ret_17 == 0
  else
    goto <bb 9>; [99.96%]

  <bb 5> [local count: 429496]:                 <<< ret_17 == 0 but query returns unknown
  ret_21 = __builtin_realloc (ptr_15(D), _1);   <<< -Wuse-after-free
Comment 7 Martin Sebor 2022-01-18 22:06:54 UTC
The debug_ranger() output is below for reference:

=========== BB 2 ============
Imports: ret_13  
Exports: ret_13  
size_9(D)	int VARYING
    <bb 2> :
    _1 = (long unsigned int) size_9(D);
    ret_13 = __builtin_realloc (ptr_11(D), _1);
    if (ret_13 == 0B)
      goto <bb 3>; [INV]
    else
      goto <bb 5>; [INV]

_1 : long unsigned int [0, 2147483647][18446744071562067968, +INF]
2->3  (T) ret_13 : 	void * [0B, 0B]
2->5  (F) ret_13 : 	void * [1B, +INF]

=========== BB 3 ============
Imports: size_9(D)  
Exports: size_9(D)  
size_9(D)	int VARYING
ret_13	void * [0B, 0B]
    <bb 3> :
    if (size_9(D) == 0)
      goto <bb 4>; [INV]
    else
      goto <bb 5>; [INV]

3->4  (T) size_9(D) : 	int [0, 0]
3->5  (F) size_9(D) : 	int [-INF, -1][1, +INF]

=========== BB 4 ============
    <bb 4> :
    ret_15 = __builtin_realloc (ptr_11(D), 1);


=========== BB 5 ============
Imports: ret_3  
Exports: ret_3  
    <bb 5> :
    # ret_3 = PHI <ret_13(2), ret_13(3), ret_15(4)>
    if (ret_3 == 0B)
      goto <bb 6>; [INV]
    else
      goto <bb 11>; [INV]

5->6  (T) ret_3 : 	void * [0B, 0B]
5->11  (F) ret_3 : 	void * [1B, +INF]

=========== BB 6 ============
Imports: ret_17  
Exports: ret_17  
size_9(D)	int VARYING
    <bb 6> :
    _2 = (long unsigned int) size_9(D);
    ret_17 = __builtin_realloc (ptr_11(D), _2);
    if (ret_17 == 0B)
      goto <bb 7>; [INV]
    else
      goto <bb 9>; [INV]

_2 : long unsigned int [0, 2147483647][18446744071562067968, +INF]
6->7  (T) ret_17 : 	void * [0B, 0B]
6->9  (F) ret_17 : 	void * [1B, +INF]

=========== BB 7 ============
Imports: size_9(D)  
Exports: size_9(D)  
size_9(D)	int VARYING
ret_17	void * [0B, 0B]
    <bb 7> :
    if (size_9(D) == 0)
      goto <bb 8>; [INV]
    else
      goto <bb 9>; [INV]

7->8  (T) size_9(D) : 	int [0, 0]
7->9  (F) size_9(D) : 	int [-INF, -1][1, +INF]

=========== BB 8 ============
    <bb 8> :
    ret_19 = __builtin_realloc (ptr_11(D), 1);


=========== BB 9 ============
Imports: ret_4  
Exports: ret_4  
    <bb 9> :
    # ret_4 = PHI <ret_17(6), ret_17(7), ret_19(8)>
    if (ret_4 == 0B)
      goto <bb 10>; [INV]
    else
      goto <bb 11>; [INV]

9->10  (T) ret_4 : 	void * [0B, 0B]
9->11  (F) ret_4 : 	void * [1B, +INF]

=========== BB 10 ============
    <bb 10> :
    __builtin_abort ();


=========== BB 11 ============
Equivalence set : []
Equivalence set : [ret_5, _21]
    <bb 11> :
    # ret_5 = PHI <ret_3(5), ret_4(9)>
    _21 = ret_5;

ret_5 : void * [1B, +INF]
_21 : void * [1B, +INF]

=========== BB 12 ============
    <bb 12> :
  <L15>:
    return _21;

Non-varying global ranges:
=========================:
_1  : long unsigned int [0, 2147483647][18446744071562067968, +INF]
_2  : long unsigned int [0, 2147483647][18446744071562067968, +INF]
ret_5  : void * [1B, +INF]
_21  : void * [1B, +INF]
Comment 8 Jakub Jelinek 2022-01-18 22:17:18 UTC
ret_7 == 0 certainly doesn't imply ret_17 == 0, so it is correct ranger doesn't know.  ret_7 == 0 implies that either ret_17 == 0 if bb 2 jumped to bb 4, in that case ret_19 isn't even defined, or ret_19 is 0 if bb 3 fell through to bb 4, in that case ret_7 is defined, but could be 0 or could be any other value.
So it isn't a bug on the ranger side, but on the warning side.
Comment 9 Martin Sebor 2022-01-18 22:47:14 UTC
My mistake, a PHI is an OR of its operands, not an AND.  With that, the IL doesn't rule out that the subsequent realloc() call isn't made with the argument to a prior successful realloc().  So for the more involved test case in comment #5 the warning is doing what it's supposed to do.
Comment 10 Jakub Jelinek 2022-01-18 22:56:13 UTC
It is not, because it emits a false positive on a fairly common code.
Anyway, if bb3 jumps to bb4, then bb3 should have in the ranger assertion that in bb3 ret_17 is 0 (it is on the true branch of the ret_17 == 0 && something test),
so for the PHI, while it is or, it is either 2->4 is the executable edge and then ret_7 == 0 implies ret_17 == 0, or 3->4 is the executable edge and then ret_17 == 0 too because that was what was the assertion in bb 3.  But arguably it isn't a very common case for PHIs.  So, either the ranger can have special case for something like that, or the warning code can.
Comment 11 Andrew Macleod 2022-01-19 00:51:13 UTC
(In reply to Jakub Jelinek from comment #10)
> It is not, because it emits a false positive on a fairly common code.
> Anyway, if bb3 jumps to bb4, then bb3 should have in the ranger assertion
> that in bb3 ret_17 is 0 (it is on the true branch of the ret_17 == 0 &&
> something test),
> so for the PHI, while it is or, it is either 2->4 is the executable edge and
> then ret_7 == 0 implies ret_17 == 0, or 3->4 is the executable edge and then
> ret_17 == 0 too because that was what was the assertion in bb 3.  But
> arguably it isn't a very common case for PHIs.  So, either the ranger can
> have special case for something like that, or the warning code can.

I dont follow what you guys are looking at. so I looked in EVRP and i see:


    ret_17 = realloc (ptr_14(D), size_15(D));
    _1 = ret_17 == 0B;
    _2 = size_15(D) == 0;
    _3 = _1 & _2;
    if (_3 != 0)
      goto <bb 3>; [INV]
    else
      goto <bb 4>; [INV]

2->3  (T) ret_17 :      void * [0B, 0B]


=========== BB 3 ============
_2      _Bool [1, 1]
size_15(D)      size_t [0, 0]
    <bb 3> :
    ret_19 = realloc (ptr_14(D), 1);


=========== BB 4 ============
Imports: ret_7
Exports: ret_7
_2      _Bool VARYING
size_15(D)      long unsigned int VARYING
    <bb 4> :
    # ret_7 = PHI <ret_17(2), ret_19(3)>
    if (ret_7 == 0B)
      goto <bb 5>; [INV]

So you are saying that ret_17 is 0 if we go 2->3, then 3->4 and set ret_7 to ret_19.
And looking backwards, if ret_7 == 0, then ret_7 is zero which implies that ret_17 was zero on the path 2->4 if we came that way

and therefore, if we take the branch 4->5, that ret_17 is 0 ?

Thats not happening any time soon. 

Theres way to much conditional context.  Basic data flow says we track ret_17 as [0,0] on 2->3 and VARYING on 2->4
BB4 is also a merge point since 3->4, so the means, based on data flow, that ret_17 is now VARYING again.
THe diamond has ended, contextual range information is gone.

First, PHI arguments are not exports from a block, their aggregator the  PHI def is... and there it ends. the computational complexities of generically doing arguments are exponential, esepcially since we know nothing about the edge taken.  You need to see some of the crazy PHIs out there.
 
What you are suggesting here is that because the PHI which uses ret_17 on one path must be zero (or whatever range) , and it happens to be zero on the other path, we must therefore know conclusively that it is also zero.  

My guess is this would be fairly uncommon.

Its possible down the road that we can use the gori unwinding to analyze ranges which end a PHI.  Winding back from 
  if (ret_7 == 0B)
we get [0,0] = PHI <ret_17(2), ret_19(3)>  that we can teach gori or some other unit about PHIS (we do not understand them yet since GORI is purely a basic block concept) to make queries about ret_17 on each edge..   even 3->4 for no apparent reason, except that we asked about ret_17.  Union them all together and see what happens. I'll keep it in mind, but that is not a high priority.

You can forget about it for this release tho :-)  Besides, the information ranger is providing shouldn't be any different than what we got before... ranger doesn't know anything, neither did previous versions.  This isnt a "loss of information" or even a "too good of information" situation.  Its basically the same as before is it not?
Comment 12 CVS Commits 2022-01-19 01:04:20 UTC
The master branch has been updated by Martin Sebor <msebor@gcc.gnu.org>:

https://gcc.gnu.org/g:2f714642e574c64e1c0e093cad3de6f8accb6ec7

commit r12-6712-g2f714642e574c64e1c0e093cad3de6f8accb6ec7
Author: Martin Sebor <msebor@redhat.com>
Date:   Tue Jan 18 17:52:01 2022 -0700

    Handle failure to determine pointer provenance conservatively [PR104069].
    
    Partly resolves:
    PR middle-end/104069 - -Werror=use-after-free false positive on elfutils-0.186
    
    gcc/ChangeLog:
            PR middle-end/104069
            * gimple-ssa-warn-access.cc (pointers_related_p): Return false for
            an unknown result as documented.
    
    gcc/testsuite/ChangeLog:
            PR middle-end/104069
            * gcc.dg/Wuse-after-free.c: New test.
Comment 13 Martin Sebor 2022-01-19 20:22:45 UTC
I confused things (including myself) yesterday, sorry.

Let me try again, with just the -O2 behavior for the test case in comment #2:

void *xrealloc (void *ptr, int size)
{
  void *ret = __builtin_realloc (ptr, size);
  if (!ret && !size)
    ret = __builtin_realloc (ptr, 1);
  if (!ret) {
    ret = __builtin_realloc (ptr, size);
    if (!ret && !size)
      ret = __builtin_realloc(ptr, 1);
    if (!ret)
      __builtin_abort ();
  }
  return ret;
}

The optimized IL the warning works with is below.  The warning is "pointer may be used after 'realloc'" because the ptr argument to the realloc calls in bb 2 and 5 is the same, and because bb 5 can't be ruled out to be entered after the call in bb 2 succeeds (i.e., when ret_17 is nonnull).  The conditional in bb 2 is too complex.  I don't see what the warning code alone could do about it but there are ways to avoid it.

void * xrealloc (void * ptr, int size)
{
  void * ret;
  long unsigned int _1;
  _Bool _2;
  _Bool _3;
  _Bool _4;
  _Bool _5;
  _Bool _6;

  <bb 2> [local count: 1073741824]:
  _1 = (long unsigned int) size_13(D);
  ret_17 = __builtin_realloc (ptr_15(D), _1);
  _2 = ret_17 == 0B;
  _3 = size_13(D) == 0;
  _4 = _2 & _3;
  if (_4 != 0)
    goto <bb 3>; [33.00%]                       >>> ret_17 may be nonnull
  else
    goto <bb 4>; [67.00%]                       >>> ret_17 may be nonnull

  <bb 3> [local count: 354334800]:
  ret_19 = __builtin_realloc (ptr_15(D), 1);

  <bb 4> [local count: 1073741824]:
  # ret_7 = PHI <ret_17(2), ret_19(3)>
  if (ret_7 == 0B)
    goto <bb 5>; [0.04%]                        >>> ret_17(2) == 0 || ret_19(3) == 0
  else
    goto <bb 9>; [99.96%]

  <bb 5> [local count: 429496]:
  ret_21 = __builtin_realloc (ptr_15(D), _1);   <<< -Wuse-after-free=2 because ret_17(2) may be nonnull
  _5 = ret_21 == 0B;
  _6 = _3 & _5;
  if (_6 != 0)
    goto <bb 6>; [33.00%]
  else
    goto <bb 7>; [67.00%]
Comment 14 Martin Sebor 2022-01-19 20:24:53 UTC
Removing the test for !size from the first conditional avoids the warning.  I don't fully understand what the code tries to do but the following avoids it at -O2 (only):

void *xrealloc (void *ptr, int size)
{
  if (!size)
    size = 1;
  void *ret = __builtin_realloc (ptr, size);
  if (!ret)
    ret = __builtin_realloc (ptr, 1);
  if (!ret) {
    ret = __builtin_realloc (ptr, size);
    if (!ret)
      ret = __builtin_realloc(ptr, 1);
    if (!ret)
      __builtin_abort ();
  }
  return ret;
}
Comment 15 Andrew Pinski 2022-01-19 20:30:35 UTC
I think the new code in comment #14 is the only code which is well defined in c17 really. Before c17 (dr400), realloc for sizes of 0 the return value was unspecified which means it could return null or an allocated one. Now in c17, it is undefined.
Comment 17 Martin Sebor 2022-01-19 20:32:19 UTC
Having spent some time trying to understand the function I think the following simplification should capture its intent.  It compiles without warnings at all levels:

void *xrealloc (void *ptr, size_t size)
{
  void *ret = __builtin_realloc (ptr, size);
  if (ret)
    return ret;

  if (!size)
    size = 1;

  ret = __builtin_realloc (ptr, size);
  if (ret)
    return ret;

  __builtin_abort ();
}
Comment 18 Jakub Jelinek 2022-01-19 20:34:29 UTC
(In reply to Martin Sebor from comment #14)
> Removing the test for !size from the first conditional avoids the warning. 
> I don't fully understand what the code tries to do but the following avoids
> it at -O2 (only):
> 
> void *xrealloc (void *ptr, int size)
> {
>   if (!size)
>     size = 1;
>   void *ret = __builtin_realloc (ptr, size);
>   if (!ret)
>     ret = __builtin_realloc (ptr, 1);
>   if (!ret) {
>     ret = __builtin_realloc (ptr, size);
>     if (!ret)
>       ret = __builtin_realloc(ptr, 1);
>     if (!ret)
>       __builtin_abort ();
>   }
>   return ret;
> }

This is definitely not what the code meant to do.
If I do xrealloc (malloc (30), 1024 * 1024 * 1024) and it returns non-NULL,
it certainly shouldn't be that it under the hood called realloc (ptr, 1) because
the first realloc failed.
If you add that if (!size) size = 1;, then I think what is meant is just
  void *ret = __builtin_realloc (ptr, size);
  if (!ret)
    {
      ret = __builtin_realloc (ptr, size);
      if (!ret)
        die (...);
    }
  return ret;
Comment 19 Jakub Jelinek 2022-01-19 20:42:59 UTC
Note, actually because of the glibc and AIX behavior, after the first realloc returns NULL if size is 0, then ptr has been freed and so the code actually is a use after free.  For BSD it is not a use after free.
Comment 20 Martin Sebor 2022-01-19 20:54:01 UTC
This warning, like all others, is meant to help find common bugs in ordinary code.  It shouldn't be expected to reflect implementation-defined behavior or to be free of false positives.  Tricky code that tries to work around implementation specifics or divergences might be better off suppressing it using a #pragma.  Now that pragma suppression works reliably with inlining it should be straightforward to do.  Since -Wuse-after-free is a multi-level warning and this instance triggers only at level 2 ("may be used"), for code that can't be changed I suggest reducing the level to 1 on the command line.
Comment 21 Jakub Jelinek 2022-01-31 11:18:11 UTC
#c0 is fixed with the r12-6712-g2f714642e574c64e1c0e093cad3de6f8accb6ec7 change.
Comment 22 Martin Sebor 2022-03-17 19:32:48 UTC
I'm no longer working on this.
Comment 23 Rolf Eike Beer 2022-05-16 14:14:05 UTC
Using gcc12 from 4943b75e9f06f0b64ed541430bb7fbccf55fc552.

$ cat rea.c
#include <stdlib.h>

char *
compact_buffer(char *inbuf, size_t oldlen, size_t k)
{
        char *foo;
#ifdef WARN
        char **buf = &foo;
#endif

        foo = realloc(inbuf, k);
        if (foo == NULL) {
#if defined(WARN) && defined(WARN2)
                foo = inbuf;
                return inbuf;
#else
                return inbuf;
#endif
        }
        return foo;
}
$ gcc-12.0.1 -c -Wuse-after-free=3 -Irepos/Qsmtp/include rea.c
$ gcc-12.0.1 -c -DWARN -Wuse-after-free=2 -Irepos/Qsmtp/include rea.c
rea.c: In function 'compact_buffer':
rea.c:17:24: warning: pointer 'inbuf' may be used after 'realloc' [-Wuse-after-free]
   17 |                 return inbuf;
      |                        ^~~~~
rea.c:11:15: note: call to 'realloc' here
   11 |         foo = realloc(inbuf, k);
      |               ^~~~~~~~~~~~~~~~~
$ gcc-12.0.1 -c -DWARN -DWARN2 -Wuse-after-free=2 -Irepos/Qsmtp/include rea.c
rea.c: In function 'compact_buffer':
rea.c:15:24: warning: pointer 'inbuf' may be used after 'realloc' [-Wuse-after-free]
   15 |                 return inbuf;
      |                        ^~~~~
rea.c:11:15: note: call to 'realloc' here
   11 |         foo = realloc(inbuf, k);
      |               ^~~~~~~~~~~~~~~~~
rea.c:14:21: warning: pointer 'inbuf' may be used after 'realloc' [-Wuse-after-free]
   14 |                 foo = inbuf;
      |                 ~~~~^~~~~~~
rea.c:11:15: note: call to 'realloc' here
   11 |         foo = realloc(inbuf, k);
      |               ^~~~~~~~~~~~~~~~~
Comment 24 vincent 2022-08-24 08:51:50 UTC
elfutils 0.187 with gcc 12.2.0

In function 'bigger_buffer',
    inlined from '__libdw_gunzip' at gzip.c:376:12:
gzip.c:98:9: error: pointer may be used after 'realloc' [-Werror=use-after-free]
   98 |     b = realloc (state->buffer, more -= 1024);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gzip.c:96:13: note: call to 'realloc' here
   96 |   char *b = realloc (state->buffer, more);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Comment 25 Mark Wielaard 2022-10-27 16:57:45 UTC
Note that elfutils-0.187 builds fine for me on fedora x86_64 with either:
gcc (GCC) 12.2.1 20220819 (Red Hat 12.2.1-2)

So this might have been fixed since 12.2.0?
Comment 26 Sergei Trofimovich 2022-10-28 07:36:06 UTC
#c12 fixed elfutils case.