[Bug tree-optimization/94527] New: RFE: Add an __attribute__ that marks a function as freeing an object

[Bug tree-optimization/94527] New: RFE: Add an __attribute__ that marks a function as freeing an object

[dhowells at redhat dot com]

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

            Bug ID: 94527
           Summary: RFE: Add an __attribute__ that marks a function as
                    freeing an object
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dhowells at redhat dot com
  Target Milestone: ---

Would it be possible to add a function attribute to indicate that the function
is going to destroy the object a the parameter points to (ie. a free()-like
function)?  Perhaps something like:

    void free(const volatile void *ptr) __attribute__((free(1)));

where the free(N) attribute indicates the parameter number[*].

[*] Note that it's possible that a free()-like function might also take
additional parameters to provide context and that context won't be destroyed.

The compiler could then warn upon future access through the pointer.

[Bug tree-optimization/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[dhowells at redhat dot com]

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

--- Comment #1 from dhowells at redhat dot com <dhowells at redhat dot com> ---
To quote Linus Torvalds,
https://lore.kernel.org/lkml/CAHk-=wg2Vsb0JETo24=Tc-T2drwMopMRfKnc__r5SZ6tEnbwcA@mail.gmail.com/

> Think of it this way: free() doesn't really change the data, it kills
> the lifetime of it. You can't access it afterwards - you can neither
> read it nor write it validly. That is a completely different - and
> independent - operation from writing to it.

Side note: I'd really love to be able to describe that operation, but
there's sadly no such extension.

So the _real_ prototype for 'free()'-like operations should be something like

    void free(const volatile killed void *ptr);

where that "killed" also tells the compiler that the pointer lifetime
is dead, so that using it afterwards is invalid. So that the compiler
could warn us about some of the most trivial use-after-free cases.

Because we've had even those trivially stupid ones

Yes, obviously various analysis systems do exactly that kind of
analysis (and usually go much further), but then it's external things
like coverity etc.

The point being that the lifetime of an object is independent from
being able to write to an object, and the "const" in the "free()" is
not "I promise to not write to it", but "I can accept a constant
pointer".

We've had a number of places in the kernel where we do that kind of
"lifetime" marking explicitly by assigning a NULL (or invalid value)
to the pointer when we free it.

I have this dim memory of us even (long long long ago) trying to use a
#define kfree() ... to do that, but it turns out to be basically
impossible to get the proper "use once" semantics, so it doesn't work
if the argument to kfree() has side effects.

[Bug tree-optimization/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[law at redhat dot com]

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

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at redhat dot com
           Severity|normal                      |enhancement

[Bug tree-optimization/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[torvalds@linux-foundation.org]

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

Linus Torvalds <torvalds@linux-foundation.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |torvalds@linux-foundation.o
                   |                            |rg

--- Comment #2 from Linus Torvalds <torvalds@linux-foundation.org> ---
One silly example of potential for dead store elimination would be
kernel/async.c: async_run_entry_fn(), where we have

        /* 2) remove self from the pending queues */
        spin_lock_irqsave(&async_lock, flags);
        list_del_init(&entry->domain_list);
        list_del_init(&entry->global_list);

        /* 3) free the entry */
        kfree(entry);  
        atomic_dec(&entry_count);

and while it's good form to do "list_del_init()" on those fields in entry, the
fact that we then do "kfree(entry)" right afterwards means that the stores that
re-initialize the entry list are dead.

If gcc knew that "kfree(entry)" de-allocates the entry pointer, it could remove
them, the same way gcc already removes dead stores to automatic variables.

But again: warnings about mis-use are likely more important than DSE. We have
had the silly kinds of bugs where we move code around, and some access remains
after a free. We have good tools (both static and dynamic) to find those
after-the-fact, of course, but the compiler warning about stupidity is even
better.

[Bug tree-optimization/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[law at redhat dot com]

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

Jeffrey A. Law <law at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-04-08
             Status|UNCONFIRMED                 |NEW

--- Comment #3 from Jeffrey A. Law <law at redhat dot com> ---
GCC already knows that free() "kills" the pointed-to memory and should be doing
DSE with that in mind.  It doesn't however know that other functions have
free-like semantics, so it wouldn't do so in for kfree.   I suspect an
attribute that either say "this function is free-like" or "this argument is
free'd" and replacing the cases where we look for BUILT_IN_FREE with checking
for the attribute would be enough to make DSE "just work" in these cases.

With regard to the warnings.  When we were investigating use-after-free and
double-free diagnostics it was our conclusion that do to any kind of reasonable
job you really have to do a whole program analysis.  Otherwise it's just a toy.
 As a result the focal point for those diagnostics is the static analyzer David
Malcolm is working on.

[Bug tree-optimization/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[torvalds@linux-foundation.org]

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

--- Comment #4 from Linus Torvalds <torvalds@linux-foundation.org> ---
(In reply to Jeffrey A. Law from comment #3)
> GCC already knows that free() "kills" the pointed-to memory and should be
> doing DSE with that in mind.  It doesn't however know that other functions
> have free-like semantics, so it wouldn't do so in for kfree. 

Oh, ok, so the logic already exists, just not the interface to tell anybody
else.

I suspect even non-kernel users might have wrappers around free that might be
able to use a "this acts like free()" marker.

> With regard to the warnings.  When we were investigating use-after-free and
> double-free diagnostics it was our conclusion that do to any kind of
> reasonable job you really have to do a whole program analysis.  Otherwise
> it's just a toy.  As a result the focal point for those diagnostics is the
> static analyzer David Malcolm is working on.

Obviously a static analyzer is better.

That said, we've had some stupid bugs wrt kfree(). Things like releasing things
twice in error paths etc.

So yeah, doing it in the compiler isn't going to catch the subtle cases, but
catching the stupid cases early would still be a good thing.

But I also realize that it might not be worth it to you guys. Since you already
effectively have the DSE code, that looks like a much cheaper thing to do.

(And maybe one day somebody will go "I can trivially see use-after-free things
too, and warn about it", so just having the marker might result in the warnings
at some point too).

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|tree-optimization           |middle-end

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Agreed that having an attribute to annotate free-like functions similar to how
we have one for malloc-like funtions would be nice.

How are semantics in the case such a function throws?  Or shall 'free' imply
nothrow and that it doesn't return abnormally (it doesn't call longjmp),
'leaf' would be a bit too strong but possibly easiest to require?

We could also simply say that the pointed to object is released (it's contents
become undefined) upon exiting the function by any means (normal return, EH
or abnormal).

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I can see uses that aren't just "frees the memory", e.g. after fclose and close
any further uses of their argument are probably errors. The close case is
interesting because it's not a pointer argument.

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
   Target Milestone|---                         |11.0
           Assignee|unassigned at gcc dot gnu.org      |msebor at gcc dot gnu.org
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
I've actually been experimenting with this for GCC 11 as an extension of
detecting uninitialized reads from dynamically allocated storage.  My initial
approach is to

1) add a second (optional) argument to attribute malloc to mention the
deallocation function (e.g., free for calloc, malloc, strdup, etc., or fclose
for fopen and fdopen)
2) add the free function attribute as described in comment #0

Besides (or instead of just) detecting uninitialized reads from allocated
storage this approach detects all accesses to freed pointers the same way
-Wreturn-local-addr detects returning addresses of auto variables (i.e., not
just dereferences of the pointers but also plain reads).  In addition, it
detects invalid pairs of calls (such as the free(fopen (...) kind, or the
similar C++ new/delete mismatch), as well as attempts to free pointers known
not to have been returned from an allocation function at all (e.g., pointers to
VLAs or those returned from alloca()).

An interesting question is what to do about functions like freopen and realloc.
 It seems that both would have to be decorated with attribute free as well as
attribute malloc, suggesting that to be fully general, attribute malloc might
need to reference at least two and possibly an arbitrary number of deallocation
functions to pair with each allocation function.

As Jon notes, there are also APIs that operate on non-pointer types.  For
those, a separate pair of analogous attributes like acquire and release might
be appropriate.

So unless someone else is much farther along into a prototype I'll assign this
to myself.

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[torvalds@linux-foundation.org]

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

--- Comment #8 from Linus Torvalds <torvalds@linux-foundation.org> ---
(In reply to Jonathan Wakely from comment #6)
> I can see uses that aren't just "frees the memory", e.g. after fclose and
> close any further uses of their argument are probably errors. The close case
> is interesting because it's not a pointer argument.

I suspect you'll find that if it's not a pointer argument, it ends up being not
very useful.

To take your "close()" example: sure, after calling close, the fd isn't active
any more, and you can't use it for IO. So it's similar to free() in many
respects: the lifetime is over and done with as a file descriptor.

But it's very different in other ways. For example, it is _entirely_ valid to
do something like this:

     close(fd);
     mark_fd_freed(fd);

where the application keeps track of free fd ranges on its own (or it keeps
track of a set of file descriptors it is polling, or whatever).

So you can still validly use the 'fd' value even after you closed it, in ways
that is not really the case with pointers from memory allocators and free().

Dereferencing a pointer after freeing it is basically _always_ a bug, but using
a file descriptor after closing it might not be.

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[dmalcolm at gcc dot gnu.org]

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

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

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

--- Comment #9 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
I hadn't seen this bug, but I've just posted:

  "[PATCH] RFC: add "deallocated_by" attribute for use by analyzer"
  https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555544.html

which does some of the things requested in this RFE, but purely within
-fanalyzer.  -fanalyzer is read-only w.r.t. GCC's internal representation, i.e.
it doesn't affect optimizations such as DSE.  Though I suppose the same
attribute could also be handled by optimization passes.

That patch has some big limitations, as I noted.  I attempted to use this to
mark up:

  extern struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags);
  extern void usb_free_urb(struct urb *urb);

as an acquire/release pair via:

  #define __deallocated_by(f)      __attribute__((deallocated_by(f)));

  extern struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags)
      __deallocated_by(usb_free_urb);

in order to detect CVE-2019-19078, a leak of struct urb in an error-handling
path in linux <= 5.3.11 in drivers/net/wireless/ath/ath10k/usb.c, but I ran
into issues where, without LTO, the analyzer knows nothing about calls to:
  usb_fill_bulk_urb
  usb_anchor_urb
  usb_unanchor_urb
that occur along that path, and so it conservatively assumes it doesn't leak. 
(Caveat: I know nothing about those calls either; I'm a user-space developer
with little knowledge of linux internals).  I can get -fanalyzer to emit a leak
warning on that code path with that patch if I hack out those calls.

[To set expectations: I should mention that my initial implementation of
-fanalyzer in gcc 10 had some major design flaws that mean it couldn't scale;
I've fixed those flaws for gcc 11, and am working hard on scaling it up to be
usable on real-world C when gcc 11 ships, but I don't feel it's there yet.  In
particular, I don't expect the current version in git to be usable with LTO
other than on toy examples without some more fixes.  The -fanalyzer option has
found some bugs, including at least one CVE, but I don't recommend it yet other
than to adventurous early adopters.   As I said, I hope to have it in much
better shape when GCC 11 actually ships in about 6 months time]

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=46181
                 CC|                            |thayer-public at e dot mail.de

--- Comment #10 from Martin Sebor <msebor at gcc dot gnu.org> ---
*** Bug 46181 has been marked as a duplicate of this bug. ***

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[msebor at gcc dot gnu.org]

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

--- Comment #11 from Martin Sebor <msebor at gcc dot gnu.org> ---
Patch: https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559053.html

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[msebor at gcc dot gnu.org]

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

--- Comment #12 from Martin Sebor <msebor at gcc dot gnu.org> ---
David Malcolm: I went with "overloading" attribute malloc in my patch for the
reasons I explained my comments on your patch and in the patch submission
email.  I'm open to changing the name (or the association from the allocator to
the deallocator) so long as it's limited to pointers (with integers and other
handles handled by some other attribute), and provided it lets users specify
the position of the argument in the deallocation function's argument list.

David Howells: If/when you have a chance please comment on the design and let
us know if you have any concerns.

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[cvs-commit at gcc dot gnu.org]

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

--- Comment #13 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Sebor <msebor@gcc.gnu.org>:

https://gcc.gnu.org/g:dce6c58db87ebf7f4477bd3126228e73e4eeee97

commit r11-5732-gdce6c58db87ebf7f4477bd3126228e73e4eeee97
Author: Martin Sebor <msebor@redhat.com>
Date:   Thu Dec 3 15:41:25 2020 -0700

    Add support for detecting mismatched allocation/deallocation calls.

    PR c++/90629 - Support for -Wmismatched-new-delete
    PR middle-end/94527 - Add an __attribute__ that marks a function as freeing
an object

    gcc/ChangeLog:

            PR c++/90629
            PR middle-end/94527
            * builtins.c (access_ref::access_ref): Initialize new member.
            (compute_objsize): Use access_ref::deref.  Handle simple pointer
            assignment.
            (expand_builtin): Remove handling of the free built-in.
            (call_dealloc_argno): Same.
            (find_assignment_location): New function.
            (fndecl_alloc_p): Same.
            (gimple_call_alloc_p): Same.
            (call_dealloc_p): Same.
            (matching_alloc_calls_p): Same.
            (warn_dealloc_offset): Same.
            (maybe_emit_free_warning): Same.
            * builtins.h (struct access_ref): Declare new member.
            (maybe_emit_free_warning): Make extern.  Make use of access_ref.
            Handle -Wmismatched-new-delete.
            * calls.c (initialize_argument_information): Call
            maybe_emit_free_warning.
            * doc/extend.texi (attribute malloc): Update.
            * doc/invoke.texi (-Wfree-nonheap-object): Expand documentation.
            (-Wmismatched-new-delete): Document new option.
            (-Wmismatched-dealloc): Document new option.

    gcc/c-family/ChangeLog:

            PR c++/90629
            PR middle-end/94527
            * c-attribs.c (handle_dealloc_attribute): New function.
            (handle_malloc_attribute): Handle argument forms of attribute.
            * c.opt (-Wmismatched-dealloc): New option.
            (-Wmismatched-new-delete): New option.

    gcc/testsuite/ChangeLog:

            PR c++/90629
            PR middle-end/94527
            * g++.dg/asan/asan_test.cc: Fix a bug.
            * g++.dg/warn/delete-array-1.C: Add expected warning.
            * g++.old-deja/g++.other/delete2.C: Add expected warning.
            * g++.dg/warn/Wfree-nonheap-object-2.C: New test.
            * g++.dg/warn/Wfree-nonheap-object.C: New test.
            * g++.dg/warn/Wmismatched-new-delete.C: New test.
            * g++.dg/warn/Wmismatched-dealloc-2.C: New test.
            * g++.dg/warn/Wmismatched-dealloc.C: New test.
            * gcc.dg/Wmismatched-dealloc.c: New test.
            * gcc.dg/analyzer/malloc-1.c: Prune out expected warning.
            * gcc.dg/attr-malloc.c: New test.
            * gcc.dg/free-1.c: Adjust text of expected warning.
            * gcc.dg/free-2.c: Same.
            * gcc.dg/torture/pr71816.c: Prune out expected warning.
            * gcc.dg/tree-ssa/pr19831-2.c: Add an expected warning.
            * gcc.dg/Wfree-nonheap-object-2.c: New test.
            * gcc.dg/Wfree-nonheap-object-3.c: New test.
            * gcc.dg/Wfree-nonheap-object.c: New test.

    libstdc++-v3/ChangeLog:

            * testsuite/ext/vstring/modifiers/clear/56166.cc: Suppress a false
            positive warning.

[Bug middle-end/94527] RFE: Add an __attribute__ that marks a function as freeing an object

[msebor at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #14 from Martin Sebor <msebor at gcc dot gnu.org> ---
Done for GCC 11.