Bug 54202 - Overeager warning about freeing non-heap objects
Summary: Overeager warning about freeing non-heap objects
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.7.0
: P3 minor
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: diagnostic
Depends on: 111715
Blocks: Wfree-nonheap-object
  Show dependency treegraph
 
Reported: 2012-08-08 13:35 UTC by Thiago Macieira
Modified: 2023-12-30 02:28 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 10.2.0, 11.0, 4.7.4, 4.8.4, 4.9.4, 5.5.0, 6.4.0, 7.2.0, 8.3.0, 9.1.0
Last reconfirmed: 2021-02-14 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Macieira 2012-08-08 13:35:49 UTC
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.
Comment 1 Richard Biener 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 ...).
Comment 2 Thiago Macieira 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.
Comment 3 Richard Biener 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?
Comment 4 Thiago Macieira 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.
Comment 5 Andrew Pinski 2012-08-08 19:00:58 UTC
Even the printf format warnings are this way too ....
Comment 6 Thiago Macieira 2017-05-30 22:00:43 UTC
ping.

If you can't fix GCC so that it can prove that the free is on a non-heap object, then please change the warning to indicate that GCC may be wrong. For example:

warning: free() may be called with non-heap object 'name'
Comment 7 Martin Sebor 2020-11-03 19:50:34 UTC
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;

}
Comment 8 Giuseppe D'Angelo 2021-05-17 12:09:56 UTC
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
}
Comment 9 Serdar Sanli 2021-06-16 13:19:06 UTC
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);
}
Comment 10 Martin Sebor 2021-06-16 16:35:18 UTC
(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.
Comment 11 Charles Blake 2023-08-25 11:46:09 UTC
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?