[Bug c/106854] New: [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[Bug c/106854] New: [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[colomar.6.4.3 at gmail dot com]

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

            Bug ID: 106854
           Summary: [[gnu::malloc(deallocator)]] for non-pointer functions
                    (e.g., fd)
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: colomar.6.4.3 at gmail dot com
  Target Milestone: ---

Some stuff is allocated and deallocated through non-pointer types.  Most of the
time it's file descriptors, i.e., int.

Since [[gnu::malloc(f)]] is independent of [[gnu::malloc]], it could be used
for such cases:

    int close(int fd);

    [[gnu::malloc(close)]]
    int open(const char *pathname, int flags, ...);

Notice that [[gnu::malloc]] can't be used above.

[[gnu::malloc(f)]] has no reason to be restricted to functions returning
pointers, has it?

Could you allow using it for file descriptors?  Otherwise, a more generic
[[open(close)]] attribute might be reasonable.


Currently, it results in a warning:

$ cat fd.c && echo && cc -Wall -Wextra -S fd.c 
#include <fcntl.h>
#include <unistd.h>

[[gnu::malloc(close)]]
int g(void)
{
        return open("foo", O_RDONLY);
}

fd.c:6:1: warning: ‘malloc’ attribute ignored on functions returning ‘int’;
valid only for pointer return types [-Wattributes]
    6 | {
      | ^

[Bug analyzer/106854] [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |dmalcolm at gcc dot gnu.org
          Component|c                           |analyzer

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Analyzer is a better place to handle this. Maybe not with this attribute but
with a different one.

[Bug analyzer/106854] [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[colomar.6.4.3 at gmail dot com]

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

--- Comment #2 from Alejandro Colomar <colomar.6.4.3 at gmail dot com> ---
Also interesting might be that one function might have more than one closer.

For example, open(2) might be closed by close(2), but it is also closed by
fdopen(3), in the sense that the file descriptor can't be (safely) used again
after that, and also that it can't be passed to close(2) after that --even if
in reality the file descriptor is still valid, for obvious reasons--.

[Bug analyzer/106854] [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[dmalcolm at gcc dot gnu.org]

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

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

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

--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Immad Mir has spent this summer working on adding support for tracking file
descriptors to -fanalyzer for GCC 13 (as part of GSoC 2022), and he's made a
lot of progress; see:
  *
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=97baacba963c06e3d0e33cde04e7e687671e60e7
which adds five new warnings, relating to misuses of file descriptors:
    -Wanalyzer-fd-access-mode-mismatch
    -Wanalyzer-fd-double-close
    -Wanalyzer-fd-leak
    -Wanalyzer-fd-use-after-close
    -Wanalyzer-fd-use-without-check 
and specialcases the analyzer's handling of open, close, read, and write.

  *
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=f8e6e2c046e1015697356ee7079fb39e0cb6add5
which adds three new function attributes for int parameters that refer to file
descriptors

  *
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=6a11f2d974a912aaaedb0ce32cdfde10193003cd
which specialcases the analyzer's handling of creat, dup, dup2 and dup3.

See also:
  https://gcc.gnu.org/bugzilla/showdependencytree.cgi?id=106003&hide_resolved=0
which I hope covers the rest of what you're asking for.

Are we missing anything?

[Bug analyzer/106854] [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[colomar.6.4.3 at gmail dot com]

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

--- Comment #4 from Alejandro Colomar <colomar.6.4.3 at gmail dot com> ---
Hi David,

I was missing that this is to be introduced in GCC 13, which of course I still
don't have; but thanks!  It'll be a great improvement.

Still, this doesn't seem to cover all cases.  See for example the case of

       int timer_create(clockid_t clockid, struct sigevent *restrict sevp,
                        timer_t *restrict timerid);
       int timer_delete(timer_t timerid);

One needs to pair those two functions.

The case with these functions has another problem: the initialized object
(which is an arithmetic type; check clockid_t(3type) --or clockid_t(3) in older
systems--) is returned via a parameter, instead of the return value.

It would be good if a more generic attribute could be used to mark such cases. 
We would need to be careful to accept both pointers and integers, to not
unnecessarily make it unusable in some future use cases, so it could be used
for malloc(3), for open(2), for timer_create(3), and for any other functions
that one may create.

I think the following syntax would make sense:

       [[gnu::init(3, timer_delete, 1)]]
       int timer_create(clockid_t clockid, struct sigevent *restrict sevp,
                        timer_t *restrict timerid);

Where the first argument, 3, refers to the position of the parameter that is
initialized to a unique value; the second refers to the function that
deinitializes it; and the third (optional), refers to the position in the
deinitializer function where the parameter is expected.  For a function like
malloc(3) or open(2), where the initialized value is returned via the return
value, the first argument should be 0.

Does this make sense?

This would superseed the [[gnu::malloc(...)]] attribute, which would be less
confusing (having two different attributes with the same name is confusing,
IMHO).

[Bug analyzer/106854] [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[colomar.6.4.3 at gmail dot com]

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

--- Comment #5 from Alejandro Colomar <colomar.6.4.3 at gmail dot com> ---
We could also keep the old [[gnu::malloc(...)]] attribute, of course, if a new
attribute would be an issue.  We would just have to add an extra argument (the
third?, or one before the function name?) to mark the position of the
initialized object.

Either [[gnu::malloc(3, timerfd_close)]] with an optional third argument of 1,
or [[gnu::malloc(timerfd_close, 1, 3)]] and force to specify the position in
the closer if the position in the initializer needs to be specified.

The second form would probably be easier to implement, and the first one might
be easier to use (having to specify less things).

[Bug analyzer/106854] [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[colomar.6.4.3 at gmail dot com]

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

--- Comment #6 from Alejandro Colomar <colomar.6.4.3 at gmail dot com> ---
timerfd_create() might not be important if the timer is not correctly deleted. 
pthread_mutex_init() is another one that is quite more important, as leaking
such a thing in a multithreaded program will be a pain to debug for sure.  This
attribute could help detect that.

[Bug analyzer/106854] [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[dmalcolm at gcc dot gnu.org]

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

--- Comment #7 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to Alejandro Colomar from comment #6)
> timerfd_create() might not be important if the timer is not correctly
> deleted.  pthread_mutex_init() is another one that is quite more important,
> as leaking such a thing in a multithreaded program will be a pain to debug
> for sure.  This attribute could help detect that.

FWIW I think we'll eventually want to model the pthreads API (e.g. for checking
of locks; see PR 105897), so pthread_mutex_init seems like a function we'd be
better off special-casing, rather than trying to express all of its semantics
with attributes.

[Bug analyzer/106854] [[gnu::malloc(deallocator)]] for non-pointer functions (e.g., fd)

[sam at gentoo dot org]

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

Sam James <sam at gentoo dot org> changed:

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

--- Comment #8 from Sam James <sam at gentoo dot org> ---
Something similarish came up with gnutls recently:
https://gitlab.com/gnutls/gnutls/-/merge_requests/1652#note_1138078435.