Bug 106003 (analyzer-fd) - RFE: -fanalyzer could complain about misuse of file-descriptors
Summary: RFE: -fanalyzer could complain about misuse of file-descriptors
Status: ASSIGNED
Alias: analyzer-fd
Product: gcc
Classification: Unclassified
Component: analyzer (show other bugs)
Version: 12.0
: P3 normal
Target Milestone: ---
Assignee: Immad Mir
URL:
Keywords: diagnostic, meta-bug
Depends on: 106283 106299 106301 106140 106286 106298 106300 106302 106551 107486
Blocks:
  Show dependency treegraph
 
Reported: 2022-06-16 15:21 UTC by David Malcolm
Modified: 2023-01-10 16:13 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-06-19 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description David Malcolm 2022-06-16 15:21:25 UTC
-fanalyzer could be extended to check POSIX file-descriptor APIs ("int" rather than FILE *).

e.g.
- check for FD leaks
- check for double "close" of a FD (CWE-1341, I believe)
- check for read/write of a closed descriptor
- check for read/write of a descriptor opened for just writing/reading
etc

Immad Mir is looking at this for GSoC 2022.

Example of a "double file-descriptor close" bug:
  http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-13351
albeit in C++.
Comment 1 David Malcolm 2022-06-16 15:30:44 UTC
See also this mailing list thread:
  https://gcc.gnu.org/pipermail/gcc/2022-June/238801.html
Comment 2 GCC Commits 2022-07-02 16:45:28 UTC
The master branch has been updated by Immad Mir <mir@gcc.gnu.org>:

https://gcc.gnu.org/g:97baacba963c06e3d0e33cde04e7e687671e60e7

commit r13-1404-g97baacba963c06e3d0e33cde04e7e687671e60e7
Author: Immad Mir <mirimmad17@gmail.com>
Date:   Sat Jul 2 22:09:37 2022 +0530

    analyzer: implement five new warnings for misuse of POSIX file descriptor APIs [PR106003].
    
    This patch adds a new state machine to the analyzer for checking usage of POSIX file descriptor
    APIs with five new warnings.
    
    It adds:
    - check for FD leaks (CWE 775).
    - check for double "close" of a FD (CWE-1341).
    - check for read/write of a closed file descriptor.
    - check whether a file descriptor was used without being checked for validity.
    - check for read/write of a descriptor opened for just writing/reading.
    
    gcc/ChangeLog:
            PR analyzer/106003
            * Makefile.in (ANALYZER_OBJS): Add sm-fd.o.
            * doc/invoke.texi:  Add -Wanalyzer-fd-double-close, -Wanalyzer-fd-leak,
            -Wanalyzer-fd-access-mode-mismatch, -Wanalyzer-fd-use-without-check,
            -Wanalyzer-fd-use-after-close.
    
    gcc/analyzer/ChangeLog:
            PR analyzer/106003
            * analyzer.opt (Wanalyzer-fd-leak): New option.
            (Wanalyzer-fd-access-mode-mismatch): New option.
            (Wanalyzer-fd-use-without-check): New option.
            (Wanalyzer-fd-double-close): New option.
            (Wanalyzer-fd-use-after-close): New option.
            * sm.h (make_fd_state_machine): New decl.
            * sm.cc (make_checkers): Call make_fd_state_machine.
            * sm-fd.cc: New file.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/106003
            * gcc.dg/analyzer/fd-1.c: New test.
            * gcc.dg/analyzer/fd-2.c: New test.
            * gcc.dg/analyzer/fd-3.c: New test.
            * gcc.dg/analyzer/fd-4.c: New test.
Comment 3 Martin Liška 2022-07-04 08:50:07 UTC
Hello Immad.

I noticed you pushed the git change, but it violates the GNU coding style in some aspects. I see usage of Windows newlines, 8 spaces are not replaced with tabular and there's one minor issue:
/home/marxin/BIG/buildbot/buildworker/marxinbox-gcc-clang/build/gcc/analyzer/sm-fd.cc:545:7: warning: field 'm_invalid' will be initialized after field 'm_valid_read_write' [-Wreorder-ctor]

Moreover, I noticed the patch hasn't been sent to gcc-patches mailing list. Please do so next time before you commit a patch.
Comment 4 Martin Liška 2022-07-04 14:08:18 UTC
> /home/marxin/BIG/buildbot/buildworker/marxinbox-gcc-clang/build/gcc/analyzer/
> sm-fd.cc:545:7: warning: field 'm_invalid' will be initialized after field
> 'm_valid_read_write' [-Wreorder-ctor]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106184
Comment 5 Immad Mir 2022-07-04 14:16:42 UTC
Thanks for bringing that to my attention. I will make the suggested corrections in a follow-up patch.
Comment 6 David Malcolm 2022-07-23 17:40:56 UTC
I don't think we were tracking the RFE for this in bugzilla, so just a note that Immad had now committed his patch for the three new attributes for functions that make use of file descriptors:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=f8e6e2c046e1015697356ee7079fb39e0cb6add5

I've also added a link to this to https://gcc.gnu.org/wiki/StaticAnalyzer

Thanks Immad
Comment 7 David Malcolm 2023-01-10 16:13:21 UTC
For reference, this article (by one of my colleagues) talks about how valgrind can detect file descriptor leaks *dynamically*:
  https://developers.redhat.com/articles/2023/01/09/how-use-valgrind-track-file-descriptors#