Bug 1878600 - -fanalyzer -Wanalyzer-file-leak false-positive if FILE is casted to void *
Summary: -fanalyzer -Wanalyzer-file-leak false-positive if FILE is casted to void *
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Fedora
Classification: Fedora
Component: gcc
Version: 32
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Dave Malcolm
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-09-14 06:19 UTC by Daiki Ueno
Modified: 2020-10-05 08:30 UTC (History)
10 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2020-09-14 16:31:18 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
reproducer (654 bytes, text/x-csrc)
2020-09-14 06:19 UTC, Daiki Ueno
no flags Details

Description Daiki Ueno 2020-09-14 06:19:10 UTC
Created attachment 1714724 [details]
reproducer

Description of problem:
When compiling the attached program with gcc -fanalyzer -c t.c, I get the warning, which I consider a false-positive, because fclose is called on the caller in the failure case:

t.c: In function ‘ini_parse_stream’:
t.c:15:12: warning: leak of FILE ‘stream’ [CWE-775] [-Wanalyzer-file-leak]
   15 |     return 0;
      |            ^
  ‘ini_parse’: events 1-6
    |
    |   23 | int ini_parse(const char* filename)
    |      |     ^~~~~~~~~
    |      |     |
    |      |     (1) entry to ‘ini_parse’
    |......
    |   28 |     file = fopen(filename, "r");
    |      |            ~~~~~~~~~~~~~~~~~~~~
    |      |            |
    |      |            (2) opened here
    |   29 |     if (!file)
    |      |        ~
    |      |        |
    |      |        (3) assuming ‘file’ is non-NULL
    |      |        (4) following ‘false’ branch (when ‘file’ is non-NULL)...
    |   30 |         return -1;
    |   31 |     error = ini_parse_file(file);
    |      |             ~~~~~~~~~~~~~~~~~~~~
    |      |             |
    |      |             (5) ...to here
    |      |             (6) calling ‘ini_parse_file’ from ‘ini_parse’
    |
    +--> ‘ini_parse_file’: events 7-8
           |
           |   18 | static int ini_parse_file(FILE* file)
           |      |            ^~~~~~~~~~~~~~
           |      |            |
           |      |            (7) entry to ‘ini_parse_file’
           |   19 | {
           |   20 |     return ini_parse_stream((ini_reader)fgets, file);
           |      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |            |
           |      |            (8) calling ‘ini_parse_stream’ from ‘ini_parse_file’
           |
           +--> ‘ini_parse_stream’: events 9-12
                  |
                  |    9 | static int ini_parse_stream(ini_reader reader, void* stream)
                  |      |            ^~~~~~~~~~~~~~~~
                  |      |            |
                  |      |            (9) entry to ‘ini_parse_stream’
                  |......
                  |   13 |     while (reader(line, max_line, stream) != NULL)
                  |      |           ~ 
                  |      |           |
                  |      |           (10) following ‘false’ branch...
                  |   14 |      ;
                  |   15 |     return 0;
                  |      |            ~
                  |      |            |
                  |      |            (11) ...to here
                  |      |            (12) ‘stream’ leaks here; was opened at (2)
                  |


Version-Release number of selected component (if applicable):
gcc-10.2.1-1.fc32.x86_64

How reproducible:
always

Steps to Reproduce:
1. gcc -fanalyzer -c t.c
2.
3.

Actual results:
The above warning is shown.

Expected results:
No warning should be shown.

Additional info:
It can be worked around by changing ini_parse_stream to taking a FILE pointer rather than a void pointer.

Comment 1 Jakub Jelinek 2020-09-14 07:12:27 UTC
Why would a routine that expects to work with FILE * pass void * instead?  Is the routine meant to handle different output mechanisms at the same time?  Otherwise it makes no sense to me.

Comment 2 Daiki Ueno 2020-09-14 07:25:01 UTC
Yes, the routine can also take a string and the user supplied callback ("reader") is responsible for the cast. It's the design of the upstream project we copy so we don't want to let the code diverge from it:
https://github.com/benhoyt/inih

Comment 3 Florian Weimer 2020-09-14 08:06:55 UTC
These lines:

    return ini_parse_stream((ini_reader)fgets, file);

and

    while (reader(line, max_line, stream) != NULL)

are undefined because the called fgets function does not have the type that is used to call it. (I don't know if this is what confuses the analyzer.)

In order to make this find, you need to define a wrapper function of the correct type that calls fgets, instead of the cast to ini_reader.

Comment 4 Dave Malcolm 2020-09-14 12:49:57 UTC
Thanks for filing this bug report.

The leak diagnostic does look like a false positive to me.

I've tested it with gcc 10.2 and with upstream git "master" (for gcc 11).

I can reproduce it with gcc 10.2.

gcc 11 is no longer affected: https://godbolt.org/z/df3G6o

It was probably fixed by the big rewrite of state-handling (upstream commit r11-2694-g808f4dfeb3a95f50f15e71148e5c1067f90a126d).

I'll look at adding this case to the upstream regression test suite for -fanalyzer, and then close this out as UPSTREAM.

Comment 5 Dave Malcolm 2020-09-14 16:31:18 UTC
Regression test committed upstream as:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=00adddd65689d995d8bdf306d0850c852ff0fd25


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