* [PATCH] analyzer PR 106003
@ 2022-06-23 18:30 Mir Immad
2022-06-23 23:20 ` David Malcolm
0 siblings, 1 reply; 8+ messages in thread
From: Mir Immad @ 2022-06-23 18:30 UTC (permalink / raw)
To: gcc, David Malcolm
diff --git gcc/Makefile.in gcc/Makefile.in
index b6dcc45a58a..04631f737ea 100644
--- gcc/Makefile.in
+++ gcc/Makefile.in
@@ -1269,6 +1269,7 @@ ANALYZER_OBJS = \
analyzer/region-model-reachability.o \
analyzer/sm.o \
analyzer/sm-file.o \
+ analyzer/sm-fd.o \
analyzer/sm-malloc.o \
analyzer/sm-pattern-test.o \
analyzer/sm-sensitive.o \
diff --git gcc/analyzer/analyzer.opt gcc/analyzer/analyzer.opt
index 23dfc797cea..e2a629bb42c 100644
--- gcc/analyzer/analyzer.opt
+++ gcc/analyzer/analyzer.opt
@@ -66,6 +66,26 @@ Wanalyzer-exposure-through-output-file
Common Var(warn_analyzer_exposure_through_output_file) Init(1) Warning
Warn about code paths in which sensitive data is written to a file.
+Wanalyzer-fd-access-mode-mismatch
+Common Var(warn_analyzer_fd_mode_mismatch) Init(1) Warning
+Warn about code paths in which read on write-only file descriptor is
attempted, or vice versa.
+
+Wanalyzer-fd-double-close
+Common Var(warn_analyzer_fd_double_close) Init(1) Warning
+Warn about code paths in which a file descriptor can be closed more than
once.
+
+Wanalyzer-fd-leak
+Common Var(warn_analyzer_fd_leak) Init(1) Warning
+Warn about code paths in which a file descriptor is not closed.
+
+Wanalyzer-fd-use-after-close
+Common Var(warn_analyzer_fd_use_after_close) Init(1) Warning
+Warn about code paths in which a read or write is performed on a closed
file descriptor.
+
+Wanalyzer-fd-use-without-check
+Common Var(warn_analyzer_fd_use_without_check) Init(1) Warning
+warn about code paths in which a possibly invalid file descriptor is
passed to a must-be-a-valid file descriptor function argument.
+
Wanalyzer-file-leak
Common Var(warn_analyzer_file_leak) Init(1) Warning
Warn about code paths in which a stdio FILE is not closed.
diff --git gcc/analyzer/sm-fd.cc gcc/analyzer/sm-fd.cc
new file mode 100644
index 00000000000..048014d7a26
--- /dev/null
+++ gcc/analyzer/sm-fd.cc
@@ -0,0 +1,770 @@
+/* A state machine for detecting misuses of POSIX file descriptor APIs.
+ Copyright (C) 2019-2022 Free Software Foundation, Inc.
+ Contributed by Immad Mir <mirimmad17@gmail.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "function.h"
+#include "basic-block.h"
+#include "gimple.h"
+#include "options.h"
+#include "diagnostic-path.h"
+#include "diagnostic-metadata.h"
+#include "function.h"
+#include "json.h"
+#include "analyzer/analyzer.h"
+#include "diagnostic-event-id.h"
+#include "analyzer/analyzer-logging.h"
+#include "analyzer/sm.h"
+#include "analyzer/pending-diagnostic.h"
+#include "analyzer/function-set.h"
+#include "analyzer/analyzer-selftests.h"
+#include "tristate.h"
+#include "selftest.h"
+#include "analyzer/call-string.h"
+#include "analyzer/program-point.h"
+#include "analyzer/store.h"
+#include "analyzer/region-model.h"
+
+#include <unistd.h>
+#if ENABLE_ANALYZER
+
+namespace ana
+{
+
+namespace
+{
+
+/* An enum for distinguishing between three different access modes. */
+
+enum access_mode
+{
+ READ_WRITE,
+ READ_ONLY,
+ WRITE_ONLY
+};
+
+class fd_state_machine : public state_machine
+{
+public:
+ fd_state_machine (logger *logger);
+
+ bool
+ inherited_state_p () const final override
+ {
+ return false;
+ }
+
+ state_machine::state_t
+ get_default_state (const svalue *sval) const final override
+ {
+ if (tree cst = sval->maybe_get_constant ())
+ {
+ int val = TREE_INT_CST_LOW (cst);
+ if (val < 0)
+ {
+ return m_invalid;
+ }
+ }
+ return m_start;
+ }
+
+ bool on_stmt (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt) const final override;
+
+ void on_condition (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const svalue *lhs, const
tree_code op,
+ const svalue *rhs) const final override;
+
+ bool can_purge_p (state_t s) const final override;
+ pending_diagnostic *on_leak (tree var) const final override;
+
+ bool is_unchecked (state_t s) const;
+ bool is_valid (state_t s) const;
+ const char *get_string_for_access_mode (enum access_mode) const;
+ enum access_mode get_access_mode_from_flag (int flag) const;
+ enum access_mode get_access_mode_from_state (state_t s) const;
+ bool check_for_closed_fd (state_t s) const;
+
+ /* States representing a file descriptor that hasn't yet been
+ checked for validity after opening, for three different
+ access modes. */
+ state_t m_unchecked_read_write;
+
+ state_t m_unchecked_read_only;
+
+ state_t m_unchecked_write_only;
+
+ /* State for reperesenting a file descriptor that is known to be valid
(>= 0),
+ for three different access */
+ state_t m_valid_read_write;
+
+ state_t m_valid_read_only;
+
+ state_t m_valid_write_only;
+
+ /* State for a file descriptor that is known to be invalid (< 0). */
+ state_t m_invalid;
+
+ /* State for a file descriptor that has been closed.*/
+ state_t m_closed;
+
+ /* State for a file descriptor that we do not want to track anymore . */
+ state_t m_stop;
+
+private:
+ void on_open (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call) const;
+ void on_close (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call, const tree callee_fndecl) const;
+ void on_read (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call, const tree callee_fndecl) const;
+ void on_write (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call, const tree callee_fndecl) const;
+};
+
+/* Base diagnostic class relative to fd_state_machine. */
+class fd_diagnostic : public pending_diagnostic
+{
+public:
+ fd_diagnostic (const fd_state_machine &sm, tree arg) : m_sm (sm), m_arg
(arg)
+ {
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ return same_tree_p (m_arg, ((const fd_diagnostic &)base_other).m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (change.m_old_state == m_sm.get_start_state ()
+ && m_sm.is_unchecked (change.m_new_state))
+ {
+ if (change.m_new_state == m_sm.m_unchecked_read_write)
+ return change.formatted_print ("opened here as %<read-write%>");
+
+ if (change.m_new_state == m_sm.m_unchecked_read_only)
+ return change.formatted_print ("opened here as %<read-only%>");
+
+ if (change.m_new_state == m_sm.m_unchecked_write_only)
+ return change.formatted_print ("opened here as %<write-only%>");
+ }
+
+ if (change.m_new_state == m_sm.m_closed)
+ return change.formatted_print ("closed here");
+
+ if (m_sm.is_unchecked (change.m_old_state)
+ && m_sm.is_valid (change.m_new_state))
+ if (change.m_expr)
+ return change.formatted_print (
+ "assuming %qE is a valid file descriptor (>= 0)",
change.m_expr);
+ else
+ return change.formatted_print ("assuming a valid file descriptor");
+
+ if (m_sm.is_unchecked (change.m_old_state)
+ && change.m_new_state == m_sm.m_invalid)
+ if (change.m_expr)
+ return change.formatted_print (
+ "assuming %qE is an invalid file descriptor (< 0)",
change.m_expr);
+ else
+ return change.formatted_print ("assuming an invalid file
descriptor");
+
+ return label_text ();
+ }
+
+protected:
+ const fd_state_machine &m_sm;
+ tree m_arg;
+};
+
+class fd_leak : public fd_diagnostic
+{
+public:
+ fd_leak (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
{}
+
+ const char *
+ get_kind () const final override
+ {
+ return "fd_leak";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_leak;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ /*CWE-775: Missing Release of File Descriptor or Handle after Effective
+ * Lifetime
+ */
+ diagnostic_metadata m;
+ m.add_cwe (775);
+ if (m_arg)
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "leak of file descriptor %qE", m_arg);
+ else
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "leak of file descriptor");
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) final override
+ {
+ if (m_sm.is_unchecked (change.m_new_state))
+ {
+ m_open_event = change.m_event_id;
+ return label_text::borrow ("opened here");
+ }
+
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ if (m_open_event.known_p ())
+ {
+ if (ev.m_expr)
+ return ev.formatted_print ("%qE leaks here; was opened at %@",
+ ev.m_expr, &m_open_event);
+ else
+ return ev.formatted_print ("leaks here; was opened at %@",
+ &m_open_event);
+ }
+ else
+ {
+ if (ev.m_expr)
+ return ev.formatted_print ("%qE leaks here", ev.m_expr);
+ else
+ return ev.formatted_print ("leaks here");
+ }
+ }
+
+private:
+ diagnostic_event_id_t m_open_event;
+};
+
+class fd_access_mode_mismatch : public fd_diagnostic
+{
+public:
+ /* mode specfies whether read on write was attempted or vice versa.
+ */
+ fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
+ enum access_mode mode, const tree callee_fndecl)
+ : m_mode (mode), m_callee_fndecl (callee_fndecl), fd_diagnostic (sm,
arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "fd_access_mode_mismatch";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_access_mode_mismatch;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on %qs file descriptor %qE", m_callee_fndecl,
+ m_sm.get_string_for_access_mode (m_mode), m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ return ev.formatted_print ("%qE on %qs file descriptor %qE",
+ m_callee_fndecl,
+ m_sm.get_string_for_access_mode (m_mode),
m_arg);
+ }
+
+private:
+ enum access_mode m_mode;
+ const tree m_callee_fndecl;
+};
+
+class double_close : public fd_diagnostic
+{
+public:
+ double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm,
arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "double_close";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_double_close;
+ }
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ diagnostic_metadata m;
+ // CWE-1341: Multiple Releases of Same Resource or Handle
+ m.add_cwe (1341);
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "double %<close%> of file descriptor %qE", m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (m_sm.is_unchecked (change.m_new_state))
+ return label_text::borrow ("opened here");
+
+ if (change.m_new_state == m_sm.m_closed)
+ {
+ m_first_close_event = change.m_event_id;
+ return change.formatted_print ("first %qs here", "close");
+ }
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ if (m_first_close_event.known_p ())
+ return ev.formatted_print ("second %qs here; first %qs was at %@",
+ "close", "close", &m_first_close_event);
+ return ev.formatted_print ("second %qs here", "close");
+ }
+
+private:
+ diagnostic_event_id_t m_first_close_event;
+};
+
+class fd_use_after_close : public fd_diagnostic
+{
+public:
+ fd_use_after_close (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl)
+ : m_callee_fndecl (callee_fndecl), fd_diagnostic (sm, arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "fd_use_after_close";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_use_after_close;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on closed file descriptor %qE",
m_callee_fndecl,
+ m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (m_sm.is_unchecked (change.m_new_state))
+ {
+ return label_text::borrow ("opened here");
+ }
+
+ if (change.m_new_state == m_sm.m_closed)
+ {
+ return change.formatted_print ("closed here");
+ }
+
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ return ev.formatted_print ("%qE on closed file descriptor %qE here",
+ m_callee_fndecl, m_arg);
+ }
+
+private:
+ const tree m_callee_fndecl;
+};
+
+class unchecked_use_of_fd : public fd_diagnostic
+{
+public:
+ unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl)
+ : m_callee_fndecl (callee_fndecl), fd_diagnostic (sm, arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "unchecked_use_of_fd";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_use_without_check;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on possibly invalid file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (m_sm.is_unchecked (change.m_new_state))
+ {
+ m_first_open_event = change.m_event_id;
+ return label_text::borrow ("opened here");
+ }
+
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ return ev.formatted_print ("%qE could be invalid: unchecked value from
%@",
+ m_arg, &m_first_open_event);
+ }
+
+private:
+ diagnostic_event_id_t m_first_open_event;
+ const tree m_callee_fndecl;
+};
+
+fd_state_machine::fd_state_machine (logger *logger)
+ : state_machine ("file-descriptor", logger),
+ m_unchecked_read_write (add_state ("fd-unchecked-read-write")),
+ m_unchecked_read_only (add_state ("fd-unchecked-read-only")),
+ m_unchecked_write_only (add_state ("fd-unchecked-write-only")),
+ m_invalid (add_state ("fd-invalid")),
+ m_valid_read_write (add_state ("fd-valid-read-write")),
+ m_valid_read_only (add_state ("fd-valid-read-only")),
+ m_valid_write_only (add_state ("fd-valid-write-only")),
+ m_closed (add_state ("fd-closed")), m_stop (add_state ("fd-stop"))
+{
+}
+
+bool
+fd_state_machine::is_unchecked (state_t s) const
+{
+ return s == m_unchecked_read_write
+ || s == m_unchecked_read_only
+ || s == m_unchecked_write_only;
+}
+
+bool
+fd_state_machine::is_valid (state_t s) const
+{
+ return s == m_valid_read_write
+ || s == m_valid_read_only
+ || s == m_valid_write_only;
+}
+
+const char *
+fd_state_machine::get_string_for_access_mode (enum access_mode mode) const
+{
+ if (mode == READ_ONLY)
+ return "read-only";
+ else if (mode == WRITE_ONLY)
+ return "write-only";
+ else
+ return "read-write";
+}
+
+enum access_mode
+fd_state_machine::get_access_mode_from_flag (int flag) const
+{
+ /* FIXME: this code assumes the access modes on the host and
+ target are the same, which in practice might not be the case. */
+ if (flag == O_RDONLY)
+ {
+ return READ_ONLY;
+ }
+ else if (flag == O_WRONLY)
+ {
+ return WRITE_ONLY;
+ }
+ return READ_WRITE;
+}
+
+enum access_mode
+fd_state_machine::get_access_mode_from_state (state_t state) const
+{
+ if (state == m_unchecked_read_only || state == m_valid_read_only)
+ return READ_ONLY;
+ else if (state == m_unchecked_write_only || state == m_valid_write_only)
+ return WRITE_ONLY;
+ else if (state == m_unchecked_read_write || state == m_valid_write_only)
+ return READ_WRITE;
+}
+
+bool
+fd_state_machine::check_for_closed_fd (state_t state) const
+{
+ return (state == m_closed);
+}
+
+bool
+fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt) const
+{
+ if (const gcall *call = dyn_cast<const gcall *> (stmt))
+ if (tree callee_fndecl = sm_ctxt->get_fndecl_for_call (call))
+ {
+ if (is_named_call_p (callee_fndecl, "open", call, 2))
+ {
+ on_open (sm_ctxt, node, stmt, call);
+ return true;
+ } // "open"
+
+ if (is_named_call_p (callee_fndecl, "close", call, 1))
+ {
+ on_close (sm_ctxt, node, stmt, call, callee_fndecl);
+ return true;
+ } // "close"
+
+ if (is_named_call_p (callee_fndecl, "write", call, 3))
+ {
+ on_write (sm_ctxt, node, stmt, call, callee_fndecl);
+ return true;
+ } // "write"
+
+ if (is_named_call_p (callee_fndecl, "read", call, 3))
+ {
+ on_read (sm_ctxt, node, stmt, call, callee_fndecl);
+ return true;
+ } // "read"
+
+ return true;
+ }
+
+ return false;
+}
+
+void
+fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call) const
+{
+ tree lhs = gimple_call_lhs (call);
+ if (lhs)
+ {
+ tree arg = gimple_call_arg (call, 1);
+ if (TREE_CODE (arg) == INTEGER_CST)
+ {
+ int flag = TREE_INT_CST_LOW (arg);
+ enum access_mode mode = get_access_mode_from_flag (flag);
+ if (mode == READ_ONLY)
+ sm_ctxt->on_transition (node, stmt, lhs, m_start,
+ m_unchecked_read_only);
+ else if (mode == WRITE_ONLY)
+ sm_ctxt->on_transition (node, stmt, lhs, m_start,
+ m_unchecked_write_only);
+ else
+ sm_ctxt->on_transition (node, stmt, lhs, m_start,
+ m_unchecked_read_write);
+ }
+ }
+ else
+ {
+ /* FIXME: add leak reporting */
+ }
+}
+
+void
+fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl) const
+{
+ tree arg = gimple_call_arg (call, 0);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+
+ sm_ctxt->on_transition (node, stmt, arg, m_start, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_write,
m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_only,
m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_unchecked_write_only,
m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_valid_read_write, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_valid_read_only, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_valid_write_only, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_invalid, m_closed);
+
+ if (check_for_closed_fd (state))
+ {
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ sm_ctxt->warn (node, stmt, arg, new double_close (*this, diag_arg));
+ sm_ctxt->set_next_state (stmt, arg, m_stop);
+ }
+}
+void
+fd_state_machine::on_read (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl) const
+{
+ tree arg = gimple_call_arg (call, 0);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ enum access_mode mode = get_access_mode_from_state (state);
+
+ if (check_for_closed_fd (state))
+ {
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_after_close (*this, diag_arg,
callee_fndecl));
+ }
+
+ if (!check_for_closed_fd (state))
+ {
+
+ if (!is_valid (sm_ctxt->get_state (stmt, arg)))
+ {
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
+ }
+
+ if (mode == WRITE_ONLY)
+ {
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg,
mode,
+ callee_fndecl));
+ }
+ }
+}
+void
+fd_state_machine::on_write (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl) const
+{
+ tree arg = gimple_call_arg (call, 0);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ enum access_mode mode = get_access_mode_from_state (state);
+
+ if (check_for_closed_fd (state))
+ {
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_after_close (*this, diag_arg,
callee_fndecl));
+ }
+
+ if (!check_for_closed_fd (state))
+ {
+
+ if (!is_valid (sm_ctxt->get_state (stmt, arg)))
+ {
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
+ }
+
+ if (mode == READ_ONLY)
+ {
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg,
mode,
+ callee_fndecl));
+ }
+ }
+}
+
+void
+fd_state_machine::on_condition (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const svalue *lhs,
+ enum tree_code op, const svalue *rhs) const
+{
+ if (!rhs->all_zeroes_p ())
+ return;
+
+ if (op == GE_EXPR)
+ {
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_write,
+ m_valid_read_write);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_only,
+ m_valid_read_only);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_write_only,
+ m_valid_write_only);
+ }
+ else if (op == LT_EXPR)
+ {
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_write,
+ m_invalid);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_only,
+ m_invalid);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_write_only,
+ m_invalid);
+ }
+}
+
+bool
+fd_state_machine::can_purge_p (state_t s) const
+{
+ if (is_unchecked (s) || is_valid (s))
+ return false;
+ else
+ return true;
+}
+
+pending_diagnostic *
+fd_state_machine::on_leak (tree var) const
+{
+ return new fd_leak (*this, var);
+}
+} // namespace
+
+state_machine *
+make_fd_state_machine (logger *logger)
+{
+ return new fd_state_machine (logger);
+}
+} // namespace ana
+
+#endif // ENABLE_ANALYZER
\ No newline at end of file
diff --git gcc/analyzer/sm.cc gcc/analyzer/sm.cc
index 622cb0b7ab3..24c20b894cd 100644
--- gcc/analyzer/sm.cc
+++ gcc/analyzer/sm.cc
@@ -167,6 +167,7 @@ make_checkers (auto_delete_vec <state_machine> &out,
logger *logger)
{
out.safe_push (make_malloc_state_machine (logger));
out.safe_push (make_fileptr_state_machine (logger));
+ out.safe_push (make_fd_state_machine (logger));
/* The "taint" checker must be explicitly enabled (as it currently
leads to state explosions that stop the other checkers working). */
if (flag_analyzer_checker)
diff --git gcc/analyzer/sm.h gcc/analyzer/sm.h
index 4cc54531c56..e80ef1fac37 100644
--- gcc/analyzer/sm.h
+++ gcc/analyzer/sm.h
@@ -301,6 +301,7 @@ extern state_machine *make_sensitive_state_machine
(logger *logger);
extern state_machine *make_signal_state_machine (logger *logger);
extern state_machine *make_pattern_test_state_machine (logger *logger);
extern state_machine *make_va_list_state_machine (logger *logger);
+extern state_machine *make_fd_state_machine (logger *logger);
} // namespace ana
diff --git gcc/testsuite/gcc.dg/analyzer/fd-1.c
gcc/testsuite/gcc.dg/analyzer/fd-1.c
new file mode 100644
index 00000000000..985e9ac75de
--- /dev/null
+++ gcc/testsuite/gcc.dg/analyzer/fd-1.c
@@ -0,0 +1,26 @@
+int open(const char *, int mode);
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDONLY); /* { dg-message "\\(1\\) opened here" }
*/
+ return; /* { dg-warning "leak of file descriptor 'fd' \\\[CWE-775\\\]"
"warning" } */
+ /* { dg-message "\\(2\\) 'fd' leaks here; was opened at \\(1\\)" "event"
{ target *-*-* } .-1 } */
+}
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" } */
+ if (fd >= 0) /* { dg-message "\\(2\\) assuming 'fd' is a valid file
descriptor" "event1" } */
+ /* { dg-message "\\(3\\) following 'true' branch \\(when 'fd >=
0'\\)..." "event2" { target *-*-* } .-1 } */
+ {
+ return; /* { dg-warning "leak of file descriptor 'fd' \\\[CWE-775\\\]"
"warning" } */
+ /* { dg-message "\\(4\\) ...to here" "event1" { target *-*-* } .-1 } */
+ /* { dg-message "\\(5\\) 'fd' leaks here; was opened at \\(1\\)"
"event2" { target *-*-* } .-2 } */
+ }
+}
+
diff --git gcc/testsuite/gcc.dg/analyzer/fd-2.c
gcc/testsuite/gcc.dg/analyzer/fd-2.c
new file mode 100644
index 00000000000..094a3db1b44
--- /dev/null
+++ gcc/testsuite/gcc.dg/analyzer/fd-2.c
@@ -0,0 +1,27 @@
+int open(const char *, int mode);
+void close(int fd);
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ close (fd); /* { dg-message "\\(2\\) first 'close' here" "event1" } */
+ close (fd); /* { dg-warning "double 'close' of file descriptor 'fd'
\\\[CWE-1341\\\]" "warning" } */
+ /* { dg-message "\\(3\\) second 'close' here; first 'close' was at
\\(2\\)" "event2" { target *-*-* } .-1 } */
+}
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ if (fd < 0) /* { dg-message "\\(2\\) assuming 'fd' is a valid file
descriptor \\(>= 0\\)" "event1" } */
+ /* { dg-message "\\(3\\) following 'false' branch \\(when 'fd >=
0'\\)..." "event2" { target *-*-* } .-1 } */
+ return;
+ close (fd); /* { dg-message "\\(4\\) ...to here" "event1" } */
+ /* { dg-message "\\(5\\) first 'close' here" "event2" { target *-*-* }
.-1 } */
+ close (fd); /* { dg-warning "double 'close' of file descriptor 'fd'
\\\[CWE-1341\\\]" "warning" } */
+ /* {dg-message "\\(6\\) second 'close' here; first was at \\(5\\)" ""
{ target *-*-* } .-1 } */
+}
\ No newline at end of file
diff --git gcc/testsuite/gcc.dg/analyzer/fd-3.c
gcc/testsuite/gcc.dg/analyzer/fd-3.c
new file mode 100644
index 00000000000..d32e3fe4923
--- /dev/null
+++ gcc/testsuite/gcc.dg/analyzer/fd-3.c
@@ -0,0 +1,27 @@
+int open(const char *, int mode);
+void close(int fd);
+int write (int fd, void *buf, int nbytes);
+int read (int fd, void *buf, int nbytes);
+
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+
+
+void
+test_1 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ write (fd, buf, 1); /* { dg-message "\\(2\\) 'fd' could be invalid:
unchecked value from \\(1\\)" } */
+ /* { dg-warning "'write' on possibly invalid file descriptor 'fd'"
"warning" { target *-*-* } .-1 } */
+ close(fd);
+}
+
+void
+test_2 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ read (fd, buf, 1); /* { dg-message "\\(2\\) 'fd' could be invalid:
unchecked value from \\(1\\)" } */
+ /* { dg-warning "'read' on possibly invalid file descriptor 'fd'"
"warning" { target *-*-* } .-1 } */
+ close (fd);
+}
diff --git gcc/testsuite/gcc.dg/analyzer/fd-4.c
gcc/testsuite/gcc.dg/analyzer/fd-4.c
new file mode 100644
index 00000000000..7eaa0c10d0d
--- /dev/null
+++ gcc/testsuite/gcc.dg/analyzer/fd-4.c
@@ -0,0 +1,60 @@
+int open(const char *, int mode);
+void close(int fd);
+int write (int fd, void *buf, int nbytes);
+int read (int fd, void *buf, int nbytes);
+
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+
+
+void
+test_1 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDONLY); /* { dg-message "opened here as
'read-only'" } */
+ if (fd >= 0) /* { dg-message "assuming 'fd' is a valid file descriptor
\\(>= 0\\)" "event1" } */
+ /* { dg-message "following 'true' branch \\(when 'fd >= 0'\\)..."
"event2" { target *-*-* } .-1 } */
+ {
+ write (fd, buf, 1); /* { dg-warning "'write' on 'read-only' file
descriptor 'fd'" "warning" } */
+ /* { dg-message "\\(4\\) ...to here" "event1" { target *-*-* } .-1
} */
+ /* { dg-message "\\(5\\) 'write' on 'read-only' file descriptor
'fd'" "event2" { target *-*-* } .-2 } */
+ close (fd);
+ }
+}
+
+void
+test_2 (const char *path, void *buf)
+{
+ int fd = open (path, O_WRONLY); /* { dg-message "opened here as
'write-only'" } */
+ if (fd >= 0) /* { dg-message "assuming 'fd' is a valid file descriptor
\\(>= 0\\)" "event1" } */
+ /* { dg-message "following 'true' branch \\(when 'fd >= 0'\\)..."
"event2" { target *-*-* } .-1 } */
+ {
+ read (fd, buf, 1); /* { dg-warning "'read' on 'write-only' file
descriptor 'fd'" "warning" } */
+ /* { dg-message "\\(4\\) ...to here" "event1" { target *-*-* } .-1
} */
+ /* { dg-message "\\(5\\) 'read' on 'write-only' file descriptor
'fd'" "event2" { target *-*-* } .-2 } */
+ close (fd);
+ }
+}
+
+
+void test_4 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ if (fd >= 0)
+ {
+ close(fd); /* {dg-message "\\(2\\) closed here"} */
+ read(fd, buf, 1); /* { dg-warning "'read' on closed file
descriptor 'fd'" } */
+ /* {dg-message "\\(3\\) 'read' on closed file descriptor 'fd'
here" "" {target *-*-*} .-1 } */
+ }
+}
+
+void test_5 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ if (fd >= 0)
+ {
+ close(fd); /* {dg-message "\\(2\\) closed here"} */
+ write(fd, buf, 1); /* { dg-warning "'write' on closed file
descriptor 'fd'" } */
+ /* {dg-message "\\(3\\) 'write' on closed file descriptor 'fd'
here" "" {target *-*-*} .-1 } */
+ }
+}
\ No newline at end of file
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] analyzer PR 106003
2022-06-23 18:30 [PATCH] analyzer PR 106003 Mir Immad
@ 2022-06-23 23:20 ` David Malcolm
2022-07-14 15:40 ` David Malcolm
0 siblings, 1 reply; 8+ messages in thread
From: David Malcolm @ 2022-06-23 23:20 UTC (permalink / raw)
To: Mir Immad, gcc
On Fri, 2022-06-24 at 00:00 +0530, Mir Immad wrote:
Thanks for the updated patch.
This is close to being ready.
One big issue is the target hook idea for isolating the target's
definition of the O_* flags as per Joseph's suggestion here:
https://gcc.gnu.org/pipermail/gcc/2022-June/238961.html
Given Joseph's comment here:
https://gcc.gnu.org/pipermail/gcc/2022-June/238954.html
I'm inclined to think that the target hook stuff could be done as a
followup.
Indentation has gone a bit wrong (presumably due to issue with the
mailer); the main thing to watch out for is to use tabs for 8 space
indentations within the file. Enabling "visual whitespace" (or
whatever it's called) in your editor can help with this.
Every patch that adds any options (e.g. to gcc/analyzer/analyzer.opt)
needs to document those options, in gcc/doc/invoke.texi. Grep for one
of the existing analyzer options in invoke.texi to see what you need to
add for the new options.
You'll need a ChangeLog entry before this can be pushed. For better or
worse, the project requires ChangeLog entries. You can
use the script ./contrib/mklog.py to help generate this. Some more
information is here:
https://www.gnu.org/prep/standards/html_node/Change-Logs.html
In GCC our ChangeLog entries are part of the git commit message, and we
have a serverside script to update the ChangeLog files in the source
tree (to avoid constantly having merge conflicts).
The Subject line for the commit should have an "analyzer: " prefix
and reference the bugzilla problem report (PR 106003)
Before you can push the patch, you'll have to test it with a full
bootstrap and regression test run. Let me know if you need help with
that.
Various other comments inline below...
[...snip...]
> diff --git gcc/analyzer/analyzer.opt gcc/analyzer/analyzer.opt
> index 23dfc797cea..e2a629bb42c 100644
> --- gcc/analyzer/analyzer.opt
> +++ gcc/analyzer/analyzer.opt
[...snip...]
> +Wanalyzer-fd-use-without-check
> +Common Var(warn_analyzer_fd_use_without_check) Init(1) Warning
> +warn about code paths in which a possibly invalid file descriptor is
> passed to a must-be-a-valid file descriptor function argument.
These should be capitalized as English sentences, and the "must-be-a-
valid" wording doesn't feel quite right to me.
Reword this to something like:
"Warn about code paths in which a file descriptor is used without being
checked for validity."
or somesuch (all on one line).
[...snip...]
> diff --git gcc/analyzer/sm-fd.cc gcc/analyzer/sm-fd.cc
> new file mode 100644
> index 00000000000..048014d7a26
> --- /dev/null
> +++ gcc/analyzer/sm-fd.cc
> @@ -0,0 +1,770 @@
> +/* A state machine for detecting misuses of POSIX file descriptor APIs.
> + Copyright (C) 2019-2022 Free Software Foundation, Inc.
> + Contributed by Immad Mir <mirimmad17@gmail.com>.
> +
> +This file is part of GCC.
> +
> +GCC is free software; you can redistribute it and/or modify it
> +under the terms of the GNU General Public License as published by
> +the Free Software Foundation; either version 3, or (at your option)
> +any later version.
> +
> +GCC is distributed in the hope that it will be useful, but
> +WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with GCC; see the file COPYING3. If not see
> +<http://www.gnu.org/licenses/>. */
> +
> +#include "config.h"
> +#include "system.h"
> +#include "coretypes.h"
> +#include "tree.h"
> +#include "function.h"
> +#include "basic-block.h"
> +#include "gimple.h"
> +#include "options.h"
> +#include "diagnostic-path.h"
> +#include "diagnostic-metadata.h"
> +#include "function.h"
> +#include "json.h"
> +#include "analyzer/analyzer.h"
> +#include "diagnostic-event-id.h"
> +#include "analyzer/analyzer-logging.h"
> +#include "analyzer/sm.h"
> +#include "analyzer/pending-diagnostic.h"
> +#include "analyzer/function-set.h"
> +#include "analyzer/analyzer-selftests.h"
> +#include "tristate.h"
> +#include "selftest.h"
> +#include "analyzer/call-string.h"
> +#include "analyzer/program-point.h"
> +#include "analyzer/store.h"
> +#include "analyzer/region-model.h"
> +
> +#include <unistd.h>
We'll want to drop the #include <unistd.h> once we have a target hook.
[...snip...]
> +
> + state_machine::state_t
> + get_default_state (const svalue *sval) const final override
> + {
> + if (tree cst = sval->maybe_get_constant ())
> + {
> + int val = TREE_INT_CST_LOW (cst);
> + if (val < 0)
> + {
> + return m_invalid;
> + }
> + }
> + return m_start;
> + }
Please add coverage to the testsuite for the case of accessing negative
and non-negative constant integer values: what happens when we hit the
above cases?
In particular, IIRC 0, 1, and 2 are stdin, stdout, and stderr
respectively, and are already open at the start of "main" (though not
necessarily at the start of any given *analysis path*, so we can't
assume that) - so there may be code out there that has hardcoded
accesses using these constant integer values.
[...snip...]
> +
> + /* State for reperesenting a file descriptor that is known to be valid (>= 0),
> + for three different access */
Add " modes." to the end of the comment.
[...snip...]
> +/* Base diagnostic class relative to fd_state_machine. */
> +class fd_diagnostic : public pending_diagnostic
> +{
> +public:
> + fd_diagnostic (const fd_state_machine &sm, tree arg) : m_sm (sm), m_arg
> (arg)
> + {
> + }
> +
> + bool
> + subclass_equal_p (const pending_diagnostic &base_other) const override
> + {
> + return same_tree_p (m_arg, ((const fd_diagnostic &)base_other).m_arg);
> + }
> +
> + label_text
> + describe_state_change (const evdesc::state_change &change) override
> + {
> + if (change.m_old_state == m_sm.get_start_state ()
> + && m_sm.is_unchecked (change.m_new_state))
> + {
> + if (change.m_new_state == m_sm.m_unchecked_read_write)
> + return change.formatted_print ("opened here as %<read-write%>");
> +
> + if (change.m_new_state == m_sm.m_unchecked_read_only)
> + return change.formatted_print ("opened here as %<read-only%>");
> +
> + if (change.m_new_state == m_sm.m_unchecked_write_only)
> + return change.formatted_print ("opened here as %<write-only%>");
"read-write" etc in these messages are part of the text of the
messages, rather than quoting a source-language construct, so drop the
%< and %> here.
> + }
> +
> + if (change.m_new_state == m_sm.m_closed)
> + return change.formatted_print ("closed here");
> +
> + if (m_sm.is_unchecked (change.m_old_state)
> + && m_sm.is_valid (change.m_new_state))
> + if (change.m_expr)
> + return change.formatted_print (
> + "assuming %qE is a valid file descriptor (>= 0)",
> change.m_expr);
> + else
> + return change.formatted_print ("assuming a valid file descriptor");
> +
> + if (m_sm.is_unchecked (change.m_old_state)
> + && change.m_new_state == m_sm.m_invalid)
> + if (change.m_expr)
> + return change.formatted_print (
> + "assuming %qE is an invalid file descriptor (< 0)",
> change.m_expr);
> + else
> + return change.formatted_print ("assuming an invalid file
> descriptor");
> +
> + return label_text ();
> + }
> +
> +protected:
> + const fd_state_machine &m_sm;
> + tree m_arg;
> +};
> +
> +class fd_access_mode_mismatch : public fd_diagnostic
> +{
> +public:
> + /* mode specfies whether read on write was attempted or vice versa.
> + */
> + fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
> + enum access_mode mode, const tree callee_fndecl)
> + : m_mode (mode), m_callee_fndecl (callee_fndecl), fd_diagnostic (sm,
> arg)
> + {
> + }
Every subclass of fd_diagnostic that adds fields ought to have its own
override of:
subclass_equal_p (const pending_diagnostic &base_other) const override
which chains up to the parent class implementation, and also compares
the new fields.
> +
> + const char *
> + get_kind () const final override
> + {
> + return "fd_access_mode_mismatch";
> + }
> +
> + int
> + get_controlling_option () const final override
> + {
> + return OPT_Wanalyzer_fd_access_mode_mismatch;
> + }
> +
> + bool
> + emit (rich_location *rich_loc) final override
> + {
> + return warning_at (rich_loc, get_controlling_option (),
> + "%qE on %qs file descriptor %qE", m_callee_fndecl,
> + m_sm.get_string_for_access_mode (m_mode), m_arg);
It's bad for localization to be constructing strings from fragments
like this.
It's a pain, but please eliminate get_string_for_access_mode in favor
of switch statements wherever you'd use it, like here.
> + }
> +
> + label_text
> + describe_state_change (const evdesc::state_change &change) override
> + {
> + return fd_diagnostic::describe_state_change (change);
> + }
> +
> + label_text
> + describe_final_event (const evdesc::final_event &ev) final override
> + {
> + return ev.formatted_print ("%qE on %qs file descriptor %qE",
> + m_callee_fndecl,
> + m_sm.get_string_for_access_mode (m_mode), m_arg);
As above, please eliminate get_string_for_access_mode and convert to a
switch.
> + }
> +
> +private:
> + enum access_mode m_mode;
> + const tree m_callee_fndecl;
> +};
> +
[...snip...]
> +
> +class fd_use_after_close : public fd_diagnostic
> +{
> +public:
> + fd_use_after_close (const fd_state_machine &sm, tree arg,
> + const tree callee_fndecl)
> + : m_callee_fndecl (callee_fndecl), fd_diagnostic (sm, arg)
> + {
> + }
This subclass has its own fields, and so needs to implement:
subclass_equal_p (const pending_diagnostic &base_other) const override
[...snip...]
> +class unchecked_use_of_fd : public fd_diagnostic
> +{
> +public:
> + unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
> + const tree callee_fndecl)
> + : m_callee_fndecl (callee_fndecl), fd_diagnostic (sm, arg)
> + {
> + }
> +
> + const char *
> + get_kind () const final override
> + {
> + return "unchecked_use_of_fd";
> + }
> +
> + int
> + get_controlling_option () const final override
> + {
> + return OPT_Wanalyzer_fd_use_without_check;
> + }
> +
> + bool
> + emit (rich_location *rich_loc) final override
> + {
> +
> + return warning_at (rich_loc, get_controlling_option (),
> + "%qE on possibly invalid file descriptor %qE",
> + m_callee_fndecl, m_arg);
How about:
"%qE on file descriptor %qE without checking %<open%> succeeded"
> + }
> +
> + label_text
> + describe_state_change (const evdesc::state_change &change) override
> + {
> + if (m_sm.is_unchecked (change.m_new_state))
> + {
> + m_first_open_event = change.m_event_id;
> + return label_text::borrow ("opened here");
> + }
> +
> + return fd_diagnostic::describe_state_change (change);
> + }
> +
> + label_text
> + describe_final_event (const evdesc::final_event &ev) final override
> + {
> + return ev.formatted_print ("%qE could be invalid: unchecked value from
> %@",
> + m_arg, &m_first_open_event);
> + }
> +
> +private:
> + diagnostic_event_id_t m_first_open_event;
> + const tree m_callee_fndecl;
> +};
> +
[...snip...]
> +bool
> +fd_state_machine::is_unchecked (state_t s) const
> +{
> + return s == m_unchecked_read_write
> + || s == m_unchecked_read_only
> + || s == m_unchecked_write_only;
> +}
Thanks for splitting these onto multiple lines, but please wrap them
with parentheses, so they look like:
return (s == m_unchecked_read_write
|| s == m_unchecked_read_only
|| s == m_unchecked_write_only);
> +
> +bool
> +fd_state_machine::is_valid (state_t s) const
> +{
> + return s == m_valid_read_write
> + || s == m_valid_read_only
> + || s == m_valid_write_only;
> +}
Likewise here.
> +
> +const char *
> +fd_state_machine::get_string_for_access_mode (enum access_mode mode) const
> +{
> + if (mode == READ_ONLY)
> + return "read-only";
> + else if (mode == WRITE_ONLY)
> + return "write-only";
> + else
> + return "read-write";
> +}
As noted above it's bad for localization to be constructing strings
from fragments like this, so please eliminate the above function, in
favor of switch statements wherever you'd use it.
> +
> +enum access_mode
> +fd_state_machine::get_access_mode_from_flag (int flag) const
> +{
> + /* FIXME: this code assumes the access modes on the host and
> + target are the same, which in practice might not be the case. */
Thanks for moving this into a subroutine.
Joseph says we should introduce a target hook for this:
https://gcc.gnu.org/pipermail/gcc/2022-June/238961.html
You can see an example of adding a target hook in commit
8cdcea51c0fd753e6a652c9b236e91b3a6e0911c.
As noted above, I think we can leave adding the target hook until a
followup patch, if this is only going to be an issue with cross-
compilation between Hurd and non-Hurd systems.
> + if (flag == O_RDONLY)
> + {
> + return READ_ONLY;
> + }
> + else if (flag == O_WRONLY)
> + {
> + return WRITE_ONLY;
> + }
> + return READ_WRITE;
> +}
> +
> +enum access_mode
> +fd_state_machine::get_access_mode_from_state (state_t state) const
> +{
> + if (state == m_unchecked_read_only || state == m_valid_read_only)
> + return READ_ONLY;
> + else if (state == m_unchecked_write_only || state == m_valid_write_only)
> + return WRITE_ONLY;
> + else if (state == m_unchecked_read_write || state == m_valid_write_only)
I think there's a copy-and-paste error in the above line: ^^^^^^^^^^^^^^^^^^
> + return READ_WRITE;
> +}
[...snip...]
> +void
> +fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const gcall *call) const
> +{
> + tree lhs = gimple_call_lhs (call);
> + if (lhs)
> + {
> + tree arg = gimple_call_arg (call, 1);
> + if (TREE_CODE (arg) == INTEGER_CST)
> + {
> + int flag = TREE_INT_CST_LOW (arg);
> + enum access_mode mode = get_access_mode_from_flag (flag);
> + if (mode == READ_ONLY)
> + sm_ctxt->on_transition (node, stmt, lhs, m_start,
> + m_unchecked_read_only);
> + else if (mode == WRITE_ONLY)
> + sm_ctxt->on_transition (node, stmt, lhs, m_start,
> + m_unchecked_write_only);
> + else
> + sm_ctxt->on_transition (node, stmt, lhs, m_start,
> + m_unchecked_read_write);
I think this can be a switch (mode).
> + }
> + }
> + else
> + {
> + /* FIXME: add leak reporting */
> + }
> +}
[...snip...]
> +void
> +fd_state_machine::on_read (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const gcall *call,
> + const tree callee_fndecl) const
> +{
> + tree arg = gimple_call_arg (call, 0);
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + state_t state = sm_ctxt->get_state (stmt, arg);
> + enum access_mode mode = get_access_mode_from_state (state);
> +
> + if (check_for_closed_fd (state))
> + {
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_use_after_close (*this, diag_arg, callee_fndecl));
> + }
> +
> + if (!check_for_closed_fd (state))
> + {
Why not just an "else" here?
> +
> + if (!is_valid (sm_ctxt->get_state (stmt, arg)))
> + {
> + sm_ctxt->warn (
> + node, stmt, arg,
> + new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
> + }
> +
> + if (mode == WRITE_ONLY)
> + {
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_access_mode_mismatch (*this, diag_arg, mode,
> + callee_fndecl));
> + }
> + }
> +}
> +void
> +fd_state_machine::on_write (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const gcall *call,
> + const tree callee_fndecl) const
> +{
> + tree arg = gimple_call_arg (call, 0);
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + state_t state = sm_ctxt->get_state (stmt, arg);
> + enum access_mode mode = get_access_mode_from_state (state);
> +
> + if (check_for_closed_fd (state))
> + {
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_use_after_close (*this, diag_arg,
> callee_fndecl));
> + }
> +
> + if (!check_for_closed_fd (state))
> + {
Again, I think this can be another "else" here.
> +
> + if (!is_valid (sm_ctxt->get_state (stmt, arg)))
> + {
> + sm_ctxt->warn (
> + node, stmt, arg,
> + new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
> + }
...but I think it's slightly cleaner to move all of this repeated logic
into a "check_for_open_fd" subroutine, that checks both for
fd_use_after_close and for unchecked_use_of_fd.
> +
> + if (mode == READ_ONLY)
> + {
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_access_mode_mismatch (*this, diag_arg,
> mode,
> + callee_fndecl));
> + }
> + }
> +}
> +
[...snip...]
> diff --git gcc/testsuite/gcc.dg/analyzer/fd-1.c
> gcc/testsuite/gcc.dg/analyzer/fd-1.c
> new file mode 100644
> index 00000000000..985e9ac75de
> --- /dev/null
> +++ gcc/testsuite/gcc.dg/analyzer/fd-1.c
> @@ -0,0 +1,26 @@
> +int open(const char *, int mode);
> +#define O_RDONLY 0
> +#define O_WRONLY 1
> +#define O_RDWR 2
If we're going with a target hook, then we should use the target's
<unistd.h> to get at the O_ macros. But as noted above, that can be
deferred to a follow-up patch.
[...snip...]
Hope this makes sense and is constructive; thanks again for the updated
patch
Dave
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] analyzer PR 106003
2022-06-23 23:20 ` David Malcolm
@ 2022-07-14 15:40 ` David Malcolm
0 siblings, 0 replies; 8+ messages in thread
From: David Malcolm @ 2022-07-14 15:40 UTC (permalink / raw)
To: Mir Immad, gcc
On Thu, 2022-06-23 at 19:20 -0400, David Malcolm wrote:
> On Fri, 2022-06-24 at 00:00 +0530, Mir Immad wrote:
[...snip...]
> > +
> > +enum access_mode
> > +fd_state_machine::get_access_mode_from_flag (int flag) const
> > +{
> > + /* FIXME: this code assumes the access modes on the host and
> > + target are the same, which in practice might not be the
> > case. */
>
> Thanks for moving this into a subroutine.
>
> Joseph says we should introduce a target hook for this:
> https://gcc.gnu.org/pipermail/gcc/2022-June/238961.html
>
> You can see an example of adding a target hook in commit
> 8cdcea51c0fd753e6a652c9b236e91b3a6e0911c.
>
> As noted above, I think we can leave adding the target hook until a
> followup patch, if this is only going to be an issue with cross-
> compilation between Hurd and non-Hurd systems.
FWIW, for reference, I've filed
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106302
about this; it strikes me that there are other flags we might
eventually want to test e.g. for mmap we'd want to look at
MAP_ANONYMOUS and the various PROT_* values.
Dave
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] analyzer PR 106003
2022-06-27 17:08 Mir Immad
@ 2022-06-27 20:55 ` David Malcolm
0 siblings, 0 replies; 8+ messages in thread
From: David Malcolm @ 2022-06-27 20:55 UTC (permalink / raw)
To: Mir Immad, gcc
Thanks for the updated patch.
Various notes below; mostly nits, but I realized there's a logic error
in fd_state_machine::on_condition that I hadn't spotted before...
[...snip...]
> diff --git a/gcc/analyzer/ChangeLog b/gcc/analyzer/ChangeLog
> index 53b3ffb487b..d30e94f2f62 100644
> --- a/gcc/analyzer/ChangeLog
> +++ b/gcc/analyzer/ChangeLog
> @@ -1,3 +1,6 @@
> +2022-06-27 Immad Mir <mir@sourceware.org>
> + * sm-fd.cc: New file.
> +
The new ChangeLog entries should be part of the commit message of the
patch, not expressed part of the patch itself. Some notes on this can
be seen at:
https://gcc.gnu.org/codingconventions.html#ChangeLogs
[...snip...]
> +/* An enum for describing the direction of an access to a file descriptor.
> */
> +
> +enum access_direction
> +{
> + DIR_READ,
> + DIR_WRITE
> +};
We already have a declaration of "enum access_direction" in
analyzer/analyzer.h, so let's just use that, rather than declaring it a
second time.
[...snip...]
> +class fd_access_mode_mismatch : public fd_diagnostic
> +{
> +public:
> + fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
> + enum access_direction access_dir,
> + const tree callee_fndecl)
> + : m_access_dir (access_dir), m_callee_fndecl (callee_fndecl),
> + fd_diagnostic (sm, arg)
The ordering in this initializer list looks wrong: I'd expect to see
the base class initializer (fd_diagnostics) *before* the fields.
I think we have a warning about this.
> + {
> + }
> +
> + const char *
> + get_kind () const final override
> + {
> + return "fd_access_mode_mismatch";
> + }
> +
> + int
> + get_controlling_option () const final override
> + {
> + return OPT_Wanalyzer_fd_access_mode_mismatch;
> + }
> +
> + bool
> + emit (rich_location *rich_loc) final override
> + {
> +
> + switch (m_access_dir)
> + {
> + case DIR_READ:
> + return warning_at (rich_loc, get_controlling_option (),
> + "%qE on %<read-only%> file descriptor %qE",
> + m_callee_fndecl, m_arg);
> + case DIR_WRITE:
> + return warning_at (rich_loc, get_controlling_option (),
> + "%qE on %<write-only%> file descriptor %qE",
> + m_callee_fndecl, m_arg);
> + default:
> + gcc_unreachable ();
> + }
I got a bit confused here between the direction in which the FD expects
accesses (e.g. due to O_RDONLY), as opposed to the direction of the
operation being attempted on said FD (e.g. due to a "write" call).
I realize that "access" is ambiguous in that it could refer to either
meaning; "m_access_dir" here means the the former.
How about the naming convention of "fd_dir" (and thus "m_fd_dir" here)
where we're talking about the former, and "FOO_dir" for some FOO (e.g.
"callee_fndecl_dir") when we're talking about the latter? Using that
consistently throughout could clarify things.
[...snip...]
> +enum access_direction
> +fd_state_machine::get_access_dir_from_state (state_t state) const
> +{
> + if (state == m_unchecked_read_only || state == m_valid_read_only)
> + return DIR_READ;
> + else if (state == m_unchecked_write_only || state == m_valid_write_only)
> + return DIR_WRITE;
> +}
Control will "fall off the end" of this function if "state" doesn't
match one of the four states you test against. Hopefully we emit a
warning for this (which should make the bootstrap build fail).
This only gets used in one place, in
fd_state_machine::check_for_open_fd, and the value is only needed once
we know we have a valid_fd, so I think it's best to simply put the
above logic there, within the switch statement...
[...snip...]
> +void
> +fd_state_machine::check_for_open_fd (sm_context *sm_ctxt, const supernode
> *node,
> + const gimple *stmt, const gcall *call,
> + const tree callee_fndecl,
> + enum access_direction access_fn) const
Please clarify this for me by renaming "access_fn" to
"callee_fndecl_dir" or "attempted_dir" or similar.
> +{
> + tree arg = gimple_call_arg (call, 0);
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + state_t state = sm_ctxt->get_state (stmt, arg);
> + enum access_direction access_dir = get_access_dir_from_state (state);
....so drop this call...
> +
> + if (is_closed_fd_p (state))
> + {
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_use_after_close (*this, diag_arg,
> callee_fndecl));
> + }
> +
> + else
> + {
> +
> + if (!is_valid_fd_p (state))
> + {
> + if (!is_constant_fd_p (state))
> + sm_ctxt->warn (
> + node, stmt, arg,
> + new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
> + }
> + switch (access_fn)
> + {
> + case DIR_READ:
...in favor of simply checking for the write-only states here:
(perhaps with a new "is_readonly_fd_p")
> + if (access_dir == DIR_WRITE)
> + {
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_access_mode_mismatch (
> + *this, diag_arg, access_dir,
> callee_fndecl));
> + }
> +
> + break;
> + case DIR_WRITE:
> +
> + if (access_dir == DIR_READ)
> + {
...and for the read-only states here:
(perhaps with a new "is_writeonly_fd_p")
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_access_mode_mismatch (
> + *this, diag_arg, access_dir,
> callee_fndecl));
> + }
> + break;
> + }
> + }
> +}
> +
> +void
> +fd_state_machine::on_condition (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const svalue *lhs,
> + enum tree_code op, const svalue *rhs) const
> +{
> +
> + if (!rhs->all_zeroes_p ())
> + return;
> +
> + if (op == GE_EXPR)
> + {
> + sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_write,
> + m_valid_read_write);
> + sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_only,
> + m_valid_read_only);
> + sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_write_only,
> + m_valid_write_only);
> + }
> + else if (op == LT_EXPR)
> + {
> + sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_write,
> + m_invalid);
> + sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_only,
> + m_invalid);
> + sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_write_only,
> + m_invalid);
> + }
Here we're detecting when the result of "open" is tested for < 0 vs
>=0.
I did some refresher reading on file descriptors over the weekend
(specifically W. Richard Stevens's classic "Advanced Programming in the
UNIX Environment") and in his examples he always checks the result of
"open" for != -1, rather than < 0.
The man page for "open" specifies:
https://www.man7.org/linux/man-pages/man2/open.2.html#RETURN_VALUE
"On success, open(), openat(), and creat() return the new file
descriptor (a nonnegative integer). On error, -1 is returned and errno
is set to indicate the error."
Similarly,
https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
has: "Upon successful completion, these functions shall open the file
and return a non-negative integer representing the file descriptor.
Otherwise, these functions shall return -1 and set errno to indicate
the error. If -1 is returned, no files shall be created or modified."
We should *not* warn about code that does a test for != -1, since
that's what the standard says to test for.
That said, I think that it would be overzealous to complain about code
that uses < 0 as a test on the result of "open".
So please add DejaGnu test coverage for such "!= -1" code, and ensure
that fd_state_machine::on_condition handles EQ_EXPR/NE_EXPR against -1
(and continues to handle the "< 0" test).
[...snip...]
> diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
> index 604f4e3b0a8..37173529b2d 100644
> --- a/gcc/testsuite/ChangeLog
> +++ b/gcc/testsuite/ChangeLog
> @@ -1,3 +1,9 @@
> +2022-06-27 Immad Mir <mir@sourceware.org>
> + * 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.
Same notes as above about how we manage ChangeLog entries.
[...snip...]
I think that with the above nits fixed, the next version of this is
likely going to be ready for trunk - assuming that you bootstrap it and
verify that the testsuite doesn't regress.
Hope the above makes sense; thanks again for the patch.
Dave
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] analyzer PR 106003
@ 2022-06-27 17:08 Mir Immad
2022-06-27 20:55 ` David Malcolm
0 siblings, 1 reply; 8+ messages in thread
From: Mir Immad @ 2022-06-27 17:08 UTC (permalink / raw)
To: gcc, David Malcolm
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index b6dcc45a58a..04631f737ea 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1269,6 +1269,7 @@ ANALYZER_OBJS = \
analyzer/region-model-reachability.o \
analyzer/sm.o \
analyzer/sm-file.o \
+ analyzer/sm-fd.o \
analyzer/sm-malloc.o \
analyzer/sm-pattern-test.o \
analyzer/sm-sensitive.o \
diff --git a/gcc/analyzer/ChangeLog b/gcc/analyzer/ChangeLog
index 53b3ffb487b..d30e94f2f62 100644
--- a/gcc/analyzer/ChangeLog
+++ b/gcc/analyzer/ChangeLog
@@ -1,3 +1,6 @@
+2022-06-27 Immad Mir <mir@sourceware.org>
+ * sm-fd.cc: New file.
+
2022-06-02 David Malcolm <dmalcolm@redhat.com>
* checker-path.cc (checker_event::get_meaning): New.
diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index 23dfc797cea..219f6180130 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -66,6 +66,26 @@ Wanalyzer-exposure-through-output-file
Common Var(warn_analyzer_exposure_through_output_file) Init(1) Warning
Warn about code paths in which sensitive data is written to a file.
+Wanalyzer-fd-access-mode-mismatch
+Common Var(warn_analyzer_fd_mode_mismatch) Init(1) Warning
+Warn about code paths in which read on write-only file descriptor is
attempted, or vice versa.
+
+Wanalyzer-fd-double-close
+Common Var(warn_analyzer_fd_double_close) Init(1) Warning
+Warn about code paths in which a file descriptor can be closed more than
once.
+
+Wanalyzer-fd-leak
+Common Var(warn_analyzer_fd_leak) Init(1) Warning
+Warn about code paths in which a file descriptor is not closed.
+
+Wanalyzer-fd-use-after-close
+Common Var(warn_analyzer_fd_use_after_close) Init(1) Warning
+Warn about code paths in which a read or write is performed on a closed
file descriptor.
+
+Wanalyzer-fd-use-without-check
+Common Var(warn_analyzer_fd_use_without_check) Init(1) Warning
+Warn about code paths in which a file descriptor is used without being
checked for validity.
+
Wanalyzer-file-leak
Common Var(warn_analyzer_file_leak) Init(1) Warning
Warn about code paths in which a stdio FILE is not closed.
diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
new file mode 100644
index 00000000000..0e9833a55a8
--- /dev/null
+++ b/gcc/analyzer/sm-fd.cc
@@ -0,0 +1,821 @@
+/* A state machine for detecting misuses of POSIX file descriptor APIs.
+ Copyright (C) 2019-2022 Free Software Foundation, Inc.
+ Contributed by Immad Mir <mir@sourceware.org>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "function.h"
+#include "basic-block.h"
+#include "gimple.h"
+#include "options.h"
+#include "diagnostic-path.h"
+#include "diagnostic-metadata.h"
+#include "function.h"
+#include "json.h"
+#include "analyzer/analyzer.h"
+#include "diagnostic-event-id.h"
+#include "analyzer/analyzer-logging.h"
+#include "analyzer/sm.h"
+#include "analyzer/pending-diagnostic.h"
+#include "analyzer/function-set.h"
+#include "analyzer/analyzer-selftests.h"
+#include "tristate.h"
+#include "selftest.h"
+#include "analyzer/call-string.h"
+#include "analyzer/program-point.h"
+#include "analyzer/store.h"
+#include "analyzer/region-model.h"
+
+#include <unistd.h>
+#if ENABLE_ANALYZER
+
+namespace ana
+{
+
+namespace
+{
+
+/* An enum for distinguishing between three different access modes. */
+
+enum access_mode
+{
+ READ_WRITE,
+ READ_ONLY,
+ WRITE_ONLY
+};
+
+/* An enum for describing the direction of an access to a file descriptor.
*/
+
+enum access_direction
+{
+ DIR_READ,
+ DIR_WRITE
+};
+
+class fd_state_machine : public state_machine
+{
+public:
+ fd_state_machine (logger *logger);
+
+ bool
+ inherited_state_p () const final override
+ {
+ return false;
+ }
+
+ state_machine::state_t
+ get_default_state (const svalue *sval) const final override
+ {
+ if (tree cst = sval->maybe_get_constant ())
+ {
+ int val = TREE_INT_CST_LOW (cst);
+ if (val >= 0)
+ return m_constant_fd;
+ else
+ return m_invalid;
+ }
+ return m_start;
+ }
+
+ bool on_stmt (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt) const final override;
+
+ void on_condition (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const svalue *lhs, const
tree_code op,
+ const svalue *rhs) const final override;
+
+ bool can_purge_p (state_t s) const final override;
+ pending_diagnostic *on_leak (tree var) const final override;
+
+ bool is_unchecked_fd_p (state_t s) const;
+ bool is_valid_fd_p (state_t s) const;
+ bool is_closed_fd_p (state_t s) const;
+ bool is_invalid_fd_p (state_t s) const;
+ bool is_constant_fd_p (state_t s) const;
+ enum access_mode get_access_mode_from_flag (int flag) const;
+ enum access_direction get_access_dir_from_state (state_t s) const;
+
+ /* State for a constant file descriptor (>= 0) */
+ state_t m_constant_fd;
+
+ /* States representing a file descriptor that hasn't yet been
+ checked for validity after opening, for three different
+ access modes. */
+ state_t m_unchecked_read_write;
+
+ state_t m_unchecked_read_only;
+
+ state_t m_unchecked_write_only;
+
+ /* States for reperesenting a file descriptor that is known to be valid
(>=
+ 0), for three different access modes.*/
+ state_t m_valid_read_write;
+
+ state_t m_valid_read_only;
+
+ state_t m_valid_write_only;
+
+ /* State for a file descriptor that is known to be invalid (< 0). */
+ state_t m_invalid;
+
+ /* State for a file descriptor that has been closed.*/
+ state_t m_closed;
+
+ /* State for a file descriptor that we do not want to track anymore . */
+ state_t m_stop;
+
+private:
+ void on_open (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call) const;
+ void on_close (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call, const tree callee_fndecl) const;
+ void on_read (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call, const tree callee_fndecl) const;
+ void on_write (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call, const tree callee_fndecl) const;
+ void check_for_open_fd (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl,
+ enum access_direction access_fn) const;
+};
+
+/* Base diagnostic class relative to fd_state_machine. */
+class fd_diagnostic : public pending_diagnostic
+{
+public:
+ fd_diagnostic (const fd_state_machine &sm, tree arg) : m_sm (sm), m_arg
(arg)
+ {
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ return same_tree_p (m_arg, ((const fd_diagnostic &)base_other).m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (change.m_old_state == m_sm.get_start_state ()
+ && m_sm.is_unchecked_fd_p (change.m_new_state))
+ {
+ if (change.m_new_state == m_sm.m_unchecked_read_write)
+ return change.formatted_print ("opened here as read-write");
+
+ if (change.m_new_state == m_sm.m_unchecked_read_only)
+ return change.formatted_print ("opened here as read-only");
+
+ if (change.m_new_state == m_sm.m_unchecked_write_only)
+ return change.formatted_print ("opened here as write-only");
+ }
+
+ if (change.m_new_state == m_sm.m_closed)
+ return change.formatted_print ("closed here");
+
+ if (m_sm.is_unchecked_fd_p (change.m_old_state)
+ && m_sm.is_valid_fd_p (change.m_new_state))
+ if (change.m_expr)
+ return change.formatted_print (
+ "assuming %qE is a valid file descriptor (>= 0)",
change.m_expr);
+ else
+ return change.formatted_print ("assuming a valid file descriptor");
+
+ if (m_sm.is_unchecked_fd_p (change.m_old_state)
+ && change.m_new_state == m_sm.m_invalid)
+ if (change.m_expr)
+ return change.formatted_print (
+ "assuming %qE is an invalid file descriptor (< 0)",
change.m_expr);
+ else
+ return change.formatted_print ("assuming an invalid file
descriptor");
+
+ return label_text ();
+ }
+
+protected:
+ const fd_state_machine &m_sm;
+ tree m_arg;
+};
+
+class fd_leak : public fd_diagnostic
+{
+public:
+ fd_leak (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
{}
+
+ const char *
+ get_kind () const final override
+ {
+ return "fd_leak";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_leak;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ /*CWE-775: Missing Release of File Descriptor or Handle after Effective
+ Lifetime
+ */
+ diagnostic_metadata m;
+ m.add_cwe (775);
+ if (m_arg)
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "leak of file descriptor %qE", m_arg);
+ else
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "leak of file descriptor");
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) final override
+ {
+ if (m_sm.is_unchecked_fd_p (change.m_new_state))
+ {
+ m_open_event = change.m_event_id;
+ return label_text::borrow ("opened here");
+ }
+
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ if (m_open_event.known_p ())
+ {
+ if (ev.m_expr)
+ return ev.formatted_print ("%qE leaks here; was opened at %@",
+ ev.m_expr, &m_open_event);
+ else
+ return ev.formatted_print ("leaks here; was opened at %@",
+ &m_open_event);
+ }
+ else
+ {
+ if (ev.m_expr)
+ return ev.formatted_print ("%qE leaks here", ev.m_expr);
+ else
+ return ev.formatted_print ("leaks here");
+ }
+ }
+
+private:
+ diagnostic_event_id_t m_open_event;
+};
+
+class fd_access_mode_mismatch : public fd_diagnostic
+{
+public:
+ fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
+ enum access_direction access_dir,
+ const tree callee_fndecl)
+ : m_access_dir (access_dir), m_callee_fndecl (callee_fndecl),
+ fd_diagnostic (sm, arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "fd_access_mode_mismatch";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_access_mode_mismatch;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+
+ switch (m_access_dir)
+ {
+ case DIR_READ:
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on %<read-only%> file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ case DIR_WRITE:
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on %<write-only%> file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ default:
+ gcc_unreachable ();
+ }
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ const fd_access_mode_mismatch &sub_other
+ = (const fd_access_mode_mismatch &)base_other;
+ return (same_tree_p (m_arg, sub_other.m_arg)
+ && m_callee_fndecl == sub_other.m_callee_fndecl
+ && m_access_dir == sub_other.m_access_dir);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ switch (m_access_dir)
+ {
+ case DIR_READ:
+ return ev.formatted_print ("%qE on %<read-only%> file descriptor
%qE",
+ m_callee_fndecl, m_arg);
+ case DIR_WRITE:
+ return ev.formatted_print ("%qE on %<write-only%> file descriptor
%qE",
+ m_callee_fndecl, m_arg);
+ default:
+ gcc_unreachable ();
+ }
+ }
+
+private:
+ enum access_direction m_access_dir;
+ const tree m_callee_fndecl;
+};
+
+class double_close : public fd_diagnostic
+{
+public:
+ double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm,
arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "double_close";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_double_close;
+ }
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ diagnostic_metadata m;
+ // CWE-1341: Multiple Releases of Same Resource or Handle
+ m.add_cwe (1341);
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "double %<close%> of file descriptor %qE", m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (m_sm.is_unchecked_fd_p (change.m_new_state))
+ return label_text::borrow ("opened here");
+
+ if (change.m_new_state == m_sm.m_closed)
+ {
+ m_first_close_event = change.m_event_id;
+ return change.formatted_print ("first %qs here", "close");
+ }
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ if (m_first_close_event.known_p ())
+ return ev.formatted_print ("second %qs here; first %qs was at %@",
+ "close", "close", &m_first_close_event);
+ return ev.formatted_print ("second %qs here", "close");
+ }
+
+private:
+ diagnostic_event_id_t m_first_close_event;
+};
+
+class fd_use_after_close : public fd_diagnostic
+{
+public:
+ fd_use_after_close (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl)
+ : m_callee_fndecl (callee_fndecl), fd_diagnostic (sm, arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "fd_use_after_close";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_use_after_close;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on closed file descriptor %qE",
m_callee_fndecl,
+ m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (m_sm.is_unchecked_fd_p (change.m_new_state))
+ return label_text::borrow ("opened here");
+
+ if (change.m_new_state == m_sm.m_closed)
+ return change.formatted_print ("closed here");
+
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ return ev.formatted_print ("%qE on closed file descriptor %qE here",
+ m_callee_fndecl, m_arg);
+ }
+
+private:
+ const tree m_callee_fndecl;
+};
+
+class unchecked_use_of_fd : public fd_diagnostic
+{
+public:
+ unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl)
+ : m_callee_fndecl (callee_fndecl), fd_diagnostic (sm, arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "unchecked_use_of_fd";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_use_without_check;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on possibly invalid file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ const unchecked_use_of_fd &sub_other
+ = (const unchecked_use_of_fd &)base_other;
+ return (same_tree_p (m_arg, sub_other.m_arg)
+ && m_callee_fndecl == sub_other.m_callee_fndecl);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (m_sm.is_unchecked_fd_p (change.m_new_state))
+ {
+ m_first_open_event = change.m_event_id;
+ return label_text::borrow ("opened here");
+ }
+
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ if (m_first_open_event.known_p ())
+ return ev.formatted_print (
+ "%qE could be invalid: unchecked value from %@", m_arg,
+ &m_first_open_event);
+ else
+ return ev.formatted_print ("%qE could be invalid", m_arg);
+ }
+
+private:
+ diagnostic_event_id_t m_first_open_event;
+ const tree m_callee_fndecl;
+};
+
+fd_state_machine::fd_state_machine (logger *logger)
+ : state_machine ("file-descriptor", logger),
+ m_constant_fd (add_state ("fd-constant")),
+ m_unchecked_read_write (add_state ("fd-unchecked-read-write")),
+ m_unchecked_read_only (add_state ("fd-unchecked-read-only")),
+ m_unchecked_write_only (add_state ("fd-unchecked-write-only")),
+ m_invalid (add_state ("fd-invalid")),
+ m_valid_read_write (add_state ("fd-valid-read-write")),
+ m_valid_read_only (add_state ("fd-valid-read-only")),
+ m_valid_write_only (add_state ("fd-valid-write-only")),
+ m_closed (add_state ("fd-closed")), m_stop (add_state ("fd-stop"))
+{
+}
+
+bool
+fd_state_machine::is_unchecked_fd_p (state_t s) const
+{
+ return (s == m_unchecked_read_write || s == m_unchecked_read_only
+ || s == m_unchecked_write_only);
+}
+
+bool
+fd_state_machine::is_valid_fd_p (state_t s) const
+{
+ return (s == m_valid_read_write || s == m_valid_read_only
+ || s == m_valid_write_only);
+}
+
+enum access_mode
+fd_state_machine::get_access_mode_from_flag (int flag) const
+{
+ /* FIXME: this code assumes the access modes on the host and
+ target are the same, which in practice might not be the case. */
+ if (flag == O_RDONLY)
+ {
+ return READ_ONLY;
+ }
+ else if (flag == O_WRONLY)
+ {
+ return WRITE_ONLY;
+ }
+ return READ_WRITE;
+}
+
+enum access_direction
+fd_state_machine::get_access_dir_from_state (state_t state) const
+{
+ if (state == m_unchecked_read_only || state == m_valid_read_only)
+ return DIR_READ;
+ else if (state == m_unchecked_write_only || state == m_valid_write_only)
+ return DIR_WRITE;
+}
+
+bool
+fd_state_machine::is_closed_fd_p (state_t state) const
+{
+ return (state == m_closed);
+}
+
+bool
+fd_state_machine::is_invalid_fd_p (state_t state) const
+{
+ return (state == m_invalid);
+}
+
+bool
+fd_state_machine::is_constant_fd_p (state_t state) const
+{
+ return (state == m_constant_fd);
+}
+
+bool
+fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt) const
+{
+
+ if (const gcall *call = dyn_cast<const gcall *> (stmt))
+ if (tree callee_fndecl = sm_ctxt->get_fndecl_for_call (call))
+ {
+ if (is_named_call_p (callee_fndecl, "open", call, 2))
+ {
+ on_open (sm_ctxt, node, stmt, call);
+ return true;
+ } // "open"
+
+ if (is_named_call_p (callee_fndecl, "close", call, 1))
+ {
+ on_close (sm_ctxt, node, stmt, call, callee_fndecl);
+ return true;
+ } // "close"
+
+ if (is_named_call_p (callee_fndecl, "write", call, 3))
+ {
+ on_write (sm_ctxt, node, stmt, call, callee_fndecl);
+ return true;
+ } // "write"
+
+ if (is_named_call_p (callee_fndecl, "read", call, 3))
+ {
+ on_read (sm_ctxt, node, stmt, call, callee_fndecl);
+ return true;
+ } // "read"
+ }
+
+ return false;
+}
+
+void
+fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call) const
+{
+ tree lhs = gimple_call_lhs (call);
+ if (lhs)
+ {
+ tree arg = gimple_call_arg (call, 1);
+ if (TREE_CODE (arg) == INTEGER_CST)
+ {
+ int flag = TREE_INT_CST_LOW (arg);
+ enum access_mode mode = get_access_mode_from_flag (flag);
+
+ switch (mode)
+ {
+ case READ_ONLY:
+ sm_ctxt->on_transition (node, stmt, lhs, m_start,
+ m_unchecked_read_only);
+ break;
+ case WRITE_ONLY:
+ sm_ctxt->on_transition (node, stmt, lhs, m_start,
+ m_unchecked_write_only);
+ break;
+ default:
+ sm_ctxt->on_transition (node, stmt, lhs, m_start,
+ m_unchecked_read_write);
+ }
+ }
+ }
+ else
+ {
+ sm_ctxt->warn (node, stmt, NULL_TREE, new fd_leak (*this,
NULL_TREE));
+ }
+}
+
+void
+fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl) const
+{
+ tree arg = gimple_call_arg (call, 0);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+
+ sm_ctxt->on_transition (node, stmt, arg, m_start, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_write,
m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_only,
m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_unchecked_write_only,
m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_valid_read_write, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_valid_read_only, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_valid_write_only, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_constant_fd, m_closed);
+
+ if (is_closed_fd_p (state))
+ {
+
+ sm_ctxt->warn (node, stmt, arg, new double_close (*this, diag_arg));
+ sm_ctxt->set_next_state (stmt, arg, m_stop);
+ }
+}
+void
+fd_state_machine::on_read (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl) const
+{
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIR_READ);
+}
+void
+fd_state_machine::on_write (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl) const
+{
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIR_WRITE);
+}
+
+void
+fd_state_machine::check_for_open_fd (sm_context *sm_ctxt, const supernode
*node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl,
+ enum access_direction access_fn) const
+{
+ tree arg = gimple_call_arg (call, 0);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ enum access_direction access_dir = get_access_dir_from_state (state);
+
+ if (is_closed_fd_p (state))
+ {
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_after_close (*this, diag_arg,
callee_fndecl));
+ }
+
+ else
+ {
+
+ if (!is_valid_fd_p (state))
+ {
+ if (!is_constant_fd_p (state))
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
+ }
+ switch (access_fn)
+ {
+ case DIR_READ:
+ if (access_dir == DIR_WRITE)
+ {
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_access_mode_mismatch (
+ *this, diag_arg, access_dir,
callee_fndecl));
+ }
+
+ break;
+ case DIR_WRITE:
+
+ if (access_dir == DIR_READ)
+ {
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_access_mode_mismatch (
+ *this, diag_arg, access_dir,
callee_fndecl));
+ }
+ break;
+ }
+ }
+}
+
+void
+fd_state_machine::on_condition (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const svalue *lhs,
+ enum tree_code op, const svalue *rhs) const
+{
+
+ if (!rhs->all_zeroes_p ())
+ return;
+
+ if (op == GE_EXPR)
+ {
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_write,
+ m_valid_read_write);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_only,
+ m_valid_read_only);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_write_only,
+ m_valid_write_only);
+ }
+ else if (op == LT_EXPR)
+ {
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_write,
+ m_invalid);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_only,
+ m_invalid);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_write_only,
+ m_invalid);
+ }
+}
+
+bool
+fd_state_machine::can_purge_p (state_t s) const
+{
+ if (is_unchecked_fd_p (s) || is_valid_fd_p (s))
+ return false;
+ else
+ return true;
+}
+
+pending_diagnostic *
+fd_state_machine::on_leak (tree var) const
+{
+ return new fd_leak (*this, var);
+}
+} // anonymus namespace
+
+state_machine *
+make_fd_state_machine (logger *logger)
+{
+ return new fd_state_machine (logger);
+}
+} // namespace ana
+
+#endif // ENABLE_ANALYZER
\ No newline at end of file
diff --git a/gcc/analyzer/sm.cc b/gcc/analyzer/sm.cc
index 622cb0b7ab3..24c20b894cd 100644
--- a/gcc/analyzer/sm.cc
+++ b/gcc/analyzer/sm.cc
@@ -167,6 +167,7 @@ make_checkers (auto_delete_vec <state_machine> &out,
logger *logger)
{
out.safe_push (make_malloc_state_machine (logger));
out.safe_push (make_fileptr_state_machine (logger));
+ out.safe_push (make_fd_state_machine (logger));
/* The "taint" checker must be explicitly enabled (as it currently
leads to state explosions that stop the other checkers working). */
if (flag_analyzer_checker)
diff --git a/gcc/analyzer/sm.h b/gcc/analyzer/sm.h
index 4cc54531c56..e80ef1fac37 100644
--- a/gcc/analyzer/sm.h
+++ b/gcc/analyzer/sm.h
@@ -301,6 +301,7 @@ extern state_machine *make_sensitive_state_machine
(logger *logger);
extern state_machine *make_signal_state_machine (logger *logger);
extern state_machine *make_pattern_test_state_machine (logger *logger);
extern state_machine *make_va_list_state_machine (logger *logger);
+extern state_machine *make_fd_state_machine (logger *logger);
} // namespace ana
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 174bc09e5cf..b4a46dddfbb 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -9709,6 +9709,11 @@ Enabling this option effectively enables the
following warnings:
-Wanalyzer-double-fclose @gol
-Wanalyzer-double-free @gol
-Wanalyzer-exposure-through-output-file @gol
+-Wanalyzer-fd-access-mode-mismatch @gol
+-Wanalyzer-fd-double-close @gol
+-Wanalyzer-fd-leak @gol
+-Wanalyzer-fd-use-after-close @gol
+-Wanalyzer-fd-use-without-check @gol
-Wanalyzer-file-leak @gol
-Wanalyzer-free-of-non-heap @gol
-Wanalyzer-malloc-leak @gol
@@ -9783,6 +9788,56 @@ This diagnostic warns for paths through the code in
which a
security-sensitive value is written to an output file
(such as writing a password to a log file).
+@item -Wno-analyzer-fd-access-mode-mismatch
+@opindex Wanalyzer-fd-access-mode-mismatch
+@opindex Wno-analyzer-fd-access-mode-mismatch
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-access-mode-mismatch}
+to disable it.
+
+This diagnostic warns for paths through code in which a
+@code{read} on a write-only file descriptor is attempted, or vice versa
+
+@item -Wno-analyzer-fd-double-close
+@opindex Wanalyzer-fd-double-close
+@opindex Wno-analyzer-fd-double-close
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-double-close}
+to disable it.
+
+This diagnostic warns for paths through code in which a
+file descriptor can be closed more than once.
+
+@item -Wno-analyzer-fd-leak
+@opindex Wanalyzer-fd-leak
+@opindex Wno-analyzer-fd-leak
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-leak}
+to disable it.
+
+This diagnostic warns for paths through code in which an
+open file descriptor is leaked.
+
+@item -Wno-analyzer-fd-use-after-close
+@opindex Wanalyzer-fd-use-after-close
+@opindex Wno-analyzer-fd-use-after-close
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-use-after-close}
+to disable it.
+
+This diagnostic warns for paths through code in which a
+read or write is called on a closed file descriptor.
+
+@item -Wno-analyzer-fd-use-without-check
+@opindex Wanalyzer-fd-use-without-check
+@opindex Wno-analyzer-fd-use-without-check
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-use-without-check}
+to disable it.
+
+This diagnostic warns for paths through code in which a
+file descriptor is used without being checked for validity.
+
@item -Wno-analyzer-file-leak
@opindex Wanalyzer-file-leak
@opindex Wno-analyzer-file-leak
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 604f4e3b0a8..37173529b2d 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2022-06-27 Immad Mir <mir@sourceware.org>
+ * 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.
+
2022-06-10 Nathan Sidwell <nathan@acm.org>
* g++.dg/modules/init-3_a.C: New.
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-1.c
b/gcc/testsuite/gcc.dg/analyzer/fd-1.c
new file mode 100644
index 00000000000..aaff8054212
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-1.c
@@ -0,0 +1,32 @@
+int open(const char *, int mode);
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDONLY); /* { dg-message "\\(1\\) opened here" }
*/
+ return; /* { dg-warning "leak of file descriptor 'fd' \\\[CWE-775\\\]"
"warning" } */
+ /* { dg-message "\\(2\\) 'fd' leaks here; was opened at \\(1\\)" "event"
{ target *-*-* } .-1 } */
+}
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" } */
+ if (fd >= 0) /* { dg-message "\\(2\\) assuming 'fd' is a valid file
descriptor" "event1" } */
+ /* { dg-message "\\(3\\) following 'true' branch \\(when 'fd >=
0'\\)..." "event2" { target *-*-* } .-1 } */
+ {
+ return; /* { dg-warning "leak of file descriptor 'fd' \\\[CWE-775\\\]"
"warning" } */
+ /* { dg-message "\\(4\\) ...to here" "event1" { target *-*-* } .-1 } */
+ /* { dg-message "\\(5\\) 'fd' leaks here; was opened at \\(1\\)"
"event2" { target *-*-* } .-2 } */
+ }
+}
+
+void test_3 (const char *path)
+{
+ open(path, O_RDONLY); /* { dg-warning "leak of file descriptor
\\\[CWE-775\\\]" } */
+ /* { dg-message "\\(1\\) leaks here" "" { target *-*-* } .-1 } */
+}
+
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-2.c
b/gcc/testsuite/gcc.dg/analyzer/fd-2.c
new file mode 100644
index 00000000000..96ccf2f7ba8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-2.c
@@ -0,0 +1,49 @@
+int open(const char *, int mode);
+void close(int fd);
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+#define STDIN 0
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ close (fd); /* { dg-message "\\(2\\) first 'close' here" "event1" } */
+ close (fd); /* { dg-warning "double 'close' of file descriptor 'fd'
\\\[CWE-1341\\\]" "warning" } */
+ /* { dg-message "\\(3\\) second 'close' here; first 'close' was at
\\(2\\)" "event2" { target *-*-* } .-1 } */
+}
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ if (fd < 0) /* { dg-message "\\(2\\) assuming 'fd' is a valid file
descriptor \\(>= 0\\)" "event1" } */
+ /* { dg-message "\\(3\\) following 'false' branch \\(when 'fd >=
0'\\)..." "event2" { target *-*-* } .-1 } */
+ return;
+ close (fd); /* { dg-message "\\(4\\) ...to here" "event1" } */
+ /* { dg-message "\\(5\\) first 'close' here" "event2" { target *-*-* }
.-1 } */
+ close (fd); /* { dg-warning "double 'close' of file descriptor 'fd'
\\\[CWE-1341\\\]" "warning" } */
+ /* {dg-message "\\(6\\) second 'close' here; first was at \\(5\\)" ""
{ target *-*-* } .-1 } */
+}
+
+void
+test_3 ()
+{
+ /* FD 0 is stdin at the entry to "main" and thus read-only, but we
have no
+ guarantees here that it hasn't been closed and then reopened for
+ writing, so we can't issue a warning */
+
+ int fd = STDIN;
+ close(fd); /* { dg-message "\\(1\\) first 'close' here" } */
+ close(fd); /* { dg-warning "double 'close' of file descriptor 'fd'
\\\[CWE-1341\\\]" "warning" } */
+ /* { dg-message "\\(2\\) second 'close' here; first 'close' was at
\\(1\\)" "event2" { target *-*-* } .-1 } */
+}
+
+void
+test_4 ()
+{
+ int fd = -1;
+ close(fd);
+ close(fd);
+}
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-3.c
b/gcc/testsuite/gcc.dg/analyzer/fd-3.c
new file mode 100644
index 00000000000..22d29389ff9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-3.c
@@ -0,0 +1,57 @@
+int open(const char *, int mode);
+void close(int fd);
+int write (int fd, void *buf, int nbytes);
+int read (int fd, void *buf, int nbytes);
+int some_condition();
+
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+#define STDIN 0
+#define O_NOATIME 262144
+
+void
+test_1 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ write (fd, buf, 1); /* { dg-message "\\(2\\) 'fd' could be invalid:
unchecked value from \\(1\\)" } */
+ /* { dg-warning "'write' on possibly invalid file descriptor 'fd'"
"warning" { target *-*-* } .-1 } */
+ close(fd);
+}
+
+void
+test_2 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ read (fd, buf, 1); /* { dg-message "\\(2\\) 'fd' could be invalid:
unchecked value from \\(1\\)" } */
+ /* { dg-warning "'read' on possibly invalid file descriptor 'fd'"
"warning" { target *-*-* } .-1 } */
+ close (fd);
+}
+
+void
+test_3 (void *buf)
+{
+ int fd = -1;
+ read (fd, buf, 1); /* { dg-warning "'read' on possibly invalid file
descriptor 'fd'" } */
+ /* { dg-message "\\(1\\) 'fd' could be invalid" "" { target *-*-* }
.-1 } */
+}
+
+void
+test_4 (void *buf)
+{
+ int fd = STDIN;
+ read (fd, buf, 1);
+ close(fd);
+}
+
+void
+test_5 (char *path, void *buf)
+{
+ int flags = O_RDONLY;
+ if (some_condition())
+ flags |= O_NOATIME;
+ int fd = open (path, flags);
+ read(fd, buf, 1); /* { dg-warning "'read' on possibly invalid file
descriptor 'fd'" } */
+ /* { dg-message "\\(1\\) 'fd' could be invalid" "" { target *-*-* }
.-1 } */
+ close(fd);
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-4.c
b/gcc/testsuite/gcc.dg/analyzer/fd-4.c
new file mode 100644
index 00000000000..56828765bdd
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-4.c
@@ -0,0 +1,60 @@
+int open(const char *, int mode);
+void close(int fd);
+int write (int fd, void *buf, int nbytes);
+int read (int fd, void *buf, int nbytes);
+
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+
+
+void
+test_1 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDONLY); /* { dg-message "opened here as
read-only" } */
+ if (fd >= 0) /* { dg-message "assuming 'fd' is a valid file descriptor
\\(>= 0\\)" "event1" } */
+ /* { dg-message "following 'true' branch \\(when 'fd >= 0'\\)..."
"event2" { target *-*-* } .-1 } */
+ {
+ write (fd, buf, 1); /* { dg-warning "'write' on 'read-only' file
descriptor 'fd'" "warning" } */
+ /* { dg-message "\\(4\\) ...to here" "event1" { target *-*-* } .-1
} */
+ /* { dg-message "\\(5\\) 'write' on 'read-only' file descriptor
'fd'" "event2" { target *-*-* } .-2 } */
+ close (fd);
+ }
+}
+
+void
+test_2 (const char *path, void *buf)
+{
+ int fd = open (path, O_WRONLY); /* { dg-message "opened here as
write-only" } */
+ if (fd >= 0) /* { dg-message "assuming 'fd' is a valid file descriptor
\\(>= 0\\)" "event1" } */
+ /* { dg-message "following 'true' branch \\(when 'fd >= 0'\\)..."
"event2" { target *-*-* } .-1 } */
+ {
+ read (fd, buf, 1); /* { dg-warning "'read' on 'write-only' file
descriptor 'fd'" "warning" } */
+ /* { dg-message "\\(4\\) ...to here" "event1" { target *-*-* } .-1
} */
+ /* { dg-message "\\(5\\) 'read' on 'write-only' file descriptor
'fd'" "event2" { target *-*-* } .-2 } */
+ close (fd);
+ }
+}
+
+
+void test_4 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ if (fd >= 0)
+ {
+ close(fd); /* {dg-message "\\(2\\) closed here"} */
+ read(fd, buf, 1); /* { dg-warning "'read' on closed file
descriptor 'fd'" } */
+ /* {dg-message "\\(3\\) 'read' on closed file descriptor 'fd'
here" "" {target *-*-*} .-1 } */
+ }
+}
+
+void test_5 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ if (fd >= 0)
+ {
+ close(fd); /* {dg-message "\\(2\\) closed here"} */
+ write(fd, buf, 1); /* { dg-warning "'write' on closed file
descriptor 'fd'" } */
+ /* {dg-message "\\(3\\) 'write' on closed file descriptor 'fd'
here" "" {target *-*-*} .-1 } */
+ }
+}
\ No newline at end of file
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] analyzer PR 106003
2022-06-26 15:53 ` David Malcolm
@ 2022-06-27 17:07 ` Mir Immad
0 siblings, 0 replies; 8+ messages in thread
From: Mir Immad @ 2022-06-27 17:07 UTC (permalink / raw)
To: David Malcolm, gcc
Thanks for the suggestions, Dave.
+ }
> + else
> + {
> + /* FIXME: add leak reporting */
> + }
> +}
> +
> Please add a testcase for this, with an "xfail" in the dg-warning
> directive.
I fixed it and made other suggested changes. All the tests (for fds) are
passing.
Sending an updated patch in a separate email.
Thanks.
Immad.
On Sun, Jun 26, 2022 at 9:23 PM David Malcolm <dmalcolm@redhat.com> wrote:
> Thanks for the updated patch.
>
> Various comments inline below.
>
> Sorry if this seems nitpicky in places.
>
> I think the patch is nearly ready; please write a ChangeLog entry for
> the next one (none of my proposed changes are going to affect what the
> ChangeLog will look like, apart from new test case files perhaps, given
> that much of it is going to be just:
>
> * sm-fd.cc: New file.
>
> [...snip...]
>
> > diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
> > new file mode 100644
> > index 00000000000..83eb0df724d
> > --- /dev/null
> > +++ b/gcc/analyzer/sm-fd.cc
>
> [...snip...]
>
> > +class fd_leak : public fd_diagnostic
> > +{
> > +public:
> > + fd_leak (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm,
> arg)
> > {}
> > +
> > + const char *
> > + get_kind () const final override
> > + {
> > + return "fd_leak";
> > + }
> > +
> > + int
> > + get_controlling_option () const final override
> > + {
> > + return OPT_Wanalyzer_fd_leak;
> > + }
> > +
> > + bool
> > + emit (rich_location *rich_loc) final override
> > + {
> > + /*CWE-775: Missing Release of File Descriptor or Handle after
> Effective
> > + * Lifetime
> > + */
>
> Formatting nit: we don't put these "*" on contination lines in
> multiline comments; this should read:
>
> > + /* CWE-775: Missing Release of File Descriptor or Handle after
> Effective
> > + Lifetime. */
>
>
> [...snip...]
>
> > +class fd_access_mode_mismatch : public fd_diagnostic
> > +{
> > +public:
> > + /* mode specfies whether read on write was attempted or vice versa.
>
> Typo: "specfies" -> "specifies"
>
> [...snip...]
>
> > + bool
> > + emit (rich_location *rich_loc) final override
> > + {
> > +
> > + switch (m_mode)
> > + {
>
> I like to put in a "default: gcc_unreachable ();" in switch statements,
> especially one that isn't covering all of the values in the enum, but
> this got me thinking: this class has "enum access_mode m_mode;", but
> that enum is describing a *set* of ways in which the FD can be validly
> accessed, whereas here we're trying to describe a particular way in
> which the FD is being invalidly accessed.
>
> analyzer.h has this enum:
>
> /* An enum for describing the direction of an access to memory. */
>
> enum access_direction
> {
> DIR_READ,
> DIR_WRITE
> };
>
> which I think would be a much better fit here for
> fd_access_mode_mismatch, so please change fd_access_mode_mismatch's:
> enum access_mode m_mode;
> to
> enum access_direction m_access_dir;
> and update this switch accordingly:
>
> > + case READ_ONLY:
> > + return warning_at (rich_loc, get_controlling_option (),
> > + "%qE on %<read-only%> file descriptor %qE",
> > + m_callee_fndecl, m_arg);
> > + case WRITE_ONLY:
> > + return warning_at (rich_loc, get_controlling_option (),
> > + "%qE on %<write-only%> file descriptor %qE",
> > + m_callee_fndecl, m_arg);
> > + }
>
> [...snip...]
>
> > +
> > + label_text
> > + describe_state_change (const evdesc::state_change &change) override
> > + {
> > + return fd_diagnostic::describe_state_change (change);
> > + }
>
> This vfunc is redundant; it can be deleted so we simply inherit the
> fd_diagnostic implementation directly.
>
> > +
> > + label_text
> > + describe_final_event (const evdesc::final_event &ev) final override
> > + {
> > + switch (m_mode)
> > + {
> > + case READ_ONLY:
> > + return ev.formatted_print ("%qE on %<read-only%> file descriptor
> > %qE",
> > + m_callee_fndecl, m_arg);
> > + case WRITE_ONLY:
> > + return ev.formatted_print ("%qE on %<write-only%> file
> descriptor
> > %qE",
> > + m_callee_fndecl, m_arg);
> > + }
> > + }
>
> Similar comments here about the switch (m_mode) as per the emit vfunc.
>
>
> > +
> > +private:
> > + enum access_mode m_mode;
>
> ...and this field.
>
> > + const tree m_callee_fndecl;
> > +};
>
>
> [...snip...]
>
> > + label_text
> > + describe_state_change (const evdesc::state_change &change) override
> > + {
> > + if (m_sm.is_unchecked (change.m_new_state))
> > + {
> > + return label_text::borrow ("opened here");
> > + }
> > +
> > + if (change.m_new_state == m_sm.m_closed)
> > + {
> > + return change.formatted_print ("closed here");
> > + }
>
> Formatting nit: we don't add the { } when they're redundant, like in
> these "if" stmts (I dislike this rule in our conventions, but it's
> probably best to be consistent).
>
> [...snip...]
>
>
> > +bool
> > +fd_state_machine::check_for_closed_fd (state_t state) const
> > +{
> > + return (state == m_closed);
> > +}
> > +
> > +bool
> > +fd_state_machine::check_for_invalid_fd (state_t state) const
> > +{
> > + return (state == m_invalid);
> > +}
> > +
> > +bool
> > +fd_state_machine::check_for_constant_fd (state_t state) const
> > +{
> > + return (state == m_constant_fd);
>
>
> You're using the prefix "check_for_" for two different kinds of
> functions:
> (a) the functions above are simple predicate functions that merely
> determine "is this state_t a <foo>" for some foo, and have no side-
> effects
>
> (b) the "check_for" functions later on *do* things: they issue warnings
> and make state transitions
>
> For clarity, please rename the type (a) "check_for_foo" functions to
> "is_foo_p" (we use the "_p" suffix to indicate a simple predicate
> without side-effects), and keep the "check_for_" prefix for the type
> (b) functions that actually make changes.
>
> Hence please rename the above to:
>
> fd_state_machine::is_closed_fd_p
> fd_state_machine::is_invalid_fd_p
> fd_state_machine::is_constant_fd_p
>
> so that the "check_for_" prefix for the functions lower down imply that
> warnings could be issued and state transitions could occur.
>
> > +void
> > +fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
> > + const gimple *stmt, const gcall *call) const
> > +{
> > + tree lhs = gimple_call_lhs (call);
> > + if (lhs)
> > + {
> > + tree arg = gimple_call_arg (call, 1);
> > + if (TREE_CODE (arg) == INTEGER_CST)
> > + {
> > + int flag = TREE_INT_CST_LOW (arg);
> > + enum access_mode mode = get_access_mode_from_flag (flag);
> > +
> > + switch (mode)
> > + {
> > + case READ_ONLY:
> > + sm_ctxt->on_transition (node, stmt, lhs, m_start,
> > + m_unchecked_read_only);
> > + break;
> > + case WRITE_ONLY:
> > + sm_ctxt->on_transition (node, stmt, lhs, m_start,
> > + m_unchecked_write_only);
> > + break;
> > + default:
> > + sm_ctxt->on_transition (node, stmt, lhs, m_start,
> > + m_unchecked_read_write);
> > + }
> > + }
>
> If the user writes code where the flag is determined with logic, then
> "arg" won't be an INTEGER_CST and we won't transition the lhs; consider
> e.g.:
>
> int flags = O_RDONLY;
> if (some_condition ())
> flags |= O_NOATIME;
> int fd = open (path, flags)
>
> I don't think we need to handle this yet, but let's add test coverage
> for it, at least.
>
>
> > + }
> > + else
> > + {
> > + /* FIXME: add leak reporting */
> > + }
> > +}
> > +
>
> Please add a testcase for this, with an "xfail" in the dg-warning
> directive.
>
>
> [...snip...]
>
>
> > +void
> > +fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
> > + const gimple *stmt, const gcall *call,
> > + const tree callee_fndecl) const
> > +{
> > + tree arg = gimple_call_arg (call, 0);
> > + state_t state = sm_ctxt->get_state (stmt, arg);
> > + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > +
> > + sm_ctxt->on_transition (node, stmt, arg, m_start, m_closed);
> > + sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_write,
> > m_closed);
> > + sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_only,
> > m_closed);
> > + sm_ctxt->on_transition (node, stmt, arg, m_unchecked_write_only,
> > m_closed);
> > + sm_ctxt->on_transition (node, stmt, arg, m_valid_read_write,
> m_closed);
> > + sm_ctxt->on_transition (node, stmt, arg, m_valid_read_only, m_closed);
> > + sm_ctxt->on_transition (node, stmt, arg, m_valid_write_only,
> m_closed);
> > + sm_ctxt->on_transition (node, stmt, arg, m_constant_fd, m_closed);
> > +
> > + if (check_for_closed_fd (state))
> > + {
> > +
> > + sm_ctxt->warn (node, stmt, arg, new double_close (*this,
> diag_arg));
> > + sm_ctxt->set_next_state (stmt, arg, m_stop);
> > + }
> > +}
> > +void
> > +fd_state_machine::on_read (sm_context *sm_ctxt, const supernode *node,
> > + const gimple *stmt, const gcall *call,
> > + const tree callee_fndecl) const
> > +{
> > + check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, true);
> > +}
> > +void
> > +fd_state_machine::on_write (sm_context *sm_ctxt, const supernode *node,
> > + const gimple *stmt, const gcall *call,
> > + const tree callee_fndecl) const
> > +{
> > + check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, false);
> > +}
> > +
> > +void
> > +fd_state_machine::check_for_open_fd (sm_context *sm_ctxt, const
> supernode
> > *node,
> > + const gimple *stmt, const gcall
> *call,
> > + const tree callee_fndecl,
> > + bool toggle) const
>
> "bool" arguments to functions are often a "code smell".
>
> This will be much clearer if you change "bool toggle" to "enum
> access_direction access_dir"...
>
> > +{
> > + tree arg = gimple_call_arg (call, 0);
> > + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > + state_t state = sm_ctxt->get_state (stmt, arg);
> > + enum access_mode mode = get_access_mode_from_state (state);
>
> ...and rename "mode" to "fd_mode", since that way it's clear to the
> reader that we're comparing how the FD is being accessed with how the
> FD expects to be accessed.
>
> > +
> > + if (check_for_closed_fd (state))
> > + {
> > + sm_ctxt->warn (node, stmt, arg,
> > + new fd_use_after_close (*this, diag_arg,
> > callee_fndecl));
> > + }
> > +
> > + else
> > + {
> > +
> > + if (!is_valid (state))
> > + {
> > + if (!check_for_constant_fd (state))
> > + sm_ctxt->warn (
> > + node, stmt, arg,
> > + new unchecked_use_of_fd (*this, diag_arg,
> callee_fndecl));
> > + }
> > + if (toggle)
> > + /* This condition determines whether we are considering "read"
> or
> > + "write" function */
>
> and so (hopefully) the above comment becomes redundant.
>
> > + {
> > + if (mode == WRITE_ONLY)
> > + {
> > + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > + sm_ctxt->warn (node, stmt, arg,
> > + new fd_access_mode_mismatch (*this,
> diag_arg,
> > mode,
> > +
> callee_fndecl));
> > + }
> > + }
> > + else
> > + {
> > + if (mode == READ_ONLY)
> > + {
> > + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > + sm_ctxt->warn (node, stmt, arg,
> > + new fd_access_mode_mismatch (*this,
> diag_arg,
> > mode,
> > +
> callee_fndecl));
> > + }
> > + }
> > + }
> > +}
>
> [...snip...]
>
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index 174bc09e5cf..e5522b95ad2 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
>
> [...snip...]
>
> > +@item -Wno-analyzer-fd-access-mode-mismatch
> > +@opindex Wanalyzer-fd-access-mode-mismatch
> > +@opindex Wno-analyzer-fd-access-mode-mismatch
> > +This warning requires @option{-fanalyzer}, which enables it; use
> > +@option{-Wno-analyzer-fd-access-mode-mismatch}
> > +to disable it.
> > +
> > +This diagnostic warns for paths through code in which a
> > +@code{read} on write-only file descriptor is attempted, or vice versa
>
> Grammar nit: "on write-only" -> "on a write-only"
>
> > +@item -Wno-analyzer-fd-double-close
> > +@opindex Wanalyzer-fd-double-close
> > +@opindex Wno-analyzer-fd-double-close
> > +This warning requires @option{-fanalyzer}, which enables it; use
> > +@option{-Wno-analyzer-fd-double-close}
> > +to disable it.
> > +
> > +This diagnostic warns for paths through code in which a
> > +file descriptor can closed more than once.
>
> Grammar nit: "can closed" -> "can be closed".
>
> > +@item -Wno-analyzer-fd-leak
> > +@opindex Wanalyzer-fd-leak
> > +@opindex Wno-analyzer-fd-leak
> > +This warning requires @option{-fanalyzer}, which enables it; use
> > +@option{-Wno-analyzer-fd-leak}
> > +to disable it.
> > +
> > +This diagnostic warns for paths through code in which a
> > +file descriptor is leaked.
>
> More precise as: "an open file descriptor".
>
> [...snip...]
>
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-3.c
> > b/gcc/testsuite/gcc.dg/analyzer/fd-3.c
> > new file mode 100644
> > index 00000000000..d8c2ff26098
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-3.c
> > @@ -0,0 +1,43 @@
> > +int open(const char *, int mode);
> > +void close(int fd);
> > +int write (int fd, void *buf, int nbytes);
> > +int read (int fd, void *buf, int nbytes);
> > +
> > +#define O_RDONLY 0
> > +#define O_WRONLY 1
> > +#define O_RDWR 2
> > +#define STDIN 0
>
> [...snip...]
>
> > +void
> > +test_4 (void *buf)
> > +{
> > + int fd = STDIN;
> > + write (fd, buf, 1);
> > + close(fd);
> > +}
>
> Probably worth adding a comment to this test case, something along the
> lines of:
>
> FD 0 is stdin at the entry to "main" and thus read-only, but we have no
> guarantees here that it hasn't been closed and then reopened for
> writing, so we can't issue a warning.
>
> or similar.
>
> [...snip...]
>
> Thanks again for the patch; hope the above makes sense
>
> Dave
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] analyzer PR 106003
2022-06-25 16:00 Mir Immad
@ 2022-06-26 15:53 ` David Malcolm
2022-06-27 17:07 ` Mir Immad
0 siblings, 1 reply; 8+ messages in thread
From: David Malcolm @ 2022-06-26 15:53 UTC (permalink / raw)
To: Mir Immad, gcc
Thanks for the updated patch.
Various comments inline below.
Sorry if this seems nitpicky in places.
I think the patch is nearly ready; please write a ChangeLog entry for
the next one (none of my proposed changes are going to affect what the
ChangeLog will look like, apart from new test case files perhaps, given
that much of it is going to be just:
* sm-fd.cc: New file.
[...snip...]
> diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
> new file mode 100644
> index 00000000000..83eb0df724d
> --- /dev/null
> +++ b/gcc/analyzer/sm-fd.cc
[...snip...]
> +class fd_leak : public fd_diagnostic
> +{
> +public:
> + fd_leak (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
> {}
> +
> + const char *
> + get_kind () const final override
> + {
> + return "fd_leak";
> + }
> +
> + int
> + get_controlling_option () const final override
> + {
> + return OPT_Wanalyzer_fd_leak;
> + }
> +
> + bool
> + emit (rich_location *rich_loc) final override
> + {
> + /*CWE-775: Missing Release of File Descriptor or Handle after Effective
> + * Lifetime
> + */
Formatting nit: we don't put these "*" on contination lines in
multiline comments; this should read:
> + /* CWE-775: Missing Release of File Descriptor or Handle after
Effective
> + Lifetime. */
[...snip...]
> +class fd_access_mode_mismatch : public fd_diagnostic
> +{
> +public:
> + /* mode specfies whether read on write was attempted or vice versa.
Typo: "specfies" -> "specifies"
[...snip...]
> + bool
> + emit (rich_location *rich_loc) final override
> + {
> +
> + switch (m_mode)
> + {
I like to put in a "default: gcc_unreachable ();" in switch statements,
especially one that isn't covering all of the values in the enum, but
this got me thinking: this class has "enum access_mode m_mode;", but
that enum is describing a *set* of ways in which the FD can be validly
accessed, whereas here we're trying to describe a particular way in
which the FD is being invalidly accessed.
analyzer.h has this enum:
/* An enum for describing the direction of an access to memory. */
enum access_direction
{
DIR_READ,
DIR_WRITE
};
which I think would be a much better fit here for
fd_access_mode_mismatch, so please change fd_access_mode_mismatch's:
enum access_mode m_mode;
to
enum access_direction m_access_dir;
and update this switch accordingly:
> + case READ_ONLY:
> + return warning_at (rich_loc, get_controlling_option (),
> + "%qE on %<read-only%> file descriptor %qE",
> + m_callee_fndecl, m_arg);
> + case WRITE_ONLY:
> + return warning_at (rich_loc, get_controlling_option (),
> + "%qE on %<write-only%> file descriptor %qE",
> + m_callee_fndecl, m_arg);
> + }
[...snip...]
> +
> + label_text
> + describe_state_change (const evdesc::state_change &change) override
> + {
> + return fd_diagnostic::describe_state_change (change);
> + }
This vfunc is redundant; it can be deleted so we simply inherit the
fd_diagnostic implementation directly.
> +
> + label_text
> + describe_final_event (const evdesc::final_event &ev) final override
> + {
> + switch (m_mode)
> + {
> + case READ_ONLY:
> + return ev.formatted_print ("%qE on %<read-only%> file descriptor
> %qE",
> + m_callee_fndecl, m_arg);
> + case WRITE_ONLY:
> + return ev.formatted_print ("%qE on %<write-only%> file descriptor
> %qE",
> + m_callee_fndecl, m_arg);
> + }
> + }
Similar comments here about the switch (m_mode) as per the emit vfunc.
> +
> +private:
> + enum access_mode m_mode;
...and this field.
> + const tree m_callee_fndecl;
> +};
[...snip...]
> + label_text
> + describe_state_change (const evdesc::state_change &change) override
> + {
> + if (m_sm.is_unchecked (change.m_new_state))
> + {
> + return label_text::borrow ("opened here");
> + }
> +
> + if (change.m_new_state == m_sm.m_closed)
> + {
> + return change.formatted_print ("closed here");
> + }
Formatting nit: we don't add the { } when they're redundant, like in
these "if" stmts (I dislike this rule in our conventions, but it's
probably best to be consistent).
[...snip...]
> +bool
> +fd_state_machine::check_for_closed_fd (state_t state) const
> +{
> + return (state == m_closed);
> +}
> +
> +bool
> +fd_state_machine::check_for_invalid_fd (state_t state) const
> +{
> + return (state == m_invalid);
> +}
> +
> +bool
> +fd_state_machine::check_for_constant_fd (state_t state) const
> +{
> + return (state == m_constant_fd);
You're using the prefix "check_for_" for two different kinds of
functions:
(a) the functions above are simple predicate functions that merely
determine "is this state_t a <foo>" for some foo, and have no side-
effects
(b) the "check_for" functions later on *do* things: they issue warnings
and make state transitions
For clarity, please rename the type (a) "check_for_foo" functions to
"is_foo_p" (we use the "_p" suffix to indicate a simple predicate
without side-effects), and keep the "check_for_" prefix for the type
(b) functions that actually make changes.
Hence please rename the above to:
fd_state_machine::is_closed_fd_p
fd_state_machine::is_invalid_fd_p
fd_state_machine::is_constant_fd_p
so that the "check_for_" prefix for the functions lower down imply that
warnings could be issued and state transitions could occur.
> +void
> +fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const gcall *call) const
> +{
> + tree lhs = gimple_call_lhs (call);
> + if (lhs)
> + {
> + tree arg = gimple_call_arg (call, 1);
> + if (TREE_CODE (arg) == INTEGER_CST)
> + {
> + int flag = TREE_INT_CST_LOW (arg);
> + enum access_mode mode = get_access_mode_from_flag (flag);
> +
> + switch (mode)
> + {
> + case READ_ONLY:
> + sm_ctxt->on_transition (node, stmt, lhs, m_start,
> + m_unchecked_read_only);
> + break;
> + case WRITE_ONLY:
> + sm_ctxt->on_transition (node, stmt, lhs, m_start,
> + m_unchecked_write_only);
> + break;
> + default:
> + sm_ctxt->on_transition (node, stmt, lhs, m_start,
> + m_unchecked_read_write);
> + }
> + }
If the user writes code where the flag is determined with logic, then
"arg" won't be an INTEGER_CST and we won't transition the lhs; consider
e.g.:
int flags = O_RDONLY;
if (some_condition ())
flags |= O_NOATIME;
int fd = open (path, flags)
I don't think we need to handle this yet, but let's add test coverage
for it, at least.
> + }
> + else
> + {
> + /* FIXME: add leak reporting */
> + }
> +}
> +
Please add a testcase for this, with an "xfail" in the dg-warning
directive.
[...snip...]
> +void
> +fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const gcall *call,
> + const tree callee_fndecl) const
> +{
> + tree arg = gimple_call_arg (call, 0);
> + state_t state = sm_ctxt->get_state (stmt, arg);
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> +
> + sm_ctxt->on_transition (node, stmt, arg, m_start, m_closed);
> + sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_write,
> m_closed);
> + sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_only,
> m_closed);
> + sm_ctxt->on_transition (node, stmt, arg, m_unchecked_write_only,
> m_closed);
> + sm_ctxt->on_transition (node, stmt, arg, m_valid_read_write, m_closed);
> + sm_ctxt->on_transition (node, stmt, arg, m_valid_read_only, m_closed);
> + sm_ctxt->on_transition (node, stmt, arg, m_valid_write_only, m_closed);
> + sm_ctxt->on_transition (node, stmt, arg, m_constant_fd, m_closed);
> +
> + if (check_for_closed_fd (state))
> + {
> +
> + sm_ctxt->warn (node, stmt, arg, new double_close (*this, diag_arg));
> + sm_ctxt->set_next_state (stmt, arg, m_stop);
> + }
> +}
> +void
> +fd_state_machine::on_read (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const gcall *call,
> + const tree callee_fndecl) const
> +{
> + check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, true);
> +}
> +void
> +fd_state_machine::on_write (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const gcall *call,
> + const tree callee_fndecl) const
> +{
> + check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, false);
> +}
> +
> +void
> +fd_state_machine::check_for_open_fd (sm_context *sm_ctxt, const supernode
> *node,
> + const gimple *stmt, const gcall *call,
> + const tree callee_fndecl,
> + bool toggle) const
"bool" arguments to functions are often a "code smell".
This will be much clearer if you change "bool toggle" to "enum
access_direction access_dir"...
> +{
> + tree arg = gimple_call_arg (call, 0);
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + state_t state = sm_ctxt->get_state (stmt, arg);
> + enum access_mode mode = get_access_mode_from_state (state);
...and rename "mode" to "fd_mode", since that way it's clear to the
reader that we're comparing how the FD is being accessed with how the
FD expects to be accessed.
> +
> + if (check_for_closed_fd (state))
> + {
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_use_after_close (*this, diag_arg,
> callee_fndecl));
> + }
> +
> + else
> + {
> +
> + if (!is_valid (state))
> + {
> + if (!check_for_constant_fd (state))
> + sm_ctxt->warn (
> + node, stmt, arg,
> + new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
> + }
> + if (toggle)
> + /* This condition determines whether we are considering "read" or
> + "write" function */
and so (hopefully) the above comment becomes redundant.
> + {
> + if (mode == WRITE_ONLY)
> + {
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_access_mode_mismatch (*this, diag_arg,
> mode,
> + callee_fndecl));
> + }
> + }
> + else
> + {
> + if (mode == READ_ONLY)
> + {
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_access_mode_mismatch (*this, diag_arg,
> mode,
> + callee_fndecl));
> + }
> + }
> + }
> +}
[...snip...]
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 174bc09e5cf..e5522b95ad2 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
[...snip...]
> +@item -Wno-analyzer-fd-access-mode-mismatch
> +@opindex Wanalyzer-fd-access-mode-mismatch
> +@opindex Wno-analyzer-fd-access-mode-mismatch
> +This warning requires @option{-fanalyzer}, which enables it; use
> +@option{-Wno-analyzer-fd-access-mode-mismatch}
> +to disable it.
> +
> +This diagnostic warns for paths through code in which a
> +@code{read} on write-only file descriptor is attempted, or vice versa
Grammar nit: "on write-only" -> "on a write-only"
> +@item -Wno-analyzer-fd-double-close
> +@opindex Wanalyzer-fd-double-close
> +@opindex Wno-analyzer-fd-double-close
> +This warning requires @option{-fanalyzer}, which enables it; use
> +@option{-Wno-analyzer-fd-double-close}
> +to disable it.
> +
> +This diagnostic warns for paths through code in which a
> +file descriptor can closed more than once.
Grammar nit: "can closed" -> "can be closed".
> +@item -Wno-analyzer-fd-leak
> +@opindex Wanalyzer-fd-leak
> +@opindex Wno-analyzer-fd-leak
> +This warning requires @option{-fanalyzer}, which enables it; use
> +@option{-Wno-analyzer-fd-leak}
> +to disable it.
> +
> +This diagnostic warns for paths through code in which a
> +file descriptor is leaked.
More precise as: "an open file descriptor".
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-3.c
> b/gcc/testsuite/gcc.dg/analyzer/fd-3.c
> new file mode 100644
> index 00000000000..d8c2ff26098
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/fd-3.c
> @@ -0,0 +1,43 @@
> +int open(const char *, int mode);
> +void close(int fd);
> +int write (int fd, void *buf, int nbytes);
> +int read (int fd, void *buf, int nbytes);
> +
> +#define O_RDONLY 0
> +#define O_WRONLY 1
> +#define O_RDWR 2
> +#define STDIN 0
[...snip...]
> +void
> +test_4 (void *buf)
> +{
> + int fd = STDIN;
> + write (fd, buf, 1);
> + close(fd);
> +}
Probably worth adding a comment to this test case, something along the
lines of:
FD 0 is stdin at the entry to "main" and thus read-only, but we have no
guarantees here that it hasn't been closed and then reopened for
writing, so we can't issue a warning.
or similar.
[...snip...]
Thanks again for the patch; hope the above makes sense
Dave
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] analyzer PR 106003
@ 2022-06-25 16:00 Mir Immad
2022-06-26 15:53 ` David Malcolm
0 siblings, 1 reply; 8+ messages in thread
From: Mir Immad @ 2022-06-25 16:00 UTC (permalink / raw)
To: gcc, David Malcolm
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index b6dcc45a58a..04631f737ea 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -1269,6 +1269,7 @@ ANALYZER_OBJS = \
analyzer/region-model-reachability.o \
analyzer/sm.o \
analyzer/sm-file.o \
+ analyzer/sm-fd.o \
analyzer/sm-malloc.o \
analyzer/sm-pattern-test.o \
analyzer/sm-sensitive.o \
diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index 23dfc797cea..219f6180130 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -66,6 +66,26 @@ Wanalyzer-exposure-through-output-file
Common Var(warn_analyzer_exposure_through_output_file) Init(1) Warning
Warn about code paths in which sensitive data is written to a file.
+Wanalyzer-fd-access-mode-mismatch
+Common Var(warn_analyzer_fd_mode_mismatch) Init(1) Warning
+Warn about code paths in which read on write-only file descriptor is
attempted, or vice versa.
+
+Wanalyzer-fd-double-close
+Common Var(warn_analyzer_fd_double_close) Init(1) Warning
+Warn about code paths in which a file descriptor can be closed more than
once.
+
+Wanalyzer-fd-leak
+Common Var(warn_analyzer_fd_leak) Init(1) Warning
+Warn about code paths in which a file descriptor is not closed.
+
+Wanalyzer-fd-use-after-close
+Common Var(warn_analyzer_fd_use_after_close) Init(1) Warning
+Warn about code paths in which a read or write is performed on a closed
file descriptor.
+
+Wanalyzer-fd-use-without-check
+Common Var(warn_analyzer_fd_use_without_check) Init(1) Warning
+Warn about code paths in which a file descriptor is used without being
checked for validity.
+
Wanalyzer-file-leak
Common Var(warn_analyzer_file_leak) Init(1) Warning
Warn about code paths in which a stdio FILE is not closed.
diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
new file mode 100644
index 00000000000..83eb0df724d
--- /dev/null
+++ b/gcc/analyzer/sm-fd.cc
@@ -0,0 +1,821 @@
+/* A state machine for detecting misuses of POSIX file descriptor APIs.
+ Copyright (C) 2019-2022 Free Software Foundation, Inc.
+ Contributed by Immad Mir <mir@sourceware.org>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tree.h"
+#include "function.h"
+#include "basic-block.h"
+#include "gimple.h"
+#include "options.h"
+#include "diagnostic-path.h"
+#include "diagnostic-metadata.h"
+#include "function.h"
+#include "json.h"
+#include "analyzer/analyzer.h"
+#include "diagnostic-event-id.h"
+#include "analyzer/analyzer-logging.h"
+#include "analyzer/sm.h"
+#include "analyzer/pending-diagnostic.h"
+#include "analyzer/function-set.h"
+#include "analyzer/analyzer-selftests.h"
+#include "tristate.h"
+#include "selftest.h"
+#include "analyzer/call-string.h"
+#include "analyzer/program-point.h"
+#include "analyzer/store.h"
+#include "analyzer/region-model.h"
+
+#include <unistd.h>
+#if ENABLE_ANALYZER
+
+namespace ana
+{
+
+namespace
+{
+
+/* An enum for distinguishing between three different access modes. */
+
+enum access_mode
+{
+ READ_WRITE,
+ READ_ONLY,
+ WRITE_ONLY
+};
+
+class fd_state_machine : public state_machine
+{
+public:
+ fd_state_machine (logger *logger);
+
+ bool
+ inherited_state_p () const final override
+ {
+ return false;
+ }
+
+ state_machine::state_t
+ get_default_state (const svalue *sval) const final override
+ {
+ if (tree cst = sval->maybe_get_constant ())
+ {
+ int val = TREE_INT_CST_LOW (cst);
+ if (val >= 0)
+ return m_constant_fd;
+ else
+ return m_invalid;
+ }
+ return m_start;
+ }
+
+ bool on_stmt (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt) const final override;
+
+ void on_condition (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const svalue *lhs, const
tree_code op,
+ const svalue *rhs) const final override;
+
+ bool can_purge_p (state_t s) const final override;
+ pending_diagnostic *on_leak (tree var) const final override;
+
+ bool is_unchecked (state_t s) const;
+ bool is_valid (state_t s) const;
+ enum access_mode get_access_mode_from_flag (int flag) const;
+ enum access_mode get_access_mode_from_state (state_t s) const;
+ bool check_for_closed_fd (state_t s) const;
+ bool check_for_invalid_fd (state_t s) const;
+ bool check_for_constant_fd (state_t s) const;
+
+ /* State for a constant file descriptor (>= 0) */
+ state_t m_constant_fd;
+
+ /* States representing a file descriptor that hasn't yet been
+ checked for validity after opening, for three different
+ access modes. */
+ state_t m_unchecked_read_write;
+
+ state_t m_unchecked_read_only;
+
+ state_t m_unchecked_write_only;
+
+ /* States for reperesenting a file descriptor that is known to be valid
(>=
+ 0), for three different access modes.*/
+ state_t m_valid_read_write;
+
+ state_t m_valid_read_only;
+
+ state_t m_valid_write_only;
+
+ /* State for a file descriptor that is known to be invalid (< 0). */
+ state_t m_invalid;
+
+ /* State for a file descriptor that has been closed.*/
+ state_t m_closed;
+
+ /* State for a file descriptor that we do not want to track anymore . */
+ state_t m_stop;
+
+private:
+ void on_open (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call) const;
+ void on_close (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call, const tree callee_fndecl) const;
+ void on_read (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call, const tree callee_fndecl) const;
+ void on_write (sm_context *sm_ctxt, const supernode *node, const gimple
*stmt,
+ const gcall *call, const tree callee_fndecl) const;
+ void check_for_open_fd (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl, bool is_read) const;
+};
+
+/* Base diagnostic class relative to fd_state_machine. */
+class fd_diagnostic : public pending_diagnostic
+{
+public:
+ fd_diagnostic (const fd_state_machine &sm, tree arg) : m_sm (sm), m_arg
(arg)
+ {
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ return same_tree_p (m_arg, ((const fd_diagnostic &)base_other).m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (change.m_old_state == m_sm.get_start_state ()
+ && m_sm.is_unchecked (change.m_new_state))
+ {
+ if (change.m_new_state == m_sm.m_unchecked_read_write)
+ return change.formatted_print ("opened here as read-write");
+
+ if (change.m_new_state == m_sm.m_unchecked_read_only)
+ return change.formatted_print ("opened here as read-only");
+
+ if (change.m_new_state == m_sm.m_unchecked_write_only)
+ return change.formatted_print ("opened here as write-only");
+ }
+
+ if (change.m_new_state == m_sm.m_closed)
+ return change.formatted_print ("closed here");
+
+ if (m_sm.is_unchecked (change.m_old_state)
+ && m_sm.is_valid (change.m_new_state))
+ if (change.m_expr)
+ return change.formatted_print (
+ "assuming %qE is a valid file descriptor (>= 0)",
change.m_expr);
+ else
+ return change.formatted_print ("assuming a valid file descriptor");
+
+ if (m_sm.is_unchecked (change.m_old_state)
+ && change.m_new_state == m_sm.m_invalid)
+ if (change.m_expr)
+ return change.formatted_print (
+ "assuming %qE is an invalid file descriptor (< 0)",
change.m_expr);
+ else
+ return change.formatted_print ("assuming an invalid file
descriptor");
+
+ return label_text ();
+ }
+
+protected:
+ const fd_state_machine &m_sm;
+ tree m_arg;
+};
+
+class fd_leak : public fd_diagnostic
+{
+public:
+ fd_leak (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
{}
+
+ const char *
+ get_kind () const final override
+ {
+ return "fd_leak";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_leak;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ /*CWE-775: Missing Release of File Descriptor or Handle after Effective
+ * Lifetime
+ */
+ diagnostic_metadata m;
+ m.add_cwe (775);
+ if (m_arg)
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "leak of file descriptor %qE", m_arg);
+ else
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "leak of file descriptor");
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) final override
+ {
+ if (m_sm.is_unchecked (change.m_new_state))
+ {
+ m_open_event = change.m_event_id;
+ return label_text::borrow ("opened here");
+ }
+
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ if (m_open_event.known_p ())
+ {
+ if (ev.m_expr)
+ return ev.formatted_print ("%qE leaks here; was opened at %@",
+ ev.m_expr, &m_open_event);
+ else
+ return ev.formatted_print ("leaks here; was opened at %@",
+ &m_open_event);
+ }
+ else
+ {
+ if (ev.m_expr)
+ return ev.formatted_print ("%qE leaks here", ev.m_expr);
+ else
+ return ev.formatted_print ("leaks here");
+ }
+ }
+
+private:
+ diagnostic_event_id_t m_open_event;
+};
+
+class fd_access_mode_mismatch : public fd_diagnostic
+{
+public:
+ /* mode specfies whether read on write was attempted or vice versa.
+ */
+ fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
+ enum access_mode mode, const tree callee_fndecl)
+ : m_mode (mode), m_callee_fndecl (callee_fndecl), fd_diagnostic (sm,
arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "fd_access_mode_mismatch";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_access_mode_mismatch;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+
+ switch (m_mode)
+ {
+ case READ_ONLY:
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on %<read-only%> file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ case WRITE_ONLY:
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on %<write-only%> file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ }
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ const fd_access_mode_mismatch &sub_other
+ = (const fd_access_mode_mismatch &)base_other;
+ return (same_tree_p (m_arg, sub_other.m_arg)
+ && m_callee_fndecl == sub_other.m_callee_fndecl
+ && m_mode == sub_other.m_mode);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ switch (m_mode)
+ {
+ case READ_ONLY:
+ return ev.formatted_print ("%qE on %<read-only%> file descriptor
%qE",
+ m_callee_fndecl, m_arg);
+ case WRITE_ONLY:
+ return ev.formatted_print ("%qE on %<write-only%> file descriptor
%qE",
+ m_callee_fndecl, m_arg);
+ }
+ }
+
+private:
+ enum access_mode m_mode;
+ const tree m_callee_fndecl;
+};
+
+class double_close : public fd_diagnostic
+{
+public:
+ double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm,
arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "double_close";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_double_close;
+ }
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ diagnostic_metadata m;
+ // CWE-1341: Multiple Releases of Same Resource or Handle
+ m.add_cwe (1341);
+ return warning_meta (rich_loc, m, get_controlling_option (),
+ "double %<close%> of file descriptor %qE", m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (m_sm.is_unchecked (change.m_new_state))
+ return label_text::borrow ("opened here");
+
+ if (change.m_new_state == m_sm.m_closed)
+ {
+ m_first_close_event = change.m_event_id;
+ return change.formatted_print ("first %qs here", "close");
+ }
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ if (m_first_close_event.known_p ())
+ return ev.formatted_print ("second %qs here; first %qs was at %@",
+ "close", "close", &m_first_close_event);
+ return ev.formatted_print ("second %qs here", "close");
+ }
+
+private:
+ diagnostic_event_id_t m_first_close_event;
+};
+
+class fd_use_after_close : public fd_diagnostic
+{
+public:
+ fd_use_after_close (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl)
+ : m_callee_fndecl (callee_fndecl), fd_diagnostic (sm, arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "fd_use_after_close";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_use_after_close;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on closed file descriptor %qE",
m_callee_fndecl,
+ m_arg);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (m_sm.is_unchecked (change.m_new_state))
+ {
+ return label_text::borrow ("opened here");
+ }
+
+ if (change.m_new_state == m_sm.m_closed)
+ {
+ return change.formatted_print ("closed here");
+ }
+
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ return ev.formatted_print ("%qE on closed file descriptor %qE here",
+ m_callee_fndecl, m_arg);
+ }
+
+private:
+ const tree m_callee_fndecl;
+};
+
+class unchecked_use_of_fd : public fd_diagnostic
+{
+public:
+ unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl)
+ : m_callee_fndecl (callee_fndecl), fd_diagnostic (sm, arg)
+ {
+ }
+
+ const char *
+ get_kind () const final override
+ {
+ return "unchecked_use_of_fd";
+ }
+
+ int
+ get_controlling_option () const final override
+ {
+ return OPT_Wanalyzer_fd_use_without_check;
+ }
+
+ bool
+ emit (rich_location *rich_loc) final override
+ {
+
+ return warning_at (rich_loc, get_controlling_option (),
+ "%qE on possibly invalid file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ const unchecked_use_of_fd &sub_other
+ = (const unchecked_use_of_fd &)base_other;
+ return (same_tree_p (m_arg, sub_other.m_arg)
+ && m_callee_fndecl == sub_other.m_callee_fndecl);
+ }
+
+ label_text
+ describe_state_change (const evdesc::state_change &change) override
+ {
+ if (m_sm.is_unchecked (change.m_new_state))
+ {
+ m_first_open_event = change.m_event_id;
+ return label_text::borrow ("opened here");
+ }
+
+ return fd_diagnostic::describe_state_change (change);
+ }
+
+ label_text
+ describe_final_event (const evdesc::final_event &ev) final override
+ {
+ if (m_first_open_event.known_p ())
+ return ev.formatted_print (
+ "%qE could be invalid: unchecked value from %@", m_arg,
+ &m_first_open_event);
+ else
+ return ev.formatted_print ("%qE could be invalid", m_arg);
+ }
+
+private:
+ diagnostic_event_id_t m_first_open_event;
+ const tree m_callee_fndecl;
+};
+
+fd_state_machine::fd_state_machine (logger *logger)
+ : state_machine ("file-descriptor", logger),
+ m_constant_fd (add_state ("fd-constant")),
+ m_unchecked_read_write (add_state ("fd-unchecked-read-write")),
+ m_unchecked_read_only (add_state ("fd-unchecked-read-only")),
+ m_unchecked_write_only (add_state ("fd-unchecked-write-only")),
+ m_invalid (add_state ("fd-invalid")),
+ m_valid_read_write (add_state ("fd-valid-read-write")),
+ m_valid_read_only (add_state ("fd-valid-read-only")),
+ m_valid_write_only (add_state ("fd-valid-write-only")),
+ m_closed (add_state ("fd-closed")), m_stop (add_state ("fd-stop"))
+{
+}
+
+bool
+fd_state_machine::is_unchecked (state_t s) const
+{
+ return (s == m_unchecked_read_write
+ || s == m_unchecked_read_only
+ || s == m_unchecked_write_only);
+}
+
+bool
+fd_state_machine::is_valid (state_t s) const
+{
+ return (s == m_valid_read_write
+ || s == m_valid_read_only
+ || s == m_valid_write_only);
+}
+
+enum access_mode
+fd_state_machine::get_access_mode_from_flag (int flag) const
+{
+ /* FIXME: this code assumes the access modes on the host and
+ target are the same, which in practice might not be the case. */
+ if (flag == O_RDONLY)
+ {
+ return READ_ONLY;
+ }
+ else if (flag == O_WRONLY)
+ {
+ return WRITE_ONLY;
+ }
+ return READ_WRITE;
+}
+
+enum access_mode
+fd_state_machine::get_access_mode_from_state (state_t state) const
+{
+ if (state == m_unchecked_read_only || state == m_valid_read_only)
+ return READ_ONLY;
+ else if (state == m_unchecked_write_only || state == m_valid_write_only)
+ return WRITE_ONLY;
+ else if (state == m_unchecked_read_write || state == m_valid_read_write)
+ return READ_WRITE;
+}
+
+bool
+fd_state_machine::check_for_closed_fd (state_t state) const
+{
+ return (state == m_closed);
+}
+
+bool
+fd_state_machine::check_for_invalid_fd (state_t state) const
+{
+ return (state == m_invalid);
+}
+
+bool
+fd_state_machine::check_for_constant_fd (state_t state) const
+{
+ return (state == m_constant_fd);
+}
+
+bool
+fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt) const
+{
+
+ if (const gcall *call = dyn_cast<const gcall *> (stmt))
+ if (tree callee_fndecl = sm_ctxt->get_fndecl_for_call (call))
+ {
+ if (is_named_call_p (callee_fndecl, "open", call, 2))
+ {
+ on_open (sm_ctxt, node, stmt, call);
+ return true;
+ } // "open"
+
+ if (is_named_call_p (callee_fndecl, "close", call, 1))
+ {
+ on_close (sm_ctxt, node, stmt, call, callee_fndecl);
+ return true;
+ } // "close"
+
+ if (is_named_call_p (callee_fndecl, "write", call, 3))
+ {
+ on_write (sm_ctxt, node, stmt, call, callee_fndecl);
+ return true;
+ } // "write"
+
+ if (is_named_call_p (callee_fndecl, "read", call, 3))
+ {
+ on_read (sm_ctxt, node, stmt, call, callee_fndecl);
+ return true;
+ } // "read"
+ }
+
+ return false;
+}
+
+void
+fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call) const
+{
+ tree lhs = gimple_call_lhs (call);
+ if (lhs)
+ {
+ tree arg = gimple_call_arg (call, 1);
+ if (TREE_CODE (arg) == INTEGER_CST)
+ {
+ int flag = TREE_INT_CST_LOW (arg);
+ enum access_mode mode = get_access_mode_from_flag (flag);
+
+ switch (mode)
+ {
+ case READ_ONLY:
+ sm_ctxt->on_transition (node, stmt, lhs, m_start,
+ m_unchecked_read_only);
+ break;
+ case WRITE_ONLY:
+ sm_ctxt->on_transition (node, stmt, lhs, m_start,
+ m_unchecked_write_only);
+ break;
+ default:
+ sm_ctxt->on_transition (node, stmt, lhs, m_start,
+ m_unchecked_read_write);
+ }
+ }
+ }
+ else
+ {
+ /* FIXME: add leak reporting */
+ }
+}
+
+void
+fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl) const
+{
+ tree arg = gimple_call_arg (call, 0);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+
+ sm_ctxt->on_transition (node, stmt, arg, m_start, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_write,
m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_unchecked_read_only,
m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_unchecked_write_only,
m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_valid_read_write, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_valid_read_only, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_valid_write_only, m_closed);
+ sm_ctxt->on_transition (node, stmt, arg, m_constant_fd, m_closed);
+
+ if (check_for_closed_fd (state))
+ {
+
+ sm_ctxt->warn (node, stmt, arg, new double_close (*this, diag_arg));
+ sm_ctxt->set_next_state (stmt, arg, m_stop);
+ }
+}
+void
+fd_state_machine::on_read (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl) const
+{
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, true);
+}
+void
+fd_state_machine::on_write (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl) const
+{
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, false);
+}
+
+void
+fd_state_machine::check_for_open_fd (sm_context *sm_ctxt, const supernode
*node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl,
+ bool toggle) const
+{
+ tree arg = gimple_call_arg (call, 0);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ enum access_mode mode = get_access_mode_from_state (state);
+
+ if (check_for_closed_fd (state))
+ {
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_after_close (*this, diag_arg,
callee_fndecl));
+ }
+
+ else
+ {
+
+ if (!is_valid (state))
+ {
+ if (!check_for_constant_fd (state))
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
+ }
+ if (toggle)
+ /* This condition determines whether we are considering "read" or
+ "write" function */
+ {
+ if (mode == WRITE_ONLY)
+ {
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg,
mode,
+ callee_fndecl));
+ }
+ }
+ else
+ {
+ if (mode == READ_ONLY)
+ {
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg,
mode,
+ callee_fndecl));
+ }
+ }
+ }
+}
+
+void
+fd_state_machine::on_condition (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const svalue *lhs,
+ enum tree_code op, const svalue *rhs) const
+{
+
+ if (!rhs->all_zeroes_p ())
+ return;
+
+ if (op == GE_EXPR)
+ {
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_write,
+ m_valid_read_write);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_only,
+ m_valid_read_only);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_write_only,
+ m_valid_write_only);
+ }
+ else if (op == LT_EXPR)
+ {
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_write,
+ m_invalid);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_read_only,
+ m_invalid);
+ sm_ctxt->on_transition (node, stmt, lhs, m_unchecked_write_only,
+ m_invalid);
+ }
+}
+
+bool
+fd_state_machine::can_purge_p (state_t s) const
+{
+ if (is_unchecked (s) || is_valid (s))
+ return false;
+ else
+ return true;
+}
+
+pending_diagnostic *
+fd_state_machine::on_leak (tree var) const
+{
+ return new fd_leak (*this, var);
+}
+} // anonymus namespace
+
+state_machine *
+make_fd_state_machine (logger *logger)
+{
+ return new fd_state_machine (logger);
+}
+} // namespace ana
+
+#endif // ENABLE_ANALYZER
\ No newline at end of file
diff --git a/gcc/analyzer/sm.cc b/gcc/analyzer/sm.cc
index 622cb0b7ab3..24c20b894cd 100644
--- a/gcc/analyzer/sm.cc
+++ b/gcc/analyzer/sm.cc
@@ -167,6 +167,7 @@ make_checkers (auto_delete_vec <state_machine> &out,
logger *logger)
{
out.safe_push (make_malloc_state_machine (logger));
out.safe_push (make_fileptr_state_machine (logger));
+ out.safe_push (make_fd_state_machine (logger));
/* The "taint" checker must be explicitly enabled (as it currently
leads to state explosions that stop the other checkers working). */
if (flag_analyzer_checker)
diff --git a/gcc/analyzer/sm.h b/gcc/analyzer/sm.h
index 4cc54531c56..e80ef1fac37 100644
--- a/gcc/analyzer/sm.h
+++ b/gcc/analyzer/sm.h
@@ -301,6 +301,7 @@ extern state_machine *make_sensitive_state_machine
(logger *logger);
extern state_machine *make_signal_state_machine (logger *logger);
extern state_machine *make_pattern_test_state_machine (logger *logger);
extern state_machine *make_va_list_state_machine (logger *logger);
+extern state_machine *make_fd_state_machine (logger *logger);
} // namespace ana
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 174bc09e5cf..e5522b95ad2 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -9709,6 +9709,11 @@ Enabling this option effectively enables the
following warnings:
-Wanalyzer-double-fclose @gol
-Wanalyzer-double-free @gol
-Wanalyzer-exposure-through-output-file @gol
+-Wanalyzer-fd-access-mode-mismatch @gol
+-Wanalyzer-fd-double-close @gol
+-Wanalyzer-fd-leak @gol
+-Wanalyzer-fd-use-after-close @gol
+-Wanalyzer-fd-use-without-check @gol
-Wanalyzer-file-leak @gol
-Wanalyzer-free-of-non-heap @gol
-Wanalyzer-malloc-leak @gol
@@ -9783,6 +9788,56 @@ This diagnostic warns for paths through the code in
which a
security-sensitive value is written to an output file
(such as writing a password to a log file).
+@item -Wno-analyzer-fd-access-mode-mismatch
+@opindex Wanalyzer-fd-access-mode-mismatch
+@opindex Wno-analyzer-fd-access-mode-mismatch
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-access-mode-mismatch}
+to disable it.
+
+This diagnostic warns for paths through code in which a
+@code{read} on write-only file descriptor is attempted, or vice versa
+
+@item -Wno-analyzer-fd-double-close
+@opindex Wanalyzer-fd-double-close
+@opindex Wno-analyzer-fd-double-close
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-double-close}
+to disable it.
+
+This diagnostic warns for paths through code in which a
+file descriptor can closed more than once.
+
+@item -Wno-analyzer-fd-leak
+@opindex Wanalyzer-fd-leak
+@opindex Wno-analyzer-fd-leak
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-leak}
+to disable it.
+
+This diagnostic warns for paths through code in which a
+file descriptor is leaked.
+
+@item -Wno-analyzer-fd-use-after-close
+@opindex Wanalyzer-fd-use-after-close
+@opindex Wno-analyzer-fd-use-after-close
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-use-after-close}
+to disable it.
+
+This diagnostic warns for paths through code in which a
+read or write is called on a closed file descriptor.
+
+@item -Wno-analyzer-fd-use-without-check
+@opindex Wanalyzer-fd-use-without-check
+@opindex Wno-analyzer-fd-use-without-check
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-fd-use-without-check}
+to disable it.
+
+This diagnostic warns for paths through code in which a
+file descriptor is used without being checked for validity.
+
@item -Wno-analyzer-file-leak
@opindex Wanalyzer-file-leak
@opindex Wno-analyzer-file-leak
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-1.c
b/gcc/testsuite/gcc.dg/analyzer/fd-1.c
new file mode 100644
index 00000000000..985e9ac75de
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-1.c
@@ -0,0 +1,26 @@
+int open(const char *, int mode);
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDONLY); /* { dg-message "\\(1\\) opened here" }
*/
+ return; /* { dg-warning "leak of file descriptor 'fd' \\\[CWE-775\\\]"
"warning" } */
+ /* { dg-message "\\(2\\) 'fd' leaks here; was opened at \\(1\\)" "event"
{ target *-*-* } .-1 } */
+}
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" } */
+ if (fd >= 0) /* { dg-message "\\(2\\) assuming 'fd' is a valid file
descriptor" "event1" } */
+ /* { dg-message "\\(3\\) following 'true' branch \\(when 'fd >=
0'\\)..." "event2" { target *-*-* } .-1 } */
+ {
+ return; /* { dg-warning "leak of file descriptor 'fd' \\\[CWE-775\\\]"
"warning" } */
+ /* { dg-message "\\(4\\) ...to here" "event1" { target *-*-* } .-1 } */
+ /* { dg-message "\\(5\\) 'fd' leaks here; was opened at \\(1\\)"
"event2" { target *-*-* } .-2 } */
+ }
+}
+
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-2.c
b/gcc/testsuite/gcc.dg/analyzer/fd-2.c
new file mode 100644
index 00000000000..2b5a684847f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-2.c
@@ -0,0 +1,45 @@
+int open(const char *, int mode);
+void close(int fd);
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+#define STDIN 0
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ close (fd); /* { dg-message "\\(2\\) first 'close' here" "event1" } */
+ close (fd); /* { dg-warning "double 'close' of file descriptor 'fd'
\\\[CWE-1341\\\]" "warning" } */
+ /* { dg-message "\\(3\\) second 'close' here; first 'close' was at
\\(2\\)" "event2" { target *-*-* } .-1 } */
+}
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ if (fd < 0) /* { dg-message "\\(2\\) assuming 'fd' is a valid file
descriptor \\(>= 0\\)" "event1" } */
+ /* { dg-message "\\(3\\) following 'false' branch \\(when 'fd >=
0'\\)..." "event2" { target *-*-* } .-1 } */
+ return;
+ close (fd); /* { dg-message "\\(4\\) ...to here" "event1" } */
+ /* { dg-message "\\(5\\) first 'close' here" "event2" { target *-*-* }
.-1 } */
+ close (fd); /* { dg-warning "double 'close' of file descriptor 'fd'
\\\[CWE-1341\\\]" "warning" } */
+ /* {dg-message "\\(6\\) second 'close' here; first was at \\(5\\)" ""
{ target *-*-* } .-1 } */
+}
+
+void
+test_3 ()
+{
+ int fd = STDIN;
+ close(fd); /* { dg-message "\\(1\\) first 'close' here" } */
+ close(fd); /* { dg-warning "double 'close' of file descriptor 'fd'
\\\[CWE-1341\\\]" "warning" } */
+ /* { dg-message "\\(2\\) second 'close' here; first 'close' was at
\\(1\\)" "event2" { target *-*-* } .-1 } */
+}
+
+void
+test_4 ()
+{
+ int fd = -1;
+ close(fd);
+ close(fd);
+}
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-3.c
b/gcc/testsuite/gcc.dg/analyzer/fd-3.c
new file mode 100644
index 00000000000..d8c2ff26098
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-3.c
@@ -0,0 +1,43 @@
+int open(const char *, int mode);
+void close(int fd);
+int write (int fd, void *buf, int nbytes);
+int read (int fd, void *buf, int nbytes);
+
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+#define STDIN 0
+
+void
+test_1 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ write (fd, buf, 1); /* { dg-message "\\(2\\) 'fd' could be invalid:
unchecked value from \\(1\\)" } */
+ /* { dg-warning "'write' on possibly invalid file descriptor 'fd'"
"warning" { target *-*-* } .-1 } */
+ close(fd);
+}
+
+void
+test_2 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ read (fd, buf, 1); /* { dg-message "\\(2\\) 'fd' could be invalid:
unchecked value from \\(1\\)" } */
+ /* { dg-warning "'read' on possibly invalid file descriptor 'fd'"
"warning" { target *-*-* } .-1 } */
+ close (fd);
+}
+
+void
+test_3 (void *buf)
+{
+ int fd = -1;
+ read (fd, buf, 1); /* { dg-warning "'read' on possibly invalid file
descriptor 'fd'" } */
+ /* { dg-message "\\(1\\) 'fd' could be invalid" "warning" { target
*-*-* } .-1 } */
+}
+
+void
+test_4 (void *buf)
+{
+ int fd = STDIN;
+ write (fd, buf, 1);
+ close(fd);
+}
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-4.c
b/gcc/testsuite/gcc.dg/analyzer/fd-4.c
new file mode 100644
index 00000000000..56828765bdd
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-4.c
@@ -0,0 +1,60 @@
+int open(const char *, int mode);
+void close(int fd);
+int write (int fd, void *buf, int nbytes);
+int read (int fd, void *buf, int nbytes);
+
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+
+
+void
+test_1 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDONLY); /* { dg-message "opened here as
read-only" } */
+ if (fd >= 0) /* { dg-message "assuming 'fd' is a valid file descriptor
\\(>= 0\\)" "event1" } */
+ /* { dg-message "following 'true' branch \\(when 'fd >= 0'\\)..."
"event2" { target *-*-* } .-1 } */
+ {
+ write (fd, buf, 1); /* { dg-warning "'write' on 'read-only' file
descriptor 'fd'" "warning" } */
+ /* { dg-message "\\(4\\) ...to here" "event1" { target *-*-* } .-1
} */
+ /* { dg-message "\\(5\\) 'write' on 'read-only' file descriptor
'fd'" "event2" { target *-*-* } .-2 } */
+ close (fd);
+ }
+}
+
+void
+test_2 (const char *path, void *buf)
+{
+ int fd = open (path, O_WRONLY); /* { dg-message "opened here as
write-only" } */
+ if (fd >= 0) /* { dg-message "assuming 'fd' is a valid file descriptor
\\(>= 0\\)" "event1" } */
+ /* { dg-message "following 'true' branch \\(when 'fd >= 0'\\)..."
"event2" { target *-*-* } .-1 } */
+ {
+ read (fd, buf, 1); /* { dg-warning "'read' on 'write-only' file
descriptor 'fd'" "warning" } */
+ /* { dg-message "\\(4\\) ...to here" "event1" { target *-*-* } .-1
} */
+ /* { dg-message "\\(5\\) 'read' on 'write-only' file descriptor
'fd'" "event2" { target *-*-* } .-2 } */
+ close (fd);
+ }
+}
+
+
+void test_4 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ if (fd >= 0)
+ {
+ close(fd); /* {dg-message "\\(2\\) closed here"} */
+ read(fd, buf, 1); /* { dg-warning "'read' on closed file
descriptor 'fd'" } */
+ /* {dg-message "\\(3\\) 'read' on closed file descriptor 'fd'
here" "" {target *-*-*} .-1 } */
+ }
+}
+
+void test_5 (const char *path, void *buf)
+{
+ int fd = open (path, O_RDWR); /* { dg-message "\\(1\\) opened here" }
*/
+ if (fd >= 0)
+ {
+ close(fd); /* {dg-message "\\(2\\) closed here"} */
+ write(fd, buf, 1); /* { dg-warning "'write' on closed file
descriptor 'fd'" } */
+ /* {dg-message "\\(3\\) 'write' on closed file descriptor 'fd'
here" "" {target *-*-*} .-1 } */
+ }
+}
\ No newline at end of file
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-07-14 15:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-23 18:30 [PATCH] analyzer PR 106003 Mir Immad
2022-06-23 23:20 ` David Malcolm
2022-07-14 15:40 ` David Malcolm
2022-06-25 16:00 Mir Immad
2022-06-26 15:53 ` David Malcolm
2022-06-27 17:07 ` Mir Immad
2022-06-27 17:08 Mir Immad
2022-06-27 20:55 ` David Malcolm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).