[Bug c/54202] New: Overeager warning about freeing non-heap objects

[Bug c/54202] New: Overeager warning about freeing non-heap objects

[thiago at kde dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54202

             Bug #: 54202
           Summary: Overeager warning about freeing non-heap objects
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: minor
          Priority: P3
         Component: c
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: thiago@kde.org


GCC 4.7 has a warning about freeing non-heap objects that is way too eager.

Compiling the following code in C or C++:
======
#include <stdlib.h>

typedef struct Data
{ 
    int refcount;
} Data; 
extern const Data shared_null;

Data *allocate()
{
    return (Data *)(&shared_null);
} 

void dispose(Data *d)
{
    if (d->refcount == 0)
        free(d);
}

void f()
{
    Data *d = allocate();
    dispose(d);
}
====

Produces the following warning:

test.c: In function 'f'
test.c:17:13: warning: attempt to free a non-heap object 'shared_null'
[-Wfree-nonheap-object]

The warning is overeager because it says "attempt to free" without indicating
that it's only a possibility. GCC cannot prove that the call to free() will
happen with that particular pointer, as the value of shared_null.refcount is
not known.

The warning should either:
 a) be modified to indicate it's only a possibility and the compiler can't
prove it;
 b) be issued only when the compiler is sure that the free will happen on
non-heap objects.

Or both, by having two warnings: one for when it's sure and one for when it
isn't.

[Bug c/54202] Overeager warning about freeing non-heap objects

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54202

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-08-08
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-08-08 13:59:58 UTC ---
Confirmed.  This way it becomes an always may-be warning because a function
may not actually be executed (apart from main()).  Almost all warnings behave
this way btw.  GCC is in this case seeing

 free (&shared_null);

and considers this a good thing to warn on.

So would you consider printing

warning: possible attempt to free a non-heap object 'shared_null'

a fix?  Unconditionally, as we really cannot prove a line of code _will_
be executed at runtime (we can replace the free by an abort call though ...).

[Bug c/54202] Overeager warning about freeing non-heap objects

[thiago at kde dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54202

--- Comment #2 from Thiago Macieira <thiago at kde dot org> 2012-08-08 14:21:59 UTC ---
To be honest, I don't want false-positive warnings. The code and data are
constructed so that it never frees the non-heap object (it has a reference
count of -1). If the driver to this warning can't be improved to be certain,
I'd recommend at least changing the text, like the -Wuninitialized one:

  'varname' may be used uninitialized in this function

When GCC warnings are assertive, like the "will break strict aliasing" one, we
go an extra mile to try and fix them.

[Bug c/54202] Overeager warning about freeing non-heap objects

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54202

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-08-08 14:36:03 UTC ---
(In reply to comment #2)
> To be honest, I don't want false-positive warnings. The code and data are
> constructed so that it never frees the non-heap object (it has a reference
> count of -1). If the driver to this warning can't be improved to be certain,
> I'd recommend at least changing the text, like the -Wuninitialized one:
> 
>   'varname' may be used uninitialized in this function
> 
> When GCC warnings are assertive, like the "will break strict aliasing" one, we
> go an extra mile to try and fix them.

Note that even for the uninitialized use case we warn for functions
that may be never executed at runtime.  So - are you happy with the
definitive warning if the free () call happens unconditionally when
the function is entered?

[Bug c/54202] Overeager warning about freeing non-heap objects

[thiago at kde dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54202

--- Comment #4 from Thiago Macieira <thiago at kde dot org> 2012-08-08 14:53:13 UTC ---
(In reply to comment #3)
> Note that even for the uninitialized use case we warn for functions
> that may be never executed at runtime.  So - are you happy with the
> definitive warning if the free () call happens unconditionally when
> the function is entered?

I'm not sure I follow your reasoning. Please bear with me.

If GCC can prove that the function will be called with a non-heap object, print
the warning, even if the function in question never gets executed. That is,
after inlining, code like:

extern Data shared_null;
void dispose()
{
    free(&shared_null);
}

*should* print the warning, regardless of whether dispose() ever gets run.

My point was that the code that GCC was seeing, after inlining, was:

void f()
{
    if (shared_null.refcount == 0)
       free(&shared_null);
}

In which case, the call to free() isn't unconditional. In this case, the
warning should either be suppressed, or indicate that it's only a possibility
instead of being assertive.

[Bug c/54202] Overeager warning about freeing non-heap objects

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54202

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> 2012-08-08 19:00:58 UTC ---
Even the printf format warnings are this way too ....

[Bug middle-end/54202] Overeager warning about freeing non-heap objects

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org
          Component|c                           |middle-end

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
All late warnings are susceptible to the same problem.  The only way to tell
from the IL that the free call isn't unconditional is from the CFG.  While that
could be used to issue either a "maybe" kind of a warning or a definitive one
per function, it would just turn most instances of these warnings into the
"maybe" kind.  Which is how all warnings need to be viewed regardless.

What might help more than rewording every warning to say "this may be
undefined" is printing the chain of conditions that need to be satisfied in
order for the warning to trigger.  The infrastructure to do this is all there,
it's just a matter of taking advantage of it: when a warning is issued, walk
the shortest path from the entry to the basic block with the problem statement
and print each conditional along the way.  With that, we should end up with
output very close to that of the analyzer:

pr54202.c:17:9: warning: ‘free’ of ‘&shared_null’ which points to memory not on
the heap [CWE-590] [-Wanalyzer-free-of-non-heap]
   17 |         free(d);
      |         ^~~~~~~
  ‘f’: events 1-3
    |
    |   16 |     if (d->refcount == 0)
    |      |        ^
    |      |        |
    |      |        (1) following ‘true’ branch...
    |   17 |         free(d);
    |      |         ~~~~~~~
    |      |         |
    |      |         (2) ...to here
    |      |         (3) call to ‘free’ here
    |

Removing basic block 5
f ()
{
  int _2;

  <bb 2> [local count: 1073741824]:
  _2 = shared_null.refcount;
  if (_2 == 0)                       <<< condition
    goto <bb 3>; [33.00%]
  else
    goto <bb 4>; [67.00%]

  <bb 3> [local count: 354334800]:
  free (&shared_null); [tail call]   <<< conditional invalid call

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

}

[Bug middle-end/54202] Overeager warning about freeing non-heap objects

[dangelog at gmail dot com]

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

Giuseppe D'Angelo <dangelog at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dangelog at gmail dot com

--- Comment #8 from Giuseppe D'Angelo <dangelog at gmail dot com> ---
The original testcase by Thiago still fails with GCC 11.

Unfortunately, GCC 11 decided to turn -Wfree-nonheap-object on by default (!),
resulting in false positives for Qt users (containers in Qt use this pattern).
For instance 

https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qlinkedlist.h?h=5.15

struct QT_DEPRECATED_VERSION_5_15 QLinkedListData
{
    QLinkedListData *n, *p;
    QtPrivate::RefCount ref; // set to -1 for shared_null
    int size;
    uint sharable : 1;

    Q_CORE_EXPORT static const QLinkedListData shared_null;
};

template <typename T>
inline QLinkedList<T>::~QLinkedList()
{
    if (!d->ref.deref())
        freeData(d); // never called on shared_null because of the check
}


template <typename T>
void QLinkedList<T>::freeData(QLinkedListData *x)
{
    // ...
    delete x; // -Wfree-nonheap-object here
}

[Bug middle-end/54202] Overeager warning about freeing non-heap objects

[mserdarsanli at gmail dot com]

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

Serdar Sanli <mserdarsanli at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mserdarsanli at gmail dot com

--- Comment #9 from Serdar Sanli <mserdarsanli at gmail dot com> ---
A simpler example not involving any globals, causing Wfree-nonheap-object
warning since GCC11
https://godbolt.org/z/MYn1zrjE4

===
struct Foo {
    void allocate(int n);
    void deallocate();

    int *_data;
};

void Foo::allocate(int n) {
    _data = new int[n] - 1;
}

void Foo::deallocate() {
    delete[] (_data+1);
}

[Bug middle-end/54202] Overeager warning about freeing non-heap objects

[msebor at gcc dot gnu.org]

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

--- Comment #10 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Serdar Sanli from comment #9)
> A simpler example not involving any globals, causing Wfree-nonheap-object
> warning since GCC11

This is actually a bug in the example: it's invalid to decrement a pointer to
the beginning of an object as done in Foo::allocate.

[Bug middle-end/54202] Overeager warning about freeing non-heap objects

[charlechaud at gmail dot com]

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

Charles Blake <charlechaud at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |charlechaud at gmail dot com

--- Comment #11 from Charles Blake <charlechaud at gmail dot com> ---
Here is another example (no external includes/etc.):

typedef long unsigned int size_t;

extern void *malloc (size_t __size) __attribute__ ((__nothrow__ , __leaf__))
__attribute__ ((__malloc__))
     __attribute__ ((__alloc_size__ (1))) __attribute__
((__warn_unused_result__));

extern void free (void *__ptr) __attribute__ ((__nothrow__ , __leaf__));

typedef struct { long count, flags; } Foo;

Foo *G(Foo *x, long flags, int unused) {
    if (x) { // originally a file descriptor
        x->count = 0; flags = 0;
    } else {
        if (!(x = (Foo *)malloc(sizeof *x))) return (Foo *)0;
        flags = 1;
    }
    x->flags = flags;
    return x;
}

Foo *F(Foo *x, long flags) {
    if (x) { // originally a pathname path
        x->count = 0; flags = 0;
    } else {
        if (!(x = (Foo *)malloc(sizeof *x))) return (Foo *)0;
        flags = 1;
    }
    x->flags = flags;
    return G(x, flags, 123);
}

void release(Foo *x) {
    if (x && x->flags) free(x);
}

int main(void) {
    Foo x;
    if (F(&x, 0))
        release(&x);
    return 0;
}

In the above, variations due to compiler optimization levels make it even more
confusing: -O1 does not warn, -O2 does warn, -O3 again does not warn (on both
gcc-12.3 and gcc-13.2 on Gentoo Linux, anyway). O3 optimizes the whole main
away.  clang never warns on this at any -O level, nor with clang --analyze.

What is particularly bad about this warning is that AFAICT there is no way to
massage the code to reliably silence it while also preserving the intended
effect.  Now it's even on by default.  This actively discourages an otherwise
clean arrangement in the C programming language when working with anyone else
who takes warns too seriously.  Any such warning should have "may be" language
not language that sounds like the compiler has "proved an actuality".

Might the same argument apply to many warnings?  Maybe.  Two words is not so
bad, though.  Other warns may have workarounds like annotations to silence them
reliably without sacrificing functionality.  Maybe I'm missing one here?