[Bug analyzer/109570] New: detect fclose on unopened or NULL files

[Bug analyzer/109570] New: detect fclose on unopened or NULL files

[vanyacpp at gmail dot com]

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

            Bug ID: 109570
           Summary: detect fclose on unopened or NULL files
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: vanyacpp at gmail dot com
  Target Milestone: ---

While cleaning up one not particularly well written program I noticed this code
fragment:

FILE* file = fopen(...);
if (!file)
{
    fclose(file);
    return false;
}

Passing NULL to fclose is undefined behavior. Perhaps -fanalyzer could warn
about code like this?

[Bug analyzer/109570] detect fclose on unopened or NULL files

[vanyacpp at gmail dot com]

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

--- Comment #1 from Ivan Sorokin <vanyacpp at gmail dot com> ---
Generalizing. Perhaps similarly free(NULL) can be detected?

void* obj = malloc(...);
if (!obj)
{
    free(obj);
    return false;
}

Unliky fclose(NULL), free(NULL) is completely well defined operation, but it
does nothing and perhaps should be removed.

[Bug analyzer/109570] detect fclose on unopened or NULL files

[dmalcolm at gcc dot gnu.org]

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

--- Comment #2 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Thanks for filing this bug.

I think -fanalyzer should warn about fclose(NULL), but not for free(NULL).

[Bug analyzer/109570] detect fclose on unopened or NULL files

[xry111 at gcc dot gnu.org]

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

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|https://sourceware.org/pipe |https://sourceware.org/git/
                   |rmail/libc-alpha/2023-April |?p=glibc.git;a=commit;h=71d
                   |/147509.html                |9e0fe766a3c22a730995b9d0249
                   |                            |60970670af

--- Comment #3 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
With the Glibc change there should be an analyzer-null-argument warning now.  I
guess we can close it as MOVED.

[Bug analyzer/109570] detect fclose on unopened or NULL files

[clyon at gcc dot gnu.org]

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

Christophe Lyon <clyon at gcc dot gnu.org> changed:

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

--- Comment #4 from Christophe Lyon <clyon at gcc dot gnu.org> ---
The glibc change is now causing failures in the GCC testsuite:
FAIL: gcc.dg/analyzer/torture/conftest-1.c   -O0  (test for excess errors)
Excess errors:
/gcc/testsuite/gcc.dg/analyzer/torture/conftest-1.c:6:24: warning: use of
possibly-NULL 'f' where non-null expected [CWE-690]
[-Wanalyzer-possible-null-argument]


FAIL: gcc.dg/analyzer/data-model-4.c (test for excess errors)
Excess errors:
/gcc/testsuite/gcc.dg/analyzer/data-model-4.c:11:24: warning: use of
possibly-NULL 'f' where non-null expected [CWE-690]
[-Wanalyzer-possible-null-argument]

[Bug analyzer/109570] detect fclose on unopened or NULL files

[clyon at gcc dot gnu.org]

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

--- Comment #5 from Christophe Lyon <clyon at gcc dot gnu.org> ---
Not sure how to update/fix the testcases though?
Since they get the declaration of fclose from stdio.h, we'd need to make
dg-error conditional to the glibc version in use, which seems unpractical.

Should we instead remove #include <stdio.h> and provide suitable declarations
in the testcase?

[Bug analyzer/109570] detect fclose on unopened or NULL files

[xry111 at gcc dot gnu.org]

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

--- Comment #6 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Christophe Lyon from comment #5)
> Not sure how to update/fix the testcases though?
> Since they get the declaration of fclose from stdio.h, we'd need to make
> dg-error conditional to the glibc version in use, which seems unpractical.
> 
> Should we instead remove #include <stdio.h> and provide suitable
> declarations in the testcase?

I guess we need to change

  return ferror (f) || fclose (f) != 0;

to

  return !f || ferror (f) || fclose (f) != 0;

Because "failing to check if the file is opened successfully" is definitely a
bug, and these tests are intended not to raise warnings for a bug-free program.

BTW ferror(f) segfaults as well when f is NULL, so IMO we should mark it
nonnull in Glibc as well.

[Bug analyzer/109570] detect fclose on unopened or NULL files

[glebfm at altlinux dot org]

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

Gleb Fotengauer-Malinovskiy <glebfm at altlinux dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |glebfm at altlinux dot org

--- Comment #7 from Gleb Fotengauer-Malinovskiy <glebfm at altlinux dot org> ---
FYI, the change in the glibc also has an effect on autoconf-based projects,
when the -fanalyzer flag is used in combination with the -Werror flag, see:

https://git.savannah.gnu.org/cgit/autoconf.git/commit/?id=ea3e0cec2e66132e34228546256a1657c7b9b2e9