[Bug tree-optimization/92116] Potential null pointer dereference in 'gomp_acc_remove_pointer'

[msebor at gcc dot gnu.org]

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

Martin Sebor  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED
   Assignee|msebor at gcc dot gnu.org  |unassigned at gcc dot 
gnu.org

--- Comment #4 from Martin Sebor  ---
(In reply to Thomas Schwinge from comment #2)

IIRC, I assumed based on the check for t != NULL in the controlling expression
that the loop could terminate with t == NULL.  The warning code predates the
patch I was testing -- it just enables it without enhancing its implementation.
 But since r279147 removes the loop I think this report can be resolved, either
as fixed or as invalid.

[Bug tree-optimization/92116] Potential null pointer dereference in 'gomp_acc_remove_pointer'

[tschwinge at gcc dot gnu.org]

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

--- Comment #3 from Thomas Schwinge  ---
Author: tschwinge
Date: Mon Dec  9 22:52:56 2019
New Revision: 279147

URL: https://gcc.gnu.org/viewcvs?rev=279147=gcc=rev
Log:
[PR92116, PR92877] [OpenACC] Replace 'openacc.data_environ' by standard libgomp
mechanics

libgomp/
PR libgomp/92116
PR libgomp/92877
* oacc-mem.c (lookup_dev): Reimplement.  Adjust all users.
* libgomp.h (struct acc_dispatch_t): Remove 'data_environ' member.
Adjust all users.
* testsuite/libgomp.oacc-c-c++-common/acc_free-pr92503-4-2.c:
Remove XFAIL.
* testsuite/libgomp.oacc-c-c++-common/acc_free-pr92503-4.c:
Likewise.
* testsuite/libgomp.oacc-c-c++-common/pr92877-1.c: New file.

Added:
trunk/libgomp/testsuite/libgomp.oacc-c-c++-common/pr92877-1.c
Modified:
trunk/libgomp/ChangeLog
trunk/libgomp/libgomp.h
trunk/libgomp/oacc-host.c
trunk/libgomp/oacc-mem.c
trunk/libgomp/target.c
trunk/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_free-pr92503-4-2.c
trunk/libgomp/testsuite/libgomp.oacc-c-c++-common/acc_free-pr92503-4.c

[Bug tree-optimization/92116] Potential null pointer dereference in 'gomp_acc_remove_pointer'

[tschwinge at gcc dot gnu.org]

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

Thomas Schwinge  changed:

   What|Removed |Added

   Keywords|openacc |
  Component|libgomp |tree-optimization
   Assignee|jules at gcc dot gnu.org   |msebor at gcc dot 
gnu.org

--- Comment #2 from Thomas Schwinge  ---
Martin Sebor, regarding this:

(In reply to myself from comment #0)
> As reported in
> :
> 
> | PS I tried compiling GCC with [a new] patch.  It fails in libgomp
> | with:
> | 
> |libgomp/oacc-mem.c: In function ‘gomp_acc_remove_pointer’:
> |cc1: warning: invalid use of a null pointer [-Wnonnull]
> | 
> | so clearly it's missing location information.  With
> | -Wnull-dereference enabled we get more detail:
> | 
> |libgomp/oacc-mem.c: In function ‘gomp_acc_remove_pointer’:
> |libgomp/oacc-mem.c:1013:31: warning: potential null pointer dereference 
> [-Wnull-dereference]
> | 1013 |   for (size_t i = 0; i < t->list_count; i++)
> |  |  ~^~~~
> |libgomp/oacc-mem.c:1012:19: warning: potential null pointer dereference 
> [-Wnull-dereference]
> | 1012 |   t->refcount = minrefs;
> |  |   ^
> |libgomp/oacc-mem.c:1013:31: warning: potential null pointer dereference 
> [-Wnull-dereference]
> | 1013 |   for (size_t i = 0; i < t->list_count; i++)
> |  |  ~^~~~
> |libgomp/oacc-mem.c:1012:19: warning: potential null pointer dereference 
> [-Wnull-dereference]
> | 1012 |   t->refcount = minrefs;
> |  |   ^
> |cc1: warning: invalid use of a null pointer [-Wnonnull]
> | 
> | I didn't spend too long examining the code but it seems like
> | the warnings might actually be justified.  When the first loop
> | terminates with t being null

It's actually not possible to terminate this loop with 't = NULL'.  ;-)

As I understand this, what your compiler analysis pass has difficulties to
figure out (from the code as written in 'gomp_acc_remove_pointer'?) is that
here:

> | the subsequent dereferences are
> | invalid:
> | 
> |if (t->refcount == minrefs)
> | {
> |   /* This is the last reference, so pull the descriptor off the
> |  chain. This prevents gomp_unmap_vars via gomp_unmap_tgt from
> |  freeing the device memory. */
> |   struct target_mem_desc *tp;
> |   for (tp = NULL, t = acc_dev->openacc.data_environ; t != NULL;
> |tp = t, t = t->prev)
> | {
> |   if (n->tgt == t)
> | {
> |   if (tp)
> | tp->prev = t->prev;
> |   else
> | acc_dev->openacc.data_environ = t->prev;
> |   break;
> | }
> | }
> | }

... we are always going to eventually find 'n->tgt == t': we start this loop
with 't == n->tgt' (see further above), and what this code here is doing is
just to locate 'n->tgt' in 'data_environ', which is guaranteed to contain it
(yes, there should be some 'assert' or similar for that...), and then un-link
it from 'data_environ'.  Then we 'break', and thus after the loop we again have
the original 't = n->tgt', and so here:

> |/* Set refcount to 1 to allow gomp_unmap_vars to unmap it.  */
> |n->refcount = 1;
> |t->refcount = minrefs;
> |for (size_t i = 0; i < t->list_count; i++)

..., there will never be 't == NULL'.

Certainly this code is a bit "strange" -- but maybe that's something your
analysis has to consider for "real-world code"?  I'm assigning this PR to you
in case you'd like to track this, otherwise please set RESOLVED, INVALID or
something like that.

..., and that said, we shall be removing this code from
'gomp_acc_remove_pointer' any moment now...  ;-)