Bug 2047926 - gcc: -fanalyzer doesn't understand reallocarray()
Summary: gcc: -fanalyzer doesn't understand reallocarray()
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: gcc
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jakub Jelinek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-01-28 21:22 UTC by Robbie Harwood
Modified: 2022-02-06 18:11 UTC (History)
10 users (show)

Fixed In Version: gcc-12.0.1-0.6.fc36
Clone Of:
Environment:
Last Closed: 2022-02-06 18:11:40 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
reallocarray analyzer problem - processed (29.50 KB, text/plain)
2022-01-31 21:28 UTC, Robbie Harwood
no flags Details
similar realloc problem - processed (84.89 KB, text/plain)
2022-01-31 21:58 UTC, Robbie Harwood
no flags Details


Links
System ID Private Priority Status Summary Last Updated
GNU Compiler Collection 104369 0 P3 RESOLVED False positive from -Wanalyzer-use-of-uninitialized-value with realloc moving buffer 2022-02-03 22:53:29 UTC
GNU Compiler Collection 104370 0 P3 UNCONFIRMED False positive from -Wanalyzer-mismatching-deallocation with reallocarray 2022-02-03 15:16:59 UTC

Description Robbie Harwood 2022-01-28 21:22:19 UTC
[root@rawhide-efi ~]# cat test.c
#include <stdlib.h>

int main() {
    void *foo, *new_foo;

    foo = calloc(200, 200);
    if (!foo)
        return 1;

    new_foo = reallocarray(foo, 201, 200);
    if (!new_foo)
        return 1;
    foo = new_foo;

    free(foo);
    return 0;
}
[root@rawhide-efi ~]# gcc -fanalyzer test.c
test.c: In function ‘main’:
test.c:10:15: warning: ‘foo’ should have been deallocated with ‘free’ but was deallocated with ‘reallocarray’ [CWE-762] [-Wanalyzer-mismatching-deallocation]
   10 |     new_foo = reallocarray(foo, 201, 200);
      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-5
    |
    |    6 |     foo = calloc(200, 200);
    |      |           ^~~~~~~~~~~~~~~~
    |      |           |
    |      |           (1) allocated here (expects deallocation with ‘free’)
    |    7 |     if (!foo)
    |      |        ~   
    |      |        |
    |      |        (2) assuming ‘foo’ is non-NULL
    |      |        (3) following ‘false’ branch (when ‘foo’ is non-NULL)...
    |......
    |   10 |     new_foo = reallocarray(foo, 201, 200);
    |      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |               |
    |      |               (4) ...to here
    |      |               (5) deallocated with ‘reallocarray’ here; allocation at (1) expects deallocation with ‘free’
    |
[root@rawhide-efi ~]# rpm -qv gcc glibc
gcc-12.0.1-0.3.fc36.x86_64
glibc-2.34.9000-38.fc36.x86_64
[root@rawhide-efi ~]# 

(This is minimized code from pesign.)

Comment 1 Robbie Harwood 2022-01-28 21:42:41 UTC
Possibly related is this additional problem:

[root@rawhide-efi ~]# cat test.c
#define _GNU_SOURCE

#include <poll.h>
#include <stdlib.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/un.h>

int main() {
    struct pollfd *pollfds, *newpollfds;
    struct sockaddr_un remote;
    socklen_t len = sizeof(remote);

    pollfds = calloc(1, sizeof(struct pollfd));
    if (!pollfds)
	return 1;

    pollfds[0].fd = 1;

    newpollfds = realloc(pollfds, 2 * sizeof(struct pollfd));
    if (!newpollfds)
	return 1;

    accept(pollfds[0].fd, &remote, &len);

    return 0;
}
[root@rawhide-efi ~]# gcc -O3 -fanalyzer test.c
test.c: In function ‘main’:
test.c:24:5: warning: use after ‘free’ of ‘pollfds’ [CWE-416] [-Wanalyzer-use-after-free]
   24 |     accept(pollfds[0].fd, &remote, &len);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ‘main’: events 1-9
    |
    |   14 |     pollfds = calloc(1, sizeof(struct pollfd));
    |      |               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |               |
    |      |               (1) allocated here
    |   15 |     if (!pollfds)
    |      |        ~       
    |      |        |
    |      |        (2) assuming ‘pollfds’ is non-NULL
    |      |        (3) following ‘false’ branch (when ‘pollfds’ is non-NULL)...
    |......
    |   18 |     pollfds[0].fd = 1;
    |      |     ~~~~~~~~~~~~~~~~~
    |      |                   |
    |      |                   (4) ...to here
    |   19 | 
    |   20 |     newpollfds = realloc(pollfds, 2 * sizeof(struct pollfd));
    |      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                  |
    |      |                  (5) freed here
    |      |                  (6) when ‘realloc’ succeeds, moving buffer
    |   21 |     if (!newpollfds)
    |      |        ~       
    |      |        |
    |      |        (7) following ‘false’ branch (when ‘newpollfds’ is non-NULL)...
    |......
    |   24 |     accept(pollfds[0].fd, &remote, &len);
    |      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |     |
    |      |     (8) ...to here
    |      |     (9) use after ‘free’ of ‘pollfds’; freed at (5)
    |
[root@rawhide-efi ~]# 

(Source continues to be pesign.)

Comment 2 Dave Malcolm 2022-01-28 21:48:37 UTC
(In reply to Robbie Harwood from comment #1)

The report in comment #1 looks to me like the analyzer is correctly reporting a bug in test.c: if realloc succeeds but moves the buffer, then the existing "pollfds" points to freed memory.

Comment 3 Dave Malcolm 2022-01-28 21:53:18 UTC
(In reply to Robbie Harwood from comment #0)
Thanks for filing this; I think the false positive in comment #0 is due to a bad interaction with attributes in the glibc headers.  Please can you attach the preprocessed source (via -E)

Comment 4 Robbie Harwood 2022-01-31 21:28:35 UTC
Created attachment 1858170 [details]
reallocarray analyzer problem - processed

Attached.

Apologies about the second one - I seem to have minimized it poorly.  I'll see if I can get something better out.

Comment 5 Robbie Harwood 2022-01-31 21:58:06 UTC
Created attachment 1858172 [details]
similar realloc problem - processed

Here's that second one, hopefully fixed :)  Source this time is:

#define _GNU_SOURCE

#include <poll.h>
#include <stdlib.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <sys/un.h>
#include <unistd.h>

int main() {
    int rc;
    int nsockets = 1;
    struct pollfd *pollfds, *newpollfds;
    struct sockaddr_un remote;
    socklen_t len = sizeof(remote);

    pollfds = calloc(1, sizeof(struct pollfd));
    if (!pollfds) {
        exit(1);
    }

    while (1) {
        rc = ppoll(pollfds, nsockets, NULL, NULL);
        if (rc < 0) {
            continue;
        }

        if (pollfds[0].revents & POLLIN) {
            nsockets++;
	    newpollfds = realloc(pollfds, nsockets * sizeof(*pollfds));
            if (!newpollfds) {
                exit(1);
            }
            pollfds = newpollfds;
            pollfds[nsockets-1].fd = accept(pollfds[0].fd, &remote,
					    &len);
        }
    }
    return 0;
}

Comment 6 Dave Malcolm 2022-02-02 16:40:05 UTC
(In reply to Robbie Harwood from comment #4)
> Created attachment 1858170 [details]
> reallocarray analyzer problem - processed
> 
> Attached.

Thanks; I can reproduce the false positive with that one.

Looking at attachment 1858170 [details], I see:

# 396 "/usr/include/stdlib.h" 2 3 4

followed by various parts of your <stdlib.h>:

[...snip...]

extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
     __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__warn_unused_result__))
     __attribute__ ((__alloc_size__ (2, 3)))
    __attribute__ ((__malloc__ (__builtin_free, 1)));

extern void *reallocarray (void *__ptr, size_t __nmemb, size_t __size)
     __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__malloc__ (reallocarray, 1)));


where there seem to be two declarations of reallocarray, with two different sets of attributes.  If I delete the 2nd, the false positive goes away.

This seems to be from:
  https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=c1760eaf3b575ad174fd88b252fd16bd525fa818

Martin: why is reallocarray declared twice?  Is it due to wanting to declare two different deallocators?  Looks like our implementations might disagree about the semantics of this attribute (bother)

Comment 7 Martin Sebor 2022-02-02 18:10:39 UTC
The first declaration just forward-declares reallocarray (and associates it with free).  The second one then associates it with itself as its own deallocator.  The two steps are necessary because a declaration needs to be in scope when it's mentioned in an attribute, and one's not yet there when attributes on the first one are being processed.

Comment 8 Dave Malcolm 2022-02-03 14:58:22 UTC
(In reply to Robbie Harwood from comment #5)
> Created attachment 1858172 [details]
> similar realloc problem - processed
> 
> Here's that second one, hopefully fixed :)  Source this time is:

[...snip...]

Thanks.  I don't see a  "use after ‘free’ of ‘pollfds" with that reproducer, but I do see a pair of bogus "-Wanalyzer-use-of-uninitialized-value" warnings.  This seems to be a separate issue from the reallocarray issue, so I've filed the bogus uninit warnings upstream as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104369 and will try to fix them there.

Comment 9 Dave Malcolm 2022-02-03 15:17:00 UTC
(In reply to Martin Sebor from comment #7)
> The first declaration just forward-declares reallocarray (and associates it
> with free).  The second one then associates it with itself as its own
> deallocator.  The two steps are necessary because a declaration needs to be
> in scope when it's mentioned in an attribute, and one's not yet there when
> attributes on the first one are being processed.

Thanks.  I've filed this issue as an analyzer bug with upstream GCC here:
  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104370

Comment 10 Dave Malcolm 2022-02-03 22:55:32 UTC
(In reply to Dave Malcolm from comment #8)
> (In reply to Robbie Harwood from comment #5)
> > Created attachment 1858172 [details]
> > similar realloc problem - processed
> > 
> > Here's that second one, hopefully fixed :)  Source this time is:
> 
> [...snip...]
> 
> Thanks.  I don't see a  "use after ‘free’ of ‘pollfds" with that reproducer,
> but I do see a pair of bogus "-Wanalyzer-use-of-uninitialized-value"
> warnings.  This seems to be a separate issue from the reallocarray issue, so
> I've filed the bogus uninit warnings upstream as
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104369 and will try to fix them
> there.

I've fixed the "uninit on realloc" bug upstream in commit r12-7040-g3ef328c293a336df0aead2d72c0c5ed9781a9861:
  https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=3ef328c293a336df0aead2d72c0c5ed9781a9861

Comment 11 Fedora Update System 2022-02-06 17:40:06 UTC
FEDORA-2022-fba9d09417 has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-fba9d09417

Comment 12 Fedora Update System 2022-02-06 18:11:40 UTC
FEDORA-2022-fba9d09417 has been pushed to the Fedora 36 stable repository.
If problem still persists, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.