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?
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.
Thanks for filing this bug. I think -fanalyzer should warn about fclose(NULL), but not for free(NULL).
With the Glibc change there should be an analyzer-null-argument warning now. I guess we can close it as MOVED.
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]
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?
(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.
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