* [PATCH] Adding three new function attributes for static analysis of file descriptors
@ 2022-07-22 15:55 Immad Mir
2022-07-22 18:27 ` David Malcolm
0 siblings, 1 reply; 11+ messages in thread
From: Immad Mir @ 2022-07-22 15:55 UTC (permalink / raw)
To: gcc-patches; +Cc: dmalcolm, mirimnan017, Immad Mir
This patch adds three new function attributes to GCC that
are used for static analysis of usage of file descriptors:
1) __attribute__ ((fd_arg(N))): The attributes may be applied to a function that
takes an open file descriptor at refrenced argument N.
It indicates that the passed filedescriptor must not have been closed.
Therefore, when the analyzer is enabled with -fanalyzer, the
analyzer may emit a -Wanalyzer-fd-use-after-close diagnostic
if it detects a code path in which a function with this attribute is
called with a closed file descriptor.
The attribute also indicates that the file descriptor must have been checked for
validity before usage. Therefore, analyzer may emit
-Wanalyzer-fd-use-without-check diagnostic if it detects a code path in
which a function with this attribute is called with a file descriptor that has
not been checked for validity.
2) __attribute__((fd_arg_read(N))): The attribute is identical to
fd_arg, but with the additional requirement that it might read from
the file descriptor, and thus, the file descriptor must not have been opened
as write-only.
The analyzer may emit a -Wanalyzer-access-mode-mismatch
diagnostic if it detects a code path in which a function with this
attribute is called on a file descriptor opened with O_WRONLY.
3) __attribute__((fd_arg_write(N))): The attribute is identical to fd_arg_read
except that the analyzer may emit a -Wanalyzer-access-mode-mismatch diagnostic if
it detects a code path in which a function with this attribute is called on a
file descriptor opened with O_RDONLY.
gcc/analyzer/ChangeLog:
* sm-fd.cc (fd_param_diagnostic): New diagnostic class.
(fd_access_mode_mismatch): Change inheritance from fd_diagnostic
to fd_param_diagnostic. Add new overloaded constructor.
(fd_use_after_close): Likewise.
(unchecked_use_of_fd): Likewise and also change name to fd_use_without_check.
(double_close): Change name to fd_double_close.
(enum access_directions): New.
(fd_state_machine::on_stmt): Handle calls to function with the
new three function attributes.
(fd_state_machine::check_for_fd_attrs): New.
(fd_state_machine::on_open): Use the new overloaded constructors
of diagnostic classes.
gcc/c-family/ChangeLog:
* c-attribs.cc: (c_common_attribute_table): add three new attributes
namely: fd_arg, fd_arg_read and fd_arg_write.
(handle_fd_arg_attribute): New.
gcc/ChangeLog:
* doc/extend.texi: Add fd_arg, fd_arg_read and fd_arg_write under
"Common Function Attributes" section.
* doc/invoke.texi: Add docs to -Wanalyzer-fd-access-mode-mismatch,
-Wanalyzer-use-after-close, -Wanalyzer-fd-use-without-check that these
warnings may be emitted through usage of three function attributes used
for static analysis of file descriptors namely fd_arg, fd_arg_read and
fd_arg_write.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/fd-5.c: New test.
* gcc.dg/analyzer/fd-4.c: Remove quotes around 'read-only' and
'write-only'.
* c-c++-common/attr-fd.c: New test.
Signed-off-by: Immad Mir <mirimmad@outlook.com>
---
gcc/analyzer/sm-fd.cc | 338 +++++++++++++++++++++------
gcc/c-family/c-attribs.cc | 31 +++
gcc/doc/extend.texi | 37 +++
gcc/doc/invoke.texi | 18 +-
gcc/testsuite/c-c++-common/attr-fd.c | 18 ++
gcc/testsuite/gcc.dg/analyzer/fd-4.c | 8 +-
gcc/testsuite/gcc.dg/analyzer/fd-5.c | 53 +++++
7 files changed, 429 insertions(+), 74 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/attr-fd.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
index 8e4300b06e2..c3dac48509e 100644
--- a/gcc/analyzer/sm-fd.cc
+++ b/gcc/analyzer/sm-fd.cc
@@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not see
#include "analyzer/analyzer-selftests.h"
#include "tristate.h"
#include "selftest.h"
+#include "stringpool.h"
+#include "attribs.h"
#include "analyzer/call-string.h"
#include "analyzer/program-point.h"
#include "analyzer/store.h"
#include "analyzer/region-model.h"
+#include "bitmap.h"
#if ENABLE_ANALYZER
@@ -59,6 +62,13 @@ enum access_mode
WRITE_ONLY
};
+enum access_directions
+{
+ DIRS_READ_WRITE,
+ DIRS_READ,
+ DIRS_WRITE
+};
+
class fd_state_machine : public state_machine
{
public:
@@ -146,7 +156,7 @@ private:
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;
+ enum access_directions access_fn) const;
void make_valid_transitions_on_condition (sm_context *sm_ctxt,
const supernode *node,
@@ -156,6 +166,10 @@ private:
const supernode *node,
const gimple *stmt,
const svalue *lhs) const;
+ void check_for_fd_attrs (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl, const char *attr_name,
+ access_directions fd_attr_access_dir) const;
};
/* Base diagnostic class relative to fd_state_machine. */
@@ -220,6 +234,70 @@ protected:
tree m_arg;
};
+class fd_param_diagnostic : public fd_diagnostic
+{
+public:
+ fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree callee_fndecl,
+ const char *attr_name, int arg_idx)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
+ m_attr_name (attr_name), m_arg_idx (arg_idx)
+ {
+ }
+
+ fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree callee_fndecl)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
+ m_attr_name (NULL), m_arg_idx (-1)
+ {
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ const fd_param_diagnostic &sub_other
+ = (const fd_param_diagnostic &)base_other;
+ return (same_tree_p (m_arg, sub_other.m_arg)
+ && same_tree_p (m_callee_fndecl, sub_other.m_callee_fndecl)
+ && m_arg_idx == sub_other.m_arg_idx
+ && ((m_attr_name)
+ ? (strcmp (m_attr_name, sub_other.m_attr_name) == 0)
+ : true));
+ }
+
+ void
+ inform_filedescriptor_attribute (access_directions fd_dir)
+ {
+
+ if (m_attr_name)
+ switch (fd_dir)
+ {
+ case DIRS_READ_WRITE:
+ inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
+ "argument %d of %qD must be an open file descriptor, due to "
+ "%<__attribute__((%s(%d)))%>",
+ m_arg_idx + 1, m_callee_fndecl, m_attr_name, m_arg_idx + 1);
+ break;
+ case DIRS_WRITE:
+ inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
+ "argument %d of %qD must be a readable file descriptor, due "
+ "to %<__attribute__((%s(%d)))%>",
+ m_arg_idx + 1, m_callee_fndecl, m_attr_name, m_arg_idx + 1);
+ break;
+ case DIRS_READ:
+ inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
+ "argument %d of %qD must be a writable file descriptor, due "
+ "to %<__attribute__((%s(%d)))%>",
+ m_arg_idx + 1, m_callee_fndecl, m_attr_name, m_arg_idx + 1);
+ break;
+ }
+ }
+
+protected:
+ tree m_callee_fndecl;
+ const char *m_attr_name;
+ /* ARG_IDX is 0-based. */
+ int m_arg_idx;
+};
+
class fd_leak : public fd_diagnostic
{
public:
@@ -290,18 +368,26 @@ private:
diagnostic_event_id_t m_open_event;
};
-class fd_access_mode_mismatch : public fd_diagnostic
+class fd_access_mode_mismatch : public fd_param_diagnostic
{
public:
fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
- enum access_direction fd_dir,
- const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
- m_callee_fndecl (callee_fndecl)
+ enum access_directions fd_dir,
+ const tree callee_fndecl, const char *attr_name,
+ int arg_idx)
+ : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx),
+ m_fd_dir (fd_dir)
{
}
+ fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
+ enum access_directions fd_dir,
+ const tree callee_fndecl)
+ : fd_param_diagnostic (sm, arg, callee_fndecl), m_fd_dir (fd_dir)
+ {
+ }
+
const char *
get_kind () const final override
{
@@ -317,29 +403,25 @@ public:
bool
emit (rich_location *rich_loc) final override
{
+ bool warned;
switch (m_fd_dir)
{
- case DIR_READ:
- return warning_at (rich_loc, get_controlling_option (),
- "%qE on %<read-only%> file descriptor %qE",
+ case DIRS_READ:
+ warned = 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",
+ break;
+ case DIRS_WRITE:
+ warned = warning_at (rich_loc, get_controlling_option (),
+ "%qE on write-only file descriptor %qE",
m_callee_fndecl, m_arg);
+ break;
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_fd_dir == sub_other.m_fd_dir);
+ if (warned)
+ inform_filedescriptor_attribute (m_fd_dir);
+ return warned;
}
label_text
@@ -347,11 +429,11 @@ public:
{
switch (m_fd_dir)
{
- case DIR_READ:
- return ev.formatted_print ("%qE on %<read-only%> file descriptor %qE",
+ case DIRS_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",
+ case DIRS_WRITE:
+ return ev.formatted_print ("%qE on write-only file descriptor %qE",
m_callee_fndecl, m_arg);
default:
gcc_unreachable ();
@@ -359,21 +441,20 @@ public:
}
private:
- enum access_direction m_fd_dir;
- const tree m_callee_fndecl;
+ enum access_directions m_fd_dir;
};
-class double_close : public fd_diagnostic
+class fd_double_close : public fd_diagnostic
{
public:
- double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
+ fd_double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
{
}
const char *
get_kind () const final override
{
- return "double_close";
+ return "fd_double_close";
}
int
@@ -418,12 +499,19 @@ private:
diagnostic_event_id_t m_first_close_event;
};
-class fd_use_after_close : public fd_diagnostic
+class fd_use_after_close : public fd_param_diagnostic
{
public:
+ fd_use_after_close (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl, const char *attr_name,
+ int arg_idx)
+ : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx)
+ {
+ }
+
fd_use_after_close (const fd_state_machine &sm, tree arg,
const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ : fd_param_diagnostic (sm, arg, callee_fndecl)
{
}
@@ -442,9 +530,13 @@ public:
bool
emit (rich_location *rich_loc) final override
{
- return warning_at (rich_loc, get_controlling_option (),
+ bool warned;
+ warned = warning_at (rich_loc, get_controlling_option (),
"%qE on closed file descriptor %qE", m_callee_fndecl,
m_arg);
+ if (warned)
+ inform_filedescriptor_attribute (DIRS_READ_WRITE);
+ return warned;
}
label_text
@@ -466,32 +558,38 @@ public:
describe_final_event (const evdesc::final_event &ev) final override
{
if (m_first_close_event.known_p ())
- return ev.formatted_print (
- "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
- m_arg, "close", &m_first_close_event);
- else
- return ev.formatted_print ("%qE on closed file descriptor %qE",
- m_callee_fndecl, m_arg);
+ return ev.formatted_print (
+ "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
+ m_arg, "close", &m_first_close_event);
+ else
+ return ev.formatted_print ("%qE on closed file descriptor %qE",
+ m_callee_fndecl, m_arg);
}
private:
diagnostic_event_id_t m_first_close_event;
- const tree m_callee_fndecl;
};
-class unchecked_use_of_fd : public fd_diagnostic
+class fd_use_without_check : public fd_param_diagnostic
{
public:
- unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
- const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ fd_use_without_check (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl, const char *attr_name,
+ int arg_idx)
+ : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx)
+ {
+ }
+
+ fd_use_without_check (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl)
+ : fd_param_diagnostic (sm, arg, callee_fndecl)
{
}
const char *
get_kind () const final override
{
- return "unchecked_use_of_fd";
+ return "fd_use_without_check";
}
int
@@ -503,18 +601,13 @@ public:
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);
+ bool warned;
+ warned = warning_at (rich_loc, get_controlling_option (),
+ "%qE on possibly invalid file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ if (warned)
+ inform_filedescriptor_attribute (DIRS_READ_WRITE);
+ return warned;
}
label_text
@@ -541,8 +634,7 @@ public:
}
private:
- diagnostic_event_id_t m_first_open_event;
- const tree m_callee_fndecl;
+ diagnostic_event_id_t m_first_open_event;
};
fd_state_machine::fd_state_machine (logger *logger)
@@ -647,11 +739,117 @@ fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
on_read (sm_ctxt, node, stmt, call, callee_fndecl);
return true;
} // "read"
+
+
+ {
+ // Handle __attribute__((fd_arg))
+
+ check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl,
+ "fd_arg", DIRS_READ_WRITE);
+
+ // Handle __attribute__((fd_arg_read))
+
+ check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl,
+ "fd_arg_read", DIRS_READ);
+
+ // Handle __attribute__((fd_arg_write))
+
+ check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl,
+ "fd_arg_write", DIRS_WRITE);
+ }
}
return false;
}
+void
+fd_state_machine::check_for_fd_attrs (
+ sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
+ const gcall *call, const tree callee_fndecl, const char *attr_name,
+ access_directions fd_attr_access_dir) const
+{
+
+ tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
+ attrs = lookup_attribute (attr_name, attrs);
+ if (!attrs)
+ return;
+
+ if (!TREE_VALUE (attrs))
+ return;
+
+ auto_bitmap argmap;
+
+ for (tree idx = TREE_VALUE (attrs); idx; idx = TREE_CHAIN (idx))
+ {
+ unsigned int val = TREE_INT_CST_LOW (TREE_VALUE (idx)) - 1;
+ bitmap_set_bit (argmap, val);
+ }
+ if (bitmap_empty_p (argmap))
+ return;
+
+ for (unsigned arg_idx = 0; arg_idx < gimple_call_num_args (call); arg_idx++)
+ {
+ tree arg = gimple_call_arg (call, arg_idx);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ bool bit_set = bitmap_bit_p (argmap, arg_idx);
+ if (TREE_CODE (TREE_TYPE (arg)) != INTEGER_TYPE)
+ continue;
+ if (bit_set) // Check if arg_idx is marked by any of the file descriptor
+ // attributes
+ {
+
+ if (is_closed_fd_p (state))
+ {
+
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_after_close (*this, diag_arg,
+ callee_fndecl, attr_name,
+ arg_idx));
+ continue;
+ }
+
+ if (!(is_valid_fd_p (state) || (state == m_stop)))
+ {
+ if (!is_constant_fd_p (state))
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_without_check (*this, diag_arg,
+ callee_fndecl, attr_name,
+ arg_idx));
+ }
+
+ switch (fd_attr_access_dir)
+ {
+ case DIRS_READ_WRITE:
+ break;
+ case DIRS_READ:
+
+ if (is_writeonly_fd_p (state))
+ {
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg, DIRS_WRITE,
+ callee_fndecl, attr_name, arg_idx));
+ }
+
+ break;
+ case DIRS_WRITE:
+
+ if (is_readonly_fd_p (state))
+ {
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg, DIRS_READ,
+ callee_fndecl, attr_name, arg_idx));
+ }
+
+ break;
+ }
+ }
+ }
+}
+
+
void
fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
const gimple *stmt, const gcall *call) const
@@ -706,7 +904,7 @@ fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
if (is_closed_fd_p (state))
{
- sm_ctxt->warn (node, stmt, arg, new double_close (*this, diag_arg));
+ sm_ctxt->warn (node, stmt, arg, new fd_double_close (*this, diag_arg));
sm_ctxt->set_next_state (stmt, arg, m_stop);
}
}
@@ -715,21 +913,21 @@ 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);
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIRS_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);
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIRS_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 callee_fndecl_dir) const
+ enum access_directions callee_fndecl_dir) const
{
tree arg = gimple_call_arg (call, 0);
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
@@ -748,30 +946,32 @@ fd_state_machine::check_for_open_fd (
if (!is_constant_fd_p (state))
sm_ctxt->warn (
node, stmt, arg,
- new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
+ new fd_use_without_check (*this, diag_arg, callee_fndecl));
}
switch (callee_fndecl_dir)
{
- case DIR_READ:
+ case DIRS_READ:
if (is_writeonly_fd_p (state))
{
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
sm_ctxt->warn (node, stmt, arg,
new fd_access_mode_mismatch (
- *this, diag_arg, DIR_WRITE, callee_fndecl));
+ *this, diag_arg, DIRS_WRITE, callee_fndecl));
}
break;
- case DIR_WRITE:
+ case DIRS_WRITE:
if (is_readonly_fd_p (state))
{
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
sm_ctxt->warn (node, stmt, arg,
new fd_access_mode_mismatch (
- *this, diag_arg, DIR_READ, callee_fndecl));
+ *this, diag_arg, DIRS_READ, callee_fndecl));
}
break;
+ default:
+ gcc_unreachable ();
}
}
}
diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index c8d96723f4c..e4f1d3542f3 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -173,6 +173,7 @@ static tree handle_objc_nullability_attribute (tree *, tree, tree, int, bool *);
static tree handle_signed_bool_precision_attribute (tree *, tree, tree, int,
bool *);
static tree handle_retain_attribute (tree *, tree, tree, int, bool *);
+static tree handle_fd_arg_attribute (tree *, tree, tree, int, bool *);
/* Helper to define attribute exclusions. */
#define ATTR_EXCL(name, function, type, variable) \
@@ -555,6 +556,12 @@ const struct attribute_spec c_common_attribute_table[] =
handle_dealloc_attribute, NULL },
{ "tainted_args", 0, 0, true, false, false, false,
handle_tainted_args_attribute, NULL },
+ { "fd_arg", 1, 1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
+ { "fd_arg_read", 1, 1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
+ { "fd_arg_write", 1, 1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
{ NULL, 0, 0, false, false, false, false, NULL, NULL }
};
@@ -4521,6 +4528,30 @@ handle_nonnull_attribute (tree *node, tree name,
return NULL_TREE;
}
+/* Handle the "fd_arg", "fd_arg_read" and "fd_arg_write" attributes */
+
+static tree
+handle_fd_arg_attribute (tree *node, tree name, tree args,
+ int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+ tree type = *node;
+ if (!args)
+ {
+ if (!prototype_p (type))
+ {
+ error ("%qE attribute without arguments on a non-prototype", name);
+ *no_add_attrs = true;
+ }
+ return NULL_TREE;
+ }
+
+ if (positional_argument (*node, name, TREE_VALUE (args), INTEGER_TYPE))
+ return NULL_TREE;
+
+ *no_add_attrs = true;
+ return NULL_TREE;
+}
+
/* Handle the "nonstring" variable attribute. */
static tree
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index dfbe33ac652..1d08324a70c 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3007,6 +3007,43 @@ produced by @command{gold}.
For other linkers that cannot generate resolution file,
explicit @code{externally_visible} attributes are still necessary.
+@item fd_arg
+@itemx fd_arg (@var{N})
+@cindex @code{fd_arg} function attribute
+The @code{fd_arg} attribute may be applied to a function that takes an open
+file descriptor at referenced argument @var{N}.
+
+It indicates that the passed filedescriptor must not have been closed.
+Therefore, when the analyzer is enabled with @option{-fanalyzer}, the
+analyzer may emit a @option{-Wanalyzer-fd-use-after-close} diagnostic
+if it detects a code path in which a function with this attribute is
+called with a closed file descriptor.
+
+The attribute also indicates that the file descriptor must have been checked for
+validity before usage. Therefore, analyzer may emit
+@option{-Wanalyzer-fd-use-without-check} diagnostic if it detects a code path in
+which a function with this attribute is called with a file descriptor that has
+not been checked for validity.
+
+@item fd_arg_read
+@itemx fd_arg_read (@var{N})
+@cindex @code{fd_arg_read} function attribute
+The @code{fd_arg_read} is identical to @code{fd_arg}, but with the additional
+requirement that it might read from the file descriptor, and thus, the file
+descriptor must not have been opened as write-only.
+
+The analyzer may emit a @option{-Wanalyzer-access-mode-mismatch}
+diagnostic if it detects a code path in which a function with this
+attribute is called on a file descriptor opened with @code{O_WRONLY}.
+
+@item fd_arg_write
+@itemx fd_arg_write (@var{N})
+@cindex @code{fd_arg_write} function attribute
+The @code{fd_arg_write} is identical to @code{fd_arg_read} except that the
+analyzer may emit a @option{-Wanalyzer-access-mode-mismatch} diagnostic if
+it detects a code path in which a function with this attribute is called on a
+file descriptor opened with @code{O_RDONLY}.
+
@item flatten
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function marked with
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index d5ff1018372..c5b97e65197 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -9843,7 +9843,13 @@ This warning requires @option{-fanalyzer}, which enables it; use
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
+@code{read} on a write-only file descriptor is attempted, or vice versa.
+
+This diagnostic also warns for code paths in a which a function with attribute
+@code{fd_arg_read (N)} is called with a file descriptor opened with
+@code{O_WRONLY} at referenced argument @code{N} or a function with attribute
+@code{fd_arg_write (N)} is called with a file descriptor opened with
+@code{O_RDONLY} at referenced argument @var{N}.
@item -Wno-analyzer-fd-double-close
@opindex Wanalyzer-fd-double-close
@@ -9875,6 +9881,11 @@ to disable it.
This diagnostic warns for paths through code in which a
read or write is called on a closed file descriptor.
+This diagnostic also warns for paths through code in which
+a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
+or @code{fd_arg_write (N)} is called with a closed file descriptor at
+referenced argument @code{N}.
+
@item -Wno-analyzer-fd-use-without-check
@opindex Wanalyzer-fd-use-without-check
@opindex Wno-analyzer-fd-use-without-check
@@ -9885,6 +9896,11 @@ to disable it.
This diagnostic warns for paths through code in which a
file descriptor is used without being checked for validity.
+This diagnostic also warns for paths through code in which
+a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
+or @code{fd_arg_write (N)} is called with a file descriptor, at referenced
+argument @code{N}, without being checked for validity.
+
@item -Wno-analyzer-file-leak
@opindex Wanalyzer-file-leak
@opindex Wno-analyzer-file-leak
diff --git a/gcc/testsuite/c-c++-common/attr-fd.c b/gcc/testsuite/c-c++-common/attr-fd.c
new file mode 100644
index 00000000000..e4bb4ed0374
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/attr-fd.c
@@ -0,0 +1,18 @@
+
+int not_a_fn __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute only applies to function types" } */
+
+void f (char *p) __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute argument value '1' refers to parameter type 'char ?\\\*'" } */
+
+
+int not_a_fn_b __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute only applies to function types" } */
+
+void g (char *p) __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute argument value '1' refers to parameter type 'char ?\\\*'" } */
+
+
+int not_a_fn_c __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute only applies to function types" } */
+
+void f (char *p) __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute argument value '1' refers to parameter type 'char ?\\\*'" } */
+
+
+void fn_a (int fd) __attribute__ ((fd_arg(0))); /* { dg-warning "'fd_arg' attribute argument value '0' does not refer to a function parameter" } */
+void fd_a_1 (int fd) __attribute__ ((fd_arg("notint"))); /* { dg-warning "'fd_arg' attribute argument has type ('char\\\[7\\\]'|'const char\\\*')" } */
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-4.c b/gcc/testsuite/gcc.dg/analyzer/fd-4.c
index fcfa6168efa..41263468a6c 100644
--- a/gcc/testsuite/gcc.dg/analyzer/fd-4.c
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-4.c
@@ -17,9 +17,9 @@ test_1 (const char *path, void *buf)
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" } */
+ 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 } */
+ /* { dg-message "\\(5\\) 'write' on read-only file descriptor 'fd'" "event2" { target *-*-* } .-2 } */
close (fd);
}
}
@@ -31,9 +31,9 @@ test_2 (const char *path, void *buf)
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" } */
+ 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 } */
+ /* { dg-message "\\(5\\) 'read' on write-only file descriptor 'fd'" "event2" { target *-*-* } .-2 } */
close (fd);
}
}
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-5.c b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
new file mode 100644
index 00000000000..8f29c11b3e8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
@@ -0,0 +1,53 @@
+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 f (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'f' must be an open file descriptor, due to '__attribute__\\(\\(fd_arg\\(1\\)\\)\\)'" } */
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDWR);
+ close(fd);
+ f(fd); /* { dg-warning "'f' on closed file descriptor 'fd'" } */
+ /* { dg-message "\\(3\\) 'f' on closed file descriptor 'fd'; 'close' was at \\(2\\)" "" { target *-*-* } .-1 } */
+}
+
+void g (int fd) __attribute__((fd_arg_read(1))); /* { dg-message "argument 1 of 'g' must be a readable file descriptor, due to '__attribute__\\(\\(fd_arg_read\\(1\\)\\)\\)'" } */
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_WRONLY);
+ if (fd != -1)
+ {
+ g (fd); /* { dg-warning "'g' on write-only file descriptor 'fd'" } */
+ }
+ close (fd);
+}
+
+void h (int fd) __attribute__((fd_arg_write(1))); /* { dg-message "argument 1 of 'h' must be a writable file descriptor, due to '__attribute__\\(\\(fd_arg_write\\(1\\)\\)\\)'" } */
+void
+test_3 (const char *path)
+{
+ int fd = open (path, O_RDONLY);
+ if (fd != -1)
+ {
+ h (fd); /* { dg-warning "'h' on read-only file descriptor 'fd'" } */
+ }
+ close(fd);
+}
+
+void ff (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'ff' must be an open file descriptor, due to '__attribute__\\(\\(fd_arg\\(1\\)\\)\\)'" } */
+
+void test_4 (const char *path)
+{
+ int fd = open (path, O_RDWR);
+ ff (fd); /* { dg-warning "'ff' on possibly invalid file descriptor 'fd'" } */
+ close(fd);
+}
\ No newline at end of file
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Adding three new function attributes for static analysis of file descriptors
2022-07-22 15:55 [PATCH] Adding three new function attributes for static analysis of file descriptors Immad Mir
@ 2022-07-22 18:27 ` David Malcolm
0 siblings, 0 replies; 11+ messages in thread
From: David Malcolm @ 2022-07-22 18:27 UTC (permalink / raw)
To: mirimnan017, gcc-patches; +Cc: Immad Mir
On Fri, 2022-07-22 at 21:25 +0530, Immad Mir wrote:
> This patch adds three new function attributes to GCC that
> are used for static analysis of usage of file descriptors:
>
> 1) __attribute__ ((fd_arg(N))): The attributes may be applied to a
> function that
> takes an open file descriptor at refrenced argument N.
>
> It indicates that the passed filedescriptor must not have been
> closed.
> Therefore, when the analyzer is enabled with -fanalyzer, the
> analyzer may emit a -Wanalyzer-fd-use-after-close diagnostic
> if it detects a code path in which a function with this attribute is
> called with a closed file descriptor.
>
> The attribute also indicates that the file descriptor must have been
> checked for
> validity before usage. Therefore, analyzer may emit
> -Wanalyzer-fd-use-without-check diagnostic if it detects a code path
> in
> which a function with this attribute is called with a file descriptor
> that has
> not been checked for validity.
>
> 2) __attribute__((fd_arg_read(N))): The attribute is identical to
> fd_arg, but with the additional requirement that it might read from
> the file descriptor, and thus, the file descriptor must not have been
> opened
> as write-only.
>
> The analyzer may emit a -Wanalyzer-access-mode-mismatch
> diagnostic if it detects a code path in which a function with this
> attribute is called on a file descriptor opened with O_WRONLY.
>
> 3) __attribute__((fd_arg_write(N))): The attribute is identical to
> fd_arg_read
> except that the analyzer may emit a -Wanalyzer-access-mode-mismatch
> diagnostic if
> it detects a code path in which a function with this attribute is
> called on a
> file descriptor opened with O_RDONLY.
[...snip...]
Thanks for the updated patch.
This version looks good for trunk. You indicated (in an off-list
email) that you've tested this, so please go ahead and push this.
Thanks
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Adding three new function attributes for static analysis of file descriptors
@ 2022-07-20 17:59 Immad Mir
2022-07-20 18:23 ` David Malcolm
2022-07-20 18:28 ` Prathamesh Kulkarni
0 siblings, 2 replies; 11+ messages in thread
From: Immad Mir @ 2022-07-20 17:59 UTC (permalink / raw)
To: gcc-patches; +Cc: dmalcolm, mirimnan017, Immad Mir
This patch adds three new function attributes to GCC that
are used for static analysis of usage of file descriptors:
1) __attribute__ ((fd_arg(N))): The attributes may be applied to a function that
takes on open file descriptor at refrenced argument N.
It indicates that the passed filedescriptor must not have been closed.
Therefore, when the analyzer is enabled with -fanalyzer, the
analyzer may emit a -Wanalyzer-fd-use-after-close diagnostic
if it detects a code path in which a function with this attribute is
called with a closed file descriptor.
The attribute also indicates that the file descriptor must have been checked for
validity before usage. Therefore, analyzer may emit
-Wanalyzer-fd-use-without-check diagnostic if it detects a code path in
which a function with this attribute is called with a file descriptor that has
not been checked for validity.
2) __attribute__((fd_arg_read(N))): The attribute is identical to
fd_arg, but with the additional requirement that it might read from
the file descriptor, and thus, the file descriptor must not have been opened
as write-only.
The analyzer may emit a -Wanalyzer-access-mode-mismatch
diagnostic if it detects a code path in which a function with this
attribute is called on a file descriptor opened with O_WRONLY.
3) __attribute__((fd_arg_write(N))): The attribute is identical to fd_arg_read
except that the analyzer may emit a -Wanalyzer-access-mode-mismatch diagnostic if
it detects a code path in which a function with this attribute is called on a
file descriptor opened with O_RDONLY.
gcc/analyzer/ChangeLog:
* sm-fd.cc (fd_param_diagnostic): New diagnostic class.
(fd_access_mode_mismatch): Change inheritance from fd_diagnostic
to fd_param_diagnostic. Add new overloaded constructor.
(fd_use_after_close): Likewise.
(unchecked_use_of_fd): Likewise and also change name to fd_use_without_check.
(double_close): Change name to fd_double_close.
(enum access_directions): New.
(fd_state_machine::on_stmt): Handle calls to function with the
new three function attributes.
(fd_state_machine::check_for_fd_attrs): New.
(fd_state_machine::on_open): Use the new overloaded constructors
of diagnostic classes.
gcc/c-family/ChangeLog:
* c-attribs.cc: (c_common_attribute_table): add three new attributes
namely: fd_arg, fd_arg_read and fd_arg_write.
(handle_fd_arg_attribute): New.
gcc/ChangeLog:
* doc/extend.texi: Add fd_arg, fd_arg_read and fd_arg_write under
"Common Function Attributes" section.
* doc/invoke.texi: Add docs to -Wanalyzer-fd-access-mode-mismatch,
-Wanalyzer-use-after-close, -Wanalyzer-fd-use-without-check that these
warnings may be emitted through usage of three function attributes used
for static analysis of file descriptors namely fd_arg, fd_arg_read and
fd_arg_write.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/fd-5.c: New test.
* c-c++-common/attr-fd.c: New test.
Signed-off-by: Immad Mir <mirimmad@outlook.com>
---
gcc/analyzer/sm-fd.cc | 335 +++++++++++++++++++++------
gcc/c-family/c-attribs.cc | 32 +++
gcc/doc/extend.texi | 37 +++
gcc/doc/invoke.texi | 17 +-
gcc/testsuite/c-c++-common/attr-fd.c | 18 ++
gcc/testsuite/gcc.dg/analyzer/fd-5.c | 53 +++++
6 files changed, 422 insertions(+), 70 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/attr-fd.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
index 8e4300b06e2..bb89c471f7e 100644
--- a/gcc/analyzer/sm-fd.cc
+++ b/gcc/analyzer/sm-fd.cc
@@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not see
#include "analyzer/analyzer-selftests.h"
#include "tristate.h"
#include "selftest.h"
+#include "stringpool.h"
+#include "attribs.h"
#include "analyzer/call-string.h"
#include "analyzer/program-point.h"
#include "analyzer/store.h"
#include "analyzer/region-model.h"
+#include "bitmap.h"
#if ENABLE_ANALYZER
@@ -59,6 +62,13 @@ enum access_mode
WRITE_ONLY
};
+enum access_directions
+{
+ DIRS_READ_WRITE,
+ DIRS_READ,
+ DIRS_WRITE
+};
+
class fd_state_machine : public state_machine
{
public:
@@ -146,7 +156,7 @@ private:
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;
+ enum access_directions access_fn) const;
void make_valid_transitions_on_condition (sm_context *sm_ctxt,
const supernode *node,
@@ -156,6 +166,10 @@ private:
const supernode *node,
const gimple *stmt,
const svalue *lhs) const;
+ void check_for_fd_attrs (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const tree callee_fndecl,
+ const char *attr_name,
+ access_directions fd_attr_access_dir) const;
};
/* Base diagnostic class relative to fd_state_machine. */
@@ -220,6 +234,68 @@ protected:
tree m_arg;
};
+class fd_param_diagnostic : public fd_diagnostic
+{
+public:
+ fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree callee_fndecl,
+ const char *attr_name, int arg_idx)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
+ m_attr_name (attr_name), m_arg_idx (arg_idx)
+ {
+ }
+
+ fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree callee_fndecl)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
+ m_attr_name (NULL), m_arg_idx (-1)
+ {
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ const fd_param_diagnostic &sub_other
+ = (const fd_param_diagnostic &)base_other;
+ return (same_tree_p (m_arg, sub_other.m_arg)
+ && same_tree_p (m_callee_fndecl, sub_other.m_callee_fndecl)
+ && m_arg_idx == sub_other.m_arg_idx
+ && ((m_attr_name) ? (m_attr_name == sub_other.m_attr_name) : true));
+ }
+
+ void
+ inform_filedescriptor_attribute (access_directions fd_dir)
+ {
+
+ if (m_attr_name)
+ switch (fd_dir)
+ {
+ case DIRS_READ_WRITE:
+ inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
+ "argument %d of %qD must be an open file descriptor, due to "
+ "%<__attribute__((%s(%d)))%>",
+ m_arg_idx + 1, m_callee_fndecl, m_attr_name, m_arg_idx + 1);
+ break;
+ case DIRS_WRITE:
+ inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
+ "argument %d of %qD must be a readable file descriptor, due "
+ "to %<__attribute__((%s(%d)))%>",
+ m_arg_idx + 1, m_callee_fndecl, m_attr_name, m_arg_idx + 1);
+ break;
+ case DIRS_READ:
+ inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
+ "argument %d of %qD must be a writable file descriptor, due "
+ "to %<__attribute__((%s(%d)))%>",
+ m_arg_idx + 1, m_callee_fndecl, m_attr_name, m_arg_idx + 1);
+ break;
+ }
+ }
+
+protected:
+ tree m_callee_fndecl;
+ const char *m_attr_name;
+ /* ARG_IDX is 0-based. */
+ int m_arg_idx;
+};
+
class fd_leak : public fd_diagnostic
{
public:
@@ -290,18 +366,26 @@ private:
diagnostic_event_id_t m_open_event;
};
-class fd_access_mode_mismatch : public fd_diagnostic
+class fd_access_mode_mismatch : public fd_param_diagnostic
{
public:
fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
- enum access_direction fd_dir,
- const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
- m_callee_fndecl (callee_fndecl)
+ enum access_directions fd_dir,
+ const tree callee_fndecl, const char *attr_name,
+ int arg_idx)
+ : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx),
+ m_fd_dir (fd_dir)
{
}
+ fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
+ enum access_directions fd_dir,
+ const tree callee_fndecl)
+ : fd_param_diagnostic (sm, arg, callee_fndecl), m_fd_dir (fd_dir)
+ {
+ }
+
const char *
get_kind () const final override
{
@@ -317,29 +401,25 @@ public:
bool
emit (rich_location *rich_loc) final override
{
+ bool warned;
switch (m_fd_dir)
{
- case DIR_READ:
- return warning_at (rich_loc, get_controlling_option (),
- "%qE on %<read-only%> file descriptor %qE",
+ case DIRS_READ:
+ warned = 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",
+ break;
+ case DIRS_WRITE:
+ warned = warning_at (rich_loc, get_controlling_option (),
+ "%qE on write-only file descriptor %qE",
m_callee_fndecl, m_arg);
+ break;
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_fd_dir == sub_other.m_fd_dir);
+ if (warned)
+ inform_filedescriptor_attribute (m_fd_dir);
+ return warned;
}
label_text
@@ -347,11 +427,11 @@ public:
{
switch (m_fd_dir)
{
- case DIR_READ:
- return ev.formatted_print ("%qE on %<read-only%> file descriptor %qE",
+ case DIRS_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",
+ case DIRS_WRITE:
+ return ev.formatted_print ("%qE on write-only file descriptor %qE",
m_callee_fndecl, m_arg);
default:
gcc_unreachable ();
@@ -359,21 +439,20 @@ public:
}
private:
- enum access_direction m_fd_dir;
- const tree m_callee_fndecl;
+ enum access_directions m_fd_dir;
};
-class double_close : public fd_diagnostic
+class fd_double_close : public fd_diagnostic
{
public:
- double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
+ fd_double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
{
}
const char *
get_kind () const final override
{
- return "double_close";
+ return "fd_double_close";
}
int
@@ -418,12 +497,19 @@ private:
diagnostic_event_id_t m_first_close_event;
};
-class fd_use_after_close : public fd_diagnostic
+class fd_use_after_close : public fd_param_diagnostic
{
public:
+ fd_use_after_close (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl, const char *attr_name,
+ int arg_idx)
+ : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx)
+ {
+ }
+
fd_use_after_close (const fd_state_machine &sm, tree arg,
const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ : fd_param_diagnostic (sm, arg, callee_fndecl)
{
}
@@ -442,9 +528,13 @@ public:
bool
emit (rich_location *rich_loc) final override
{
- return warning_at (rich_loc, get_controlling_option (),
+ bool warned;
+ warned = warning_at (rich_loc, get_controlling_option (),
"%qE on closed file descriptor %qE", m_callee_fndecl,
m_arg);
+ if (warned)
+ inform_filedescriptor_attribute (DIRS_READ_WRITE);
+ return warned;
}
label_text
@@ -466,32 +556,38 @@ public:
describe_final_event (const evdesc::final_event &ev) final override
{
if (m_first_close_event.known_p ())
- return ev.formatted_print (
- "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
- m_arg, "close", &m_first_close_event);
- else
- return ev.formatted_print ("%qE on closed file descriptor %qE",
- m_callee_fndecl, m_arg);
+ return ev.formatted_print (
+ "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
+ m_arg, "close", &m_first_close_event);
+ else
+ return ev.formatted_print ("%qE on closed file descriptor %qE",
+ m_callee_fndecl, m_arg);
}
private:
diagnostic_event_id_t m_first_close_event;
- const tree m_callee_fndecl;
};
-class unchecked_use_of_fd : public fd_diagnostic
+class fd_use_without_check : public fd_param_diagnostic
{
public:
- unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
- const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ fd_use_without_check (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl, const char *attr_name,
+ int arg_idx)
+ : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx)
+ {
+ }
+
+ fd_use_without_check (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl)
+ : fd_param_diagnostic (sm, arg, callee_fndecl)
{
}
const char *
get_kind () const final override
{
- return "unchecked_use_of_fd";
+ return "fd_use_without_check";
}
int
@@ -503,18 +599,12 @@ public:
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);
+ bool warned;
+ warned = warning_at (rich_loc, get_controlling_option (),
+ "%qE on possibly invalid file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ if (warned)
+ inform_filedescriptor_attribute (DIRS_READ_WRITE);
}
label_text
@@ -541,8 +631,7 @@ public:
}
private:
- diagnostic_event_id_t m_first_open_event;
- const tree m_callee_fndecl;
+ diagnostic_event_id_t m_first_open_event;
};
fd_state_machine::fd_state_machine (logger *logger)
@@ -647,11 +736,117 @@ fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
on_read (sm_ctxt, node, stmt, call, callee_fndecl);
return true;
} // "read"
+
+
+ {
+ // Handle __attribute__((fd_arg))
+
+ check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl, "fd_arg", DIRS_READ_WRITE);
+
+ // Handle __attribute__((fd_arg_read))
+
+ check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl, "fd_arg_read", DIRS_READ);
+
+ // Handle __attribute__((fd_arg_write))
+
+ check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl, "fd_arg_write", DIRS_WRITE);
+
+ return true;
+ }
+
}
return false;
}
+void
+fd_state_machine::check_for_fd_attrs (
+ sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
+ const tree callee_fndecl, const char *attr_name,
+ access_directions fd_attr_access_dir) const
+{
+
+ tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
+ attrs = lookup_attribute (attr_name, attrs);
+ if (!attrs)
+ return;
+
+ if (!TREE_VALUE (attrs))
+ return;
+
+ auto_bitmap argmap;
+
+ for (tree idx = TREE_VALUE (attrs); idx; idx = TREE_CHAIN (idx))
+ {
+ unsigned int val = TREE_INT_CST_LOW (TREE_VALUE (idx)) - 1;
+ bitmap_set_bit (argmap, val);
+ }
+ if (bitmap_empty_p (argmap))
+ return;
+
+ for (unsigned arg_idx = 0; arg_idx < gimple_call_num_args (stmt); arg_idx++)
+ {
+ tree arg = gimple_call_arg (stmt, arg_idx);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ bool bit_set = bitmap_bit_p (argmap, arg_idx);
+ if (TREE_CODE (TREE_TYPE (arg)) != INTEGER_TYPE)
+ continue;
+ if (bit_set) // Check if arg_idx is marked by any of the file descriptor
+ // attributes
+ {
+
+ if (is_closed_fd_p (state))
+ {
+
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_after_close (*this, diag_arg,
+ callee_fndecl, attr_name,
+ arg_idx));
+ continue;
+ }
+
+ if (!(is_valid_fd_p (state) || (state == m_stop)))
+ {
+ if (!is_constant_fd_p (state))
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_without_check (*this, diag_arg,
+ callee_fndecl, attr_name,
+ arg_idx));
+ }
+
+ switch (fd_attr_access_dir)
+ {
+ case DIRS_READ_WRITE:
+ break;
+ case DIRS_READ:
+
+ if (is_writeonly_fd_p (state))
+ {
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg, DIRS_WRITE,
+ callee_fndecl, attr_name, arg_idx));
+ }
+
+ break;
+ case DIRS_WRITE:
+
+ if (is_readonly_fd_p (state))
+ {
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg, DIRS_READ,
+ callee_fndecl, attr_name, arg_idx));
+ }
+
+ break;
+ }
+ }
+ }
+}
+
+
void
fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
const gimple *stmt, const gcall *call) const
@@ -706,7 +901,7 @@ fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
if (is_closed_fd_p (state))
{
- sm_ctxt->warn (node, stmt, arg, new double_close (*this, diag_arg));
+ sm_ctxt->warn (node, stmt, arg, new fd_double_close (*this, diag_arg));
sm_ctxt->set_next_state (stmt, arg, m_stop);
}
}
@@ -715,21 +910,21 @@ 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);
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIRS_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);
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIRS_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 callee_fndecl_dir) const
+ enum access_directions callee_fndecl_dir) const
{
tree arg = gimple_call_arg (call, 0);
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
@@ -748,30 +943,32 @@ fd_state_machine::check_for_open_fd (
if (!is_constant_fd_p (state))
sm_ctxt->warn (
node, stmt, arg,
- new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
+ new fd_use_without_check (*this, diag_arg, callee_fndecl));
}
switch (callee_fndecl_dir)
{
- case DIR_READ:
+ case DIRS_READ:
if (is_writeonly_fd_p (state))
{
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
sm_ctxt->warn (node, stmt, arg,
new fd_access_mode_mismatch (
- *this, diag_arg, DIR_WRITE, callee_fndecl));
+ *this, diag_arg, DIRS_WRITE, callee_fndecl));
}
break;
- case DIR_WRITE:
+ case DIRS_WRITE:
if (is_readonly_fd_p (state))
{
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
sm_ctxt->warn (node, stmt, arg,
new fd_access_mode_mismatch (
- *this, diag_arg, DIR_READ, callee_fndecl));
+ *this, diag_arg, DIRS_READ, callee_fndecl));
}
break;
+ default:
+ gcc_unreachable ();
}
}
}
diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index c8d96723f4c..0ad3a1a7040 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -173,6 +173,7 @@ static tree handle_objc_nullability_attribute (tree *, tree, tree, int, bool *);
static tree handle_signed_bool_precision_attribute (tree *, tree, tree, int,
bool *);
static tree handle_retain_attribute (tree *, tree, tree, int, bool *);
+static tree handle_fd_arg_attribute (tree *, tree, tree, int, bool *);
/* Helper to define attribute exclusions. */
#define ATTR_EXCL(name, function, type, variable) \
@@ -555,6 +556,12 @@ const struct attribute_spec c_common_attribute_table[] =
handle_dealloc_attribute, NULL },
{ "tainted_args", 0, 0, true, false, false, false,
handle_tainted_args_attribute, NULL },
+ { "fd_arg", 1, 1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
+ { "fd_arg_read", 1, 1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
+ { "fd_arg_write", 1, 1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
{ NULL, 0, 0, false, false, false, false, NULL, NULL }
};
@@ -4521,6 +4528,31 @@ handle_nonnull_attribute (tree *node, tree name,
return NULL_TREE;
}
+/* Handle the "fd_arg", "fd_arg_read" and "fd_arg_write" attributes */
+
+static tree
+handle_fd_arg_attribute (tree *node, tree name, tree args,
+ int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+ tree type = *node;
+ if (!args)
+ {
+ if (!prototype_p (type))
+ {
+ error ("%qE attribute without arguments on a non-prototype", name);
+ *no_add_attrs = true;
+ }
+ return NULL_TREE;
+ }
+
+ if (tree val = positional_argument (*node, name, TREE_VALUE (args),
+ INTEGER_TYPE))
+ return NULL_TREE;
+
+ *no_add_attrs = true;
+ return NULL_TREE;
+}
+
/* Handle the "nonstring" variable attribute. */
static tree
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index dfbe33ac652..1d08324a70c 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3007,6 +3007,43 @@ produced by @command{gold}.
For other linkers that cannot generate resolution file,
explicit @code{externally_visible} attributes are still necessary.
+@item fd_arg
+@itemx fd_arg (@var{N})
+@cindex @code{fd_arg} function attribute
+The @code{fd_arg} attribute may be applied to a function that takes an open
+file descriptor at referenced argument @var{N}.
+
+It indicates that the passed filedescriptor must not have been closed.
+Therefore, when the analyzer is enabled with @option{-fanalyzer}, the
+analyzer may emit a @option{-Wanalyzer-fd-use-after-close} diagnostic
+if it detects a code path in which a function with this attribute is
+called with a closed file descriptor.
+
+The attribute also indicates that the file descriptor must have been checked for
+validity before usage. Therefore, analyzer may emit
+@option{-Wanalyzer-fd-use-without-check} diagnostic if it detects a code path in
+which a function with this attribute is called with a file descriptor that has
+not been checked for validity.
+
+@item fd_arg_read
+@itemx fd_arg_read (@var{N})
+@cindex @code{fd_arg_read} function attribute
+The @code{fd_arg_read} is identical to @code{fd_arg}, but with the additional
+requirement that it might read from the file descriptor, and thus, the file
+descriptor must not have been opened as write-only.
+
+The analyzer may emit a @option{-Wanalyzer-access-mode-mismatch}
+diagnostic if it detects a code path in which a function with this
+attribute is called on a file descriptor opened with @code{O_WRONLY}.
+
+@item fd_arg_write
+@itemx fd_arg_write (@var{N})
+@cindex @code{fd_arg_write} function attribute
+The @code{fd_arg_write} is identical to @code{fd_arg_read} except that the
+analyzer may emit a @option{-Wanalyzer-access-mode-mismatch} diagnostic if
+it detects a code path in which a function with this attribute is called on a
+file descriptor opened with @code{O_RDONLY}.
+
@item flatten
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function marked with
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index d5ff1018372..9234275ca6d 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -9843,7 +9843,12 @@ This warning requires @option{-fanalyzer}, which enables it; use
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
+@code{read} on a write-only file descriptor is attempted, or vice versa.
+
+This diagnostic also warns for code paths in a which a function with attribute
+@code{fd_arg_read (N)} is called with a file descriptor opened with @code{O_WRONLY}
+at refrenced argument @code{N} or a function with attribute @code{fd_arg_write (N)}
+is called with a file descriptor opened with @code{O_RDONLY} at refrenced argument @var{N},
@item -Wno-analyzer-fd-double-close
@opindex Wanalyzer-fd-double-close
@@ -9875,6 +9880,11 @@ to disable it.
This diagnostic warns for paths through code in which a
read or write is called on a closed file descriptor.
+This diagnostic also warns for paths through code in which
+a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
+or @code{fd_arg_write (N)} is called with a closed file descriptor at
+refrenced argument @code{N}.
+
@item -Wno-analyzer-fd-use-without-check
@opindex Wanalyzer-fd-use-without-check
@opindex Wno-analyzer-fd-use-without-check
@@ -9885,6 +9895,11 @@ to disable it.
This diagnostic warns for paths through code in which a
file descriptor is used without being checked for validity.
+This diagnostic also warns for paths through code in which
+a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
+or @code{fd_arg_write (N)} is called with a file descriptor, at refrenced
+argument @code{N}, without being checked for validity.
+
@item -Wno-analyzer-file-leak
@opindex Wanalyzer-file-leak
@opindex Wno-analyzer-file-leak
diff --git a/gcc/testsuite/c-c++-common/attr-fd.c b/gcc/testsuite/c-c++-common/attr-fd.c
new file mode 100644
index 00000000000..292c329856e
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/attr-fd.c
@@ -0,0 +1,18 @@
+
+int not_a_fn __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute only applies to function types" } */
+
+void f (char *p) __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute argument value '1' refers to parameter type 'char \\\*'" } */
+
+
+int not_a_fn_b __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute only applies to function types" } */
+
+void g (char *p) __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute argument value '1' refers to parameter type 'char \\\*'" } */
+
+
+int not_a_fn_c __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute only applies to function types" } */
+
+void f (char *p) __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute argument value '1' refers to parameter type 'char \\\*'" } */
+
+
+void fn_a (int fd) __attribute__ ((fd_arg(0))); /* { dg-warning "'fd_arg' attribute argument value '0' does not refer to a function parameter" } */
+void fd_a_1 (int fd) __attribute__ ((fd_arg("notint"))); /* { dg-warning "'fd_arg' attribute argument has type 'char\\\[7\\\]'" } */
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-5.c b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
new file mode 100644
index 00000000000..9805f7f79c5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
@@ -0,0 +1,53 @@
+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 f (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'f' must be an open file descriptor, due to '__attribute__\\(\\(fd_arg\\(1\\)\\)\\)'" } */
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDWR);
+ close(fd);
+ f(fd); /* { dg-warning "'f' on closed file descriptor 'fd'" } */
+ /* { dg-message "\\(3\\) 'f' on closed file descriptor 'fd'; 'close' was at \\(2\\)" "" { target *-*-* } .-1 } */
+}
+
+void g (int fd) __attribute__((fd_arg_read(1))); /* { dg-message "argument 1 of 'g' must be a readable file descriptor, due to '__attribute__\\(\\(fd_arg_read\\(1\\)\\)\\)'" } */
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_WRONLY);
+ if (fd != -1)
+ {
+ g (fd); /* { dg-warning "'g' on 'write-only' file descriptor 'fd'" } */
+ }
+ close (fd);
+}
+
+void h (int fd) __attribute__((fd_arg_write(1))); /* { dg-message "argument 1 of 'h' must be a writable file descriptor, due to '__attribute__\\(\\(fd_arg_write\\(1\\)\\)\\)'" } */
+void
+test_3 (const char *path)
+{
+ int fd = open (path, O_RDONLY);
+ if (fd != -1)
+ {
+ h (fd); /* { dg-warning "'h' on 'read-only' file descriptor 'fd'" } */
+ }
+ close(fd);
+}
+
+void ff (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'ff' must be an open file descriptor, due to '__attribute__\\(\\(fd_arg\\(1\\)\\)\\)'" } */
+
+void test_4 (const char *path)
+{
+ int fd = open (path, O_RDWR);
+ ff (fd); /* { dg-warning "'ff' on possibly invalid file descriptor 'fd'" } */
+ close(fd);
+}
\ No newline at end of file
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Adding three new function attributes for static analysis of file descriptors
2022-07-20 17:59 Immad Mir
@ 2022-07-20 18:23 ` David Malcolm
2022-07-20 18:28 ` Prathamesh Kulkarni
1 sibling, 0 replies; 11+ messages in thread
From: David Malcolm @ 2022-07-20 18:23 UTC (permalink / raw)
To: mirimnan017, gcc-patches; +Cc: Immad Mir
On Wed, 2022-07-20 at 23:29 +0530, Immad Mir wrote:
> This patch adds three new function attributes to GCC that
> are used for static analysis of usage of file descriptors:
Thanks for the updated patch.
Some very minor spelling/grammar/whitespace nits...
>
> 1) __attribute__ ((fd_arg(N))): The attributes may be applied to a function that
> takes on open file descriptor at refrenced argument N.
"on open" -> "an open"
"refrenced" -> "referenced"
[...snip...]
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index d5ff1018372..9234275ca6d 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -9843,7 +9843,12 @@ This warning requires @option{-fanalyzer}, which enables it; use
> 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
> +@code{read} on a write-only file descriptor is attempted, or vice versa.
> +
> +This diagnostic also warns for code paths in a which a function with attribute
> +@code{fd_arg_read (N)} is called with a file descriptor opened with @code{O_WRONLY}
> +at refrenced argument @code{N} or a function with attribute @code{fd_arg_write (N)}
"refrenced" -> "referenced"
> +is called with a file descriptor opened with @code{O_RDONLY} at refrenced argument @var{N},
"refrenced" -> "referenced".
Please wrap these lines to avoid going over 80 columns.
>
> @item -Wno-analyzer-fd-double-close
> @opindex Wanalyzer-fd-double-close
> @@ -9875,6 +9880,11 @@ to disable it.
> This diagnostic warns for paths through code in which a
> read or write is called on a closed file descriptor.
>
> +This diagnostic also warns for paths through code in which
> +a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
> +or @code{fd_arg_write (N)} is called with a closed file descriptor at
> +refrenced argument @code{N}.
"refrenced" -> "referenced".
> +
> @item -Wno-analyzer-fd-use-without-check
> @opindex Wanalyzer-fd-use-without-check
> @opindex Wno-analyzer-fd-use-without-check
> @@ -9885,6 +9895,11 @@ to disable it.
> This diagnostic warns for paths through code in which a
> file descriptor is used without being checked for validity.
>
> +This diagnostic also warns for paths through code in which
> +a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
> +or @code{fd_arg_write (N)} is called with a file descriptor, at refrenced
> +argument @code{N}, without being checked for validity.
"refrenced" -> "referenced".
> +
> @item -Wno-analyzer-file-leak
> @opindex Wanalyzer-file-leak
> @opindex Wno-analyzer-file-leak
[...snip...]
...but with those fixed, this patch is OK for trunk, assuming it
bootstraps and passes regression tests.
Thanks!
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Adding three new function attributes for static analysis of file descriptors
2022-07-20 17:59 Immad Mir
2022-07-20 18:23 ` David Malcolm
@ 2022-07-20 18:28 ` Prathamesh Kulkarni
2022-07-20 18:39 ` Mir Immad
1 sibling, 1 reply; 11+ messages in thread
From: Prathamesh Kulkarni @ 2022-07-20 18:28 UTC (permalink / raw)
To: mirimnan017; +Cc: gcc-patches, Immad Mir
On Wed, 20 Jul 2022 at 23:31, Immad Mir via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This patch adds three new function attributes to GCC that
> are used for static analysis of usage of file descriptors:
>
> 1) __attribute__ ((fd_arg(N))): The attributes may be applied to a function that
> takes on open file descriptor at refrenced argument N.
>
> It indicates that the passed filedescriptor must not have been closed.
> Therefore, when the analyzer is enabled with -fanalyzer, the
> analyzer may emit a -Wanalyzer-fd-use-after-close diagnostic
> if it detects a code path in which a function with this attribute is
> called with a closed file descriptor.
>
> The attribute also indicates that the file descriptor must have been checked for
> validity before usage. Therefore, analyzer may emit
> -Wanalyzer-fd-use-without-check diagnostic if it detects a code path in
> which a function with this attribute is called with a file descriptor that has
> not been checked for validity.
>
> 2) __attribute__((fd_arg_read(N))): The attribute is identical to
> fd_arg, but with the additional requirement that it might read from
> the file descriptor, and thus, the file descriptor must not have been opened
> as write-only.
>
> The analyzer may emit a -Wanalyzer-access-mode-mismatch
> diagnostic if it detects a code path in which a function with this
> attribute is called on a file descriptor opened with O_WRONLY.
>
> 3) __attribute__((fd_arg_write(N))): The attribute is identical to fd_arg_read
> except that the analyzer may emit a -Wanalyzer-access-mode-mismatch diagnostic if
> it detects a code path in which a function with this attribute is called on a
> file descriptor opened with O_RDONLY.
>
> gcc/analyzer/ChangeLog:
> * sm-fd.cc (fd_param_diagnostic): New diagnostic class.
> (fd_access_mode_mismatch): Change inheritance from fd_diagnostic
> to fd_param_diagnostic. Add new overloaded constructor.
> (fd_use_after_close): Likewise.
> (unchecked_use_of_fd): Likewise and also change name to fd_use_without_check.
> (double_close): Change name to fd_double_close.
> (enum access_directions): New.
> (fd_state_machine::on_stmt): Handle calls to function with the
> new three function attributes.
> (fd_state_machine::check_for_fd_attrs): New.
> (fd_state_machine::on_open): Use the new overloaded constructors
> of diagnostic classes.
>
> gcc/c-family/ChangeLog:
> * c-attribs.cc: (c_common_attribute_table): add three new attributes
> namely: fd_arg, fd_arg_read and fd_arg_write.
> (handle_fd_arg_attribute): New.
>
> gcc/ChangeLog:
> * doc/extend.texi: Add fd_arg, fd_arg_read and fd_arg_write under
> "Common Function Attributes" section.
> * doc/invoke.texi: Add docs to -Wanalyzer-fd-access-mode-mismatch,
> -Wanalyzer-use-after-close, -Wanalyzer-fd-use-without-check that these
> warnings may be emitted through usage of three function attributes used
> for static analysis of file descriptors namely fd_arg, fd_arg_read and
> fd_arg_write.
>
> gcc/testsuite/ChangeLog:
> * gcc.dg/analyzer/fd-5.c: New test.
> * c-c++-common/attr-fd.c: New test.
>
> Signed-off-by: Immad Mir <mirimmad@outlook.com>
> ---
> gcc/analyzer/sm-fd.cc | 335 +++++++++++++++++++++------
> gcc/c-family/c-attribs.cc | 32 +++
> gcc/doc/extend.texi | 37 +++
> gcc/doc/invoke.texi | 17 +-
> gcc/testsuite/c-c++-common/attr-fd.c | 18 ++
> gcc/testsuite/gcc.dg/analyzer/fd-5.c | 53 +++++
> 6 files changed, 422 insertions(+), 70 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/attr-fd.c
> create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
>
> diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
> index 8e4300b06e2..bb89c471f7e 100644
> --- a/gcc/analyzer/sm-fd.cc
> +++ b/gcc/analyzer/sm-fd.cc
> @@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not see
> #include "analyzer/analyzer-selftests.h"
> #include "tristate.h"
> #include "selftest.h"
> +#include "stringpool.h"
> +#include "attribs.h"
> #include "analyzer/call-string.h"
> #include "analyzer/program-point.h"
> #include "analyzer/store.h"
> #include "analyzer/region-model.h"
> +#include "bitmap.h"
>
> #if ENABLE_ANALYZER
>
> @@ -59,6 +62,13 @@ enum access_mode
> WRITE_ONLY
> };
>
> +enum access_directions
> +{
> + DIRS_READ_WRITE,
> + DIRS_READ,
> + DIRS_WRITE
> +};
> +
> class fd_state_machine : public state_machine
> {
> public:
> @@ -146,7 +156,7 @@ private:
> 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;
> + enum access_directions access_fn) const;
>
> void make_valid_transitions_on_condition (sm_context *sm_ctxt,
> const supernode *node,
> @@ -156,6 +166,10 @@ private:
> const supernode *node,
> const gimple *stmt,
> const svalue *lhs) const;
> + void check_for_fd_attrs (sm_context *sm_ctxt, const supernode *node,
> + const gimple *stmt, const tree callee_fndecl,
Hi Immad,
Sorry to nitpick -- I assume stmt here refers to a call stmt ?
In that case, I suppose it'd be better to use const gcall *stmt ?
Thanks,
Prathamesh
> + const char *attr_name,
> + access_directions fd_attr_access_dir) const;
> };
>
> /* Base diagnostic class relative to fd_state_machine. */
> @@ -220,6 +234,68 @@ protected:
> tree m_arg;
> };
>
> +class fd_param_diagnostic : public fd_diagnostic
> +{
> +public:
> + fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree callee_fndecl,
> + const char *attr_name, int arg_idx)
> + : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
> + m_attr_name (attr_name), m_arg_idx (arg_idx)
> + {
> + }
> +
> + fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree callee_fndecl)
> + : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
> + m_attr_name (NULL), m_arg_idx (-1)
> + {
> + }
> +
> + bool
> + subclass_equal_p (const pending_diagnostic &base_other) const override
> + {
> + const fd_param_diagnostic &sub_other
> + = (const fd_param_diagnostic &)base_other;
> + return (same_tree_p (m_arg, sub_other.m_arg)
> + && same_tree_p (m_callee_fndecl, sub_other.m_callee_fndecl)
> + && m_arg_idx == sub_other.m_arg_idx
> + && ((m_attr_name) ? (m_attr_name == sub_other.m_attr_name) : true));
> + }
> +
> + void
> + inform_filedescriptor_attribute (access_directions fd_dir)
> + {
> +
> + if (m_attr_name)
> + switch (fd_dir)
> + {
> + case DIRS_READ_WRITE:
> + inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
> + "argument %d of %qD must be an open file descriptor, due to "
> + "%<__attribute__((%s(%d)))%>",
> + m_arg_idx + 1, m_callee_fndecl, m_attr_name, m_arg_idx + 1);
> + break;
> + case DIRS_WRITE:
> + inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
> + "argument %d of %qD must be a readable file descriptor, due "
> + "to %<__attribute__((%s(%d)))%>",
> + m_arg_idx + 1, m_callee_fndecl, m_attr_name, m_arg_idx + 1);
> + break;
> + case DIRS_READ:
> + inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
> + "argument %d of %qD must be a writable file descriptor, due "
> + "to %<__attribute__((%s(%d)))%>",
> + m_arg_idx + 1, m_callee_fndecl, m_attr_name, m_arg_idx + 1);
> + break;
> + }
> + }
> +
> +protected:
> + tree m_callee_fndecl;
> + const char *m_attr_name;
> + /* ARG_IDX is 0-based. */
> + int m_arg_idx;
> +};
> +
> class fd_leak : public fd_diagnostic
> {
> public:
> @@ -290,18 +366,26 @@ private:
> diagnostic_event_id_t m_open_event;
> };
>
> -class fd_access_mode_mismatch : public fd_diagnostic
> +class fd_access_mode_mismatch : public fd_param_diagnostic
> {
> public:
> fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
> - enum access_direction fd_dir,
> - const tree callee_fndecl)
> - : fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
> - m_callee_fndecl (callee_fndecl)
> + enum access_directions fd_dir,
> + const tree callee_fndecl, const char *attr_name,
> + int arg_idx)
> + : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx),
> + m_fd_dir (fd_dir)
>
> {
> }
>
> + fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
> + enum access_directions fd_dir,
> + const tree callee_fndecl)
> + : fd_param_diagnostic (sm, arg, callee_fndecl), m_fd_dir (fd_dir)
> + {
> + }
> +
> const char *
> get_kind () const final override
> {
> @@ -317,29 +401,25 @@ public:
> bool
> emit (rich_location *rich_loc) final override
> {
> + bool warned;
> switch (m_fd_dir)
> {
> - case DIR_READ:
> - return warning_at (rich_loc, get_controlling_option (),
> - "%qE on %<read-only%> file descriptor %qE",
> + case DIRS_READ:
> + warned = 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",
> + break;
> + case DIRS_WRITE:
> + warned = warning_at (rich_loc, get_controlling_option (),
> + "%qE on write-only file descriptor %qE",
> m_callee_fndecl, m_arg);
> + break;
> 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_fd_dir == sub_other.m_fd_dir);
> + if (warned)
> + inform_filedescriptor_attribute (m_fd_dir);
> + return warned;
> }
>
> label_text
> @@ -347,11 +427,11 @@ public:
> {
> switch (m_fd_dir)
> {
> - case DIR_READ:
> - return ev.formatted_print ("%qE on %<read-only%> file descriptor %qE",
> + case DIRS_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",
> + case DIRS_WRITE:
> + return ev.formatted_print ("%qE on write-only file descriptor %qE",
> m_callee_fndecl, m_arg);
> default:
> gcc_unreachable ();
> @@ -359,21 +439,20 @@ public:
> }
>
> private:
> - enum access_direction m_fd_dir;
> - const tree m_callee_fndecl;
> + enum access_directions m_fd_dir;
> };
>
> -class double_close : public fd_diagnostic
> +class fd_double_close : public fd_diagnostic
> {
> public:
> - double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
> + fd_double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
> {
> }
>
> const char *
> get_kind () const final override
> {
> - return "double_close";
> + return "fd_double_close";
> }
>
> int
> @@ -418,12 +497,19 @@ private:
> diagnostic_event_id_t m_first_close_event;
> };
>
> -class fd_use_after_close : public fd_diagnostic
> +class fd_use_after_close : public fd_param_diagnostic
> {
> public:
> + fd_use_after_close (const fd_state_machine &sm, tree arg,
> + const tree callee_fndecl, const char *attr_name,
> + int arg_idx)
> + : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx)
> + {
> + }
> +
> fd_use_after_close (const fd_state_machine &sm, tree arg,
> const tree callee_fndecl)
> - : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
> + : fd_param_diagnostic (sm, arg, callee_fndecl)
> {
> }
>
> @@ -442,9 +528,13 @@ public:
> bool
> emit (rich_location *rich_loc) final override
> {
> - return warning_at (rich_loc, get_controlling_option (),
> + bool warned;
> + warned = warning_at (rich_loc, get_controlling_option (),
> "%qE on closed file descriptor %qE", m_callee_fndecl,
> m_arg);
> + if (warned)
> + inform_filedescriptor_attribute (DIRS_READ_WRITE);
> + return warned;
> }
>
> label_text
> @@ -466,32 +556,38 @@ public:
> describe_final_event (const evdesc::final_event &ev) final override
> {
> if (m_first_close_event.known_p ())
> - return ev.formatted_print (
> - "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
> - m_arg, "close", &m_first_close_event);
> - else
> - return ev.formatted_print ("%qE on closed file descriptor %qE",
> - m_callee_fndecl, m_arg);
> + return ev.formatted_print (
> + "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
> + m_arg, "close", &m_first_close_event);
> + else
> + return ev.formatted_print ("%qE on closed file descriptor %qE",
> + m_callee_fndecl, m_arg);
> }
>
> private:
> diagnostic_event_id_t m_first_close_event;
> - const tree m_callee_fndecl;
> };
>
> -class unchecked_use_of_fd : public fd_diagnostic
> +class fd_use_without_check : public fd_param_diagnostic
> {
> public:
> - unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
> - const tree callee_fndecl)
> - : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
> + fd_use_without_check (const fd_state_machine &sm, tree arg,
> + const tree callee_fndecl, const char *attr_name,
> + int arg_idx)
> + : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx)
> + {
> + }
> +
> + fd_use_without_check (const fd_state_machine &sm, tree arg,
> + const tree callee_fndecl)
> + : fd_param_diagnostic (sm, arg, callee_fndecl)
> {
> }
>
> const char *
> get_kind () const final override
> {
> - return "unchecked_use_of_fd";
> + return "fd_use_without_check";
> }
>
> int
> @@ -503,18 +599,12 @@ public:
> 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);
> + bool warned;
> + warned = warning_at (rich_loc, get_controlling_option (),
> + "%qE on possibly invalid file descriptor %qE",
> + m_callee_fndecl, m_arg);
> + if (warned)
> + inform_filedescriptor_attribute (DIRS_READ_WRITE);
> }
>
> label_text
> @@ -541,8 +631,7 @@ public:
> }
>
> private:
> - diagnostic_event_id_t m_first_open_event;
> - const tree m_callee_fndecl;
> + diagnostic_event_id_t m_first_open_event;
> };
>
> fd_state_machine::fd_state_machine (logger *logger)
> @@ -647,11 +736,117 @@ fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
> on_read (sm_ctxt, node, stmt, call, callee_fndecl);
> return true;
> } // "read"
> +
> +
> + {
> + // Handle __attribute__((fd_arg))
> +
> + check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl, "fd_arg", DIRS_READ_WRITE);
> +
> + // Handle __attribute__((fd_arg_read))
> +
> + check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl, "fd_arg_read", DIRS_READ);
> +
> + // Handle __attribute__((fd_arg_write))
> +
> + check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl, "fd_arg_write", DIRS_WRITE);
> +
> + return true;
> + }
> +
> }
>
> return false;
> }
>
> +void
> +fd_state_machine::check_for_fd_attrs (
> + sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
> + const tree callee_fndecl, const char *attr_name,
> + access_directions fd_attr_access_dir) const
> +{
> +
> + tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
> + attrs = lookup_attribute (attr_name, attrs);
> + if (!attrs)
> + return;
> +
> + if (!TREE_VALUE (attrs))
> + return;
> +
> + auto_bitmap argmap;
> +
> + for (tree idx = TREE_VALUE (attrs); idx; idx = TREE_CHAIN (idx))
> + {
> + unsigned int val = TREE_INT_CST_LOW (TREE_VALUE (idx)) - 1;
> + bitmap_set_bit (argmap, val);
> + }
> + if (bitmap_empty_p (argmap))
> + return;
> +
> + for (unsigned arg_idx = 0; arg_idx < gimple_call_num_args (stmt); arg_idx++)
> + {
> + tree arg = gimple_call_arg (stmt, arg_idx);
> + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> + state_t state = sm_ctxt->get_state (stmt, arg);
> + bool bit_set = bitmap_bit_p (argmap, arg_idx);
> + if (TREE_CODE (TREE_TYPE (arg)) != INTEGER_TYPE)
> + continue;
> + if (bit_set) // Check if arg_idx is marked by any of the file descriptor
> + // attributes
> + {
> +
> + if (is_closed_fd_p (state))
> + {
> +
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_use_after_close (*this, diag_arg,
> + callee_fndecl, attr_name,
> + arg_idx));
> + continue;
> + }
> +
> + if (!(is_valid_fd_p (state) || (state == m_stop)))
> + {
> + if (!is_constant_fd_p (state))
> + sm_ctxt->warn (node, stmt, arg,
> + new fd_use_without_check (*this, diag_arg,
> + callee_fndecl, attr_name,
> + arg_idx));
> + }
> +
> + switch (fd_attr_access_dir)
> + {
> + case DIRS_READ_WRITE:
> + break;
> + case DIRS_READ:
> +
> + if (is_writeonly_fd_p (state))
> + {
> + sm_ctxt->warn (
> + node, stmt, arg,
> + new fd_access_mode_mismatch (*this, diag_arg, DIRS_WRITE,
> + callee_fndecl, attr_name, arg_idx));
> + }
> +
> + break;
> + case DIRS_WRITE:
> +
> + if (is_readonly_fd_p (state))
> + {
> + sm_ctxt->warn (
> + node, stmt, arg,
> + new fd_access_mode_mismatch (*this, diag_arg, DIRS_READ,
> + callee_fndecl, attr_name, arg_idx));
> + }
> +
> + break;
> + }
> + }
> + }
> +}
> +
> +
> void
> fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
> const gimple *stmt, const gcall *call) const
> @@ -706,7 +901,7 @@ fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
>
> if (is_closed_fd_p (state))
> {
> - sm_ctxt->warn (node, stmt, arg, new double_close (*this, diag_arg));
> + sm_ctxt->warn (node, stmt, arg, new fd_double_close (*this, diag_arg));
> sm_ctxt->set_next_state (stmt, arg, m_stop);
> }
> }
> @@ -715,21 +910,21 @@ 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);
> + check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIRS_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);
> + check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIRS_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 callee_fndecl_dir) const
> + enum access_directions callee_fndecl_dir) const
> {
> tree arg = gimple_call_arg (call, 0);
> tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> @@ -748,30 +943,32 @@ fd_state_machine::check_for_open_fd (
> if (!is_constant_fd_p (state))
> sm_ctxt->warn (
> node, stmt, arg,
> - new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
> + new fd_use_without_check (*this, diag_arg, callee_fndecl));
> }
> switch (callee_fndecl_dir)
> {
> - case DIR_READ:
> + case DIRS_READ:
> if (is_writeonly_fd_p (state))
> {
> tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> sm_ctxt->warn (node, stmt, arg,
> new fd_access_mode_mismatch (
> - *this, diag_arg, DIR_WRITE, callee_fndecl));
> + *this, diag_arg, DIRS_WRITE, callee_fndecl));
> }
>
> break;
> - case DIR_WRITE:
> + case DIRS_WRITE:
>
> if (is_readonly_fd_p (state))
> {
> tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> sm_ctxt->warn (node, stmt, arg,
> new fd_access_mode_mismatch (
> - *this, diag_arg, DIR_READ, callee_fndecl));
> + *this, diag_arg, DIRS_READ, callee_fndecl));
> }
> break;
> + default:
> + gcc_unreachable ();
> }
> }
> }
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index c8d96723f4c..0ad3a1a7040 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -173,6 +173,7 @@ static tree handle_objc_nullability_attribute (tree *, tree, tree, int, bool *);
> static tree handle_signed_bool_precision_attribute (tree *, tree, tree, int,
> bool *);
> static tree handle_retain_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_fd_arg_attribute (tree *, tree, tree, int, bool *);
>
> /* Helper to define attribute exclusions. */
> #define ATTR_EXCL(name, function, type, variable) \
> @@ -555,6 +556,12 @@ const struct attribute_spec c_common_attribute_table[] =
> handle_dealloc_attribute, NULL },
> { "tainted_args", 0, 0, true, false, false, false,
> handle_tainted_args_attribute, NULL },
> + { "fd_arg", 1, 1, false, true, true, false,
> + handle_fd_arg_attribute, NULL},
> + { "fd_arg_read", 1, 1, false, true, true, false,
> + handle_fd_arg_attribute, NULL},
> + { "fd_arg_write", 1, 1, false, true, true, false,
> + handle_fd_arg_attribute, NULL},
> { NULL, 0, 0, false, false, false, false, NULL, NULL }
> };
>
> @@ -4521,6 +4528,31 @@ handle_nonnull_attribute (tree *node, tree name,
> return NULL_TREE;
> }
>
> +/* Handle the "fd_arg", "fd_arg_read" and "fd_arg_write" attributes */
> +
> +static tree
> +handle_fd_arg_attribute (tree *node, tree name, tree args,
> + int ARG_UNUSED (flags), bool *no_add_attrs)
> +{
> + tree type = *node;
> + if (!args)
> + {
> + if (!prototype_p (type))
> + {
> + error ("%qE attribute without arguments on a non-prototype", name);
> + *no_add_attrs = true;
> + }
> + return NULL_TREE;
> + }
> +
> + if (tree val = positional_argument (*node, name, TREE_VALUE (args),
> + INTEGER_TYPE))
> + return NULL_TREE;
> +
> + *no_add_attrs = true;
> + return NULL_TREE;
> +}
> +
> /* Handle the "nonstring" variable attribute. */
>
> static tree
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index dfbe33ac652..1d08324a70c 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3007,6 +3007,43 @@ produced by @command{gold}.
> For other linkers that cannot generate resolution file,
> explicit @code{externally_visible} attributes are still necessary.
>
> +@item fd_arg
> +@itemx fd_arg (@var{N})
> +@cindex @code{fd_arg} function attribute
> +The @code{fd_arg} attribute may be applied to a function that takes an open
> +file descriptor at referenced argument @var{N}.
> +
> +It indicates that the passed filedescriptor must not have been closed.
> +Therefore, when the analyzer is enabled with @option{-fanalyzer}, the
> +analyzer may emit a @option{-Wanalyzer-fd-use-after-close} diagnostic
> +if it detects a code path in which a function with this attribute is
> +called with a closed file descriptor.
> +
> +The attribute also indicates that the file descriptor must have been checked for
> +validity before usage. Therefore, analyzer may emit
> +@option{-Wanalyzer-fd-use-without-check} diagnostic if it detects a code path in
> +which a function with this attribute is called with a file descriptor that has
> +not been checked for validity.
> +
> +@item fd_arg_read
> +@itemx fd_arg_read (@var{N})
> +@cindex @code{fd_arg_read} function attribute
> +The @code{fd_arg_read} is identical to @code{fd_arg}, but with the additional
> +requirement that it might read from the file descriptor, and thus, the file
> +descriptor must not have been opened as write-only.
> +
> +The analyzer may emit a @option{-Wanalyzer-access-mode-mismatch}
> +diagnostic if it detects a code path in which a function with this
> +attribute is called on a file descriptor opened with @code{O_WRONLY}.
> +
> +@item fd_arg_write
> +@itemx fd_arg_write (@var{N})
> +@cindex @code{fd_arg_write} function attribute
> +The @code{fd_arg_write} is identical to @code{fd_arg_read} except that the
> +analyzer may emit a @option{-Wanalyzer-access-mode-mismatch} diagnostic if
> +it detects a code path in which a function with this attribute is called on a
> +file descriptor opened with @code{O_RDONLY}.
> +
> @item flatten
> @cindex @code{flatten} function attribute
> Generally, inlining into a function is limited. For a function marked with
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index d5ff1018372..9234275ca6d 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -9843,7 +9843,12 @@ This warning requires @option{-fanalyzer}, which enables it; use
> 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
> +@code{read} on a write-only file descriptor is attempted, or vice versa.
> +
> +This diagnostic also warns for code paths in a which a function with attribute
> +@code{fd_arg_read (N)} is called with a file descriptor opened with @code{O_WRONLY}
> +at refrenced argument @code{N} or a function with attribute @code{fd_arg_write (N)}
> +is called with a file descriptor opened with @code{O_RDONLY} at refrenced argument @var{N},
>
> @item -Wno-analyzer-fd-double-close
> @opindex Wanalyzer-fd-double-close
> @@ -9875,6 +9880,11 @@ to disable it.
> This diagnostic warns for paths through code in which a
> read or write is called on a closed file descriptor.
>
> +This diagnostic also warns for paths through code in which
> +a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
> +or @code{fd_arg_write (N)} is called with a closed file descriptor at
> +refrenced argument @code{N}.
> +
> @item -Wno-analyzer-fd-use-without-check
> @opindex Wanalyzer-fd-use-without-check
> @opindex Wno-analyzer-fd-use-without-check
> @@ -9885,6 +9895,11 @@ to disable it.
> This diagnostic warns for paths through code in which a
> file descriptor is used without being checked for validity.
>
> +This diagnostic also warns for paths through code in which
> +a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
> +or @code{fd_arg_write (N)} is called with a file descriptor, at refrenced
> +argument @code{N}, without being checked for validity.
> +
> @item -Wno-analyzer-file-leak
> @opindex Wanalyzer-file-leak
> @opindex Wno-analyzer-file-leak
> diff --git a/gcc/testsuite/c-c++-common/attr-fd.c b/gcc/testsuite/c-c++-common/attr-fd.c
> new file mode 100644
> index 00000000000..292c329856e
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/attr-fd.c
> @@ -0,0 +1,18 @@
> +
> +int not_a_fn __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute only applies to function types" } */
> +
> +void f (char *p) __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute argument value '1' refers to parameter type 'char \\\*'" } */
> +
> +
> +int not_a_fn_b __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute only applies to function types" } */
> +
> +void g (char *p) __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute argument value '1' refers to parameter type 'char \\\*'" } */
> +
> +
> +int not_a_fn_c __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute only applies to function types" } */
> +
> +void f (char *p) __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute argument value '1' refers to parameter type 'char \\\*'" } */
> +
> +
> +void fn_a (int fd) __attribute__ ((fd_arg(0))); /* { dg-warning "'fd_arg' attribute argument value '0' does not refer to a function parameter" } */
> +void fd_a_1 (int fd) __attribute__ ((fd_arg("notint"))); /* { dg-warning "'fd_arg' attribute argument has type 'char\\\[7\\\]'" } */
> diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-5.c b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> new file mode 100644
> index 00000000000..9805f7f79c5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> @@ -0,0 +1,53 @@
> +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 f (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'f' must be an open file descriptor, due to '__attribute__\\(\\(fd_arg\\(1\\)\\)\\)'" } */
> +
> +void
> +test_1 (const char *path)
> +{
> + int fd = open (path, O_RDWR);
> + close(fd);
> + f(fd); /* { dg-warning "'f' on closed file descriptor 'fd'" } */
> + /* { dg-message "\\(3\\) 'f' on closed file descriptor 'fd'; 'close' was at \\(2\\)" "" { target *-*-* } .-1 } */
> +}
> +
> +void g (int fd) __attribute__((fd_arg_read(1))); /* { dg-message "argument 1 of 'g' must be a readable file descriptor, due to '__attribute__\\(\\(fd_arg_read\\(1\\)\\)\\)'" } */
> +
> +void
> +test_2 (const char *path)
> +{
> + int fd = open (path, O_WRONLY);
> + if (fd != -1)
> + {
> + g (fd); /* { dg-warning "'g' on 'write-only' file descriptor 'fd'" } */
> + }
> + close (fd);
> +}
> +
> +void h (int fd) __attribute__((fd_arg_write(1))); /* { dg-message "argument 1 of 'h' must be a writable file descriptor, due to '__attribute__\\(\\(fd_arg_write\\(1\\)\\)\\)'" } */
> +void
> +test_3 (const char *path)
> +{
> + int fd = open (path, O_RDONLY);
> + if (fd != -1)
> + {
> + h (fd); /* { dg-warning "'h' on 'read-only' file descriptor 'fd'" } */
> + }
> + close(fd);
> +}
> +
> +void ff (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'ff' must be an open file descriptor, due to '__attribute__\\(\\(fd_arg\\(1\\)\\)\\)'" } */
> +
> +void test_4 (const char *path)
> +{
> + int fd = open (path, O_RDWR);
> + ff (fd); /* { dg-warning "'ff' on possibly invalid file descriptor 'fd'" } */
> + close(fd);
> +}
> \ No newline at end of file
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Adding three new function attributes for static analysis of file descriptors
2022-07-20 18:28 ` Prathamesh Kulkarni
@ 2022-07-20 18:39 ` Mir Immad
0 siblings, 0 replies; 11+ messages in thread
From: Mir Immad @ 2022-07-20 18:39 UTC (permalink / raw)
To: Prathamesh Kulkarni; +Cc: gcc-patches
> Sorry to nitpick -- I assume stmt here refers to a call stmt ?
> In that case, I suppose it'd be better to use const gcall *stmt ?
Thanks for the catch, Prathamesh.
Immad.
On Wed, Jul 20, 2022 at 11:59 PM Prathamesh Kulkarni <
prathamesh.kulkarni@linaro.org> wrote:
> On Wed, 20 Jul 2022 at 23:31, Immad Mir via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > This patch adds three new function attributes to GCC that
> > are used for static analysis of usage of file descriptors:
> >
> > 1) __attribute__ ((fd_arg(N))): The attributes may be applied to a
> function that
> > takes on open file descriptor at refrenced argument N.
> >
> > It indicates that the passed filedescriptor must not have been closed.
> > Therefore, when the analyzer is enabled with -fanalyzer, the
> > analyzer may emit a -Wanalyzer-fd-use-after-close diagnostic
> > if it detects a code path in which a function with this attribute is
> > called with a closed file descriptor.
> >
> > The attribute also indicates that the file descriptor must have been
> checked for
> > validity before usage. Therefore, analyzer may emit
> > -Wanalyzer-fd-use-without-check diagnostic if it detects a code path in
> > which a function with this attribute is called with a file descriptor
> that has
> > not been checked for validity.
> >
> > 2) __attribute__((fd_arg_read(N))): The attribute is identical to
> > fd_arg, but with the additional requirement that it might read from
> > the file descriptor, and thus, the file descriptor must not have been
> opened
> > as write-only.
> >
> > The analyzer may emit a -Wanalyzer-access-mode-mismatch
> > diagnostic if it detects a code path in which a function with this
> > attribute is called on a file descriptor opened with O_WRONLY.
> >
> > 3) __attribute__((fd_arg_write(N))): The attribute is identical to
> fd_arg_read
> > except that the analyzer may emit a -Wanalyzer-access-mode-mismatch
> diagnostic if
> > it detects a code path in which a function with this attribute is called
> on a
> > file descriptor opened with O_RDONLY.
> >
> > gcc/analyzer/ChangeLog:
> > * sm-fd.cc (fd_param_diagnostic): New diagnostic class.
> > (fd_access_mode_mismatch): Change inheritance from fd_diagnostic
> > to fd_param_diagnostic. Add new overloaded constructor.
> > (fd_use_after_close): Likewise.
> > (unchecked_use_of_fd): Likewise and also change name to
> fd_use_without_check.
> > (double_close): Change name to fd_double_close.
> > (enum access_directions): New.
> > (fd_state_machine::on_stmt): Handle calls to function with the
> > new three function attributes.
> > (fd_state_machine::check_for_fd_attrs): New.
> > (fd_state_machine::on_open): Use the new overloaded constructors
> > of diagnostic classes.
> >
> > gcc/c-family/ChangeLog:
> > * c-attribs.cc: (c_common_attribute_table): add three new
> attributes
> > namely: fd_arg, fd_arg_read and fd_arg_write.
> > (handle_fd_arg_attribute): New.
> >
> > gcc/ChangeLog:
> > * doc/extend.texi: Add fd_arg, fd_arg_read and fd_arg_write under
> > "Common Function Attributes" section.
> > * doc/invoke.texi: Add docs to
> -Wanalyzer-fd-access-mode-mismatch,
> > -Wanalyzer-use-after-close, -Wanalyzer-fd-use-without-check that
> these
> > warnings may be emitted through usage of three function
> attributes used
> > for static analysis of file descriptors namely fd_arg,
> fd_arg_read and
> > fd_arg_write.
> >
> > gcc/testsuite/ChangeLog:
> > * gcc.dg/analyzer/fd-5.c: New test.
> > * c-c++-common/attr-fd.c: New test.
> >
> > Signed-off-by: Immad Mir <mirimmad@outlook.com>
> > ---
> > gcc/analyzer/sm-fd.cc | 335 +++++++++++++++++++++------
> > gcc/c-family/c-attribs.cc | 32 +++
> > gcc/doc/extend.texi | 37 +++
> > gcc/doc/invoke.texi | 17 +-
> > gcc/testsuite/c-c++-common/attr-fd.c | 18 ++
> > gcc/testsuite/gcc.dg/analyzer/fd-5.c | 53 +++++
> > 6 files changed, 422 insertions(+), 70 deletions(-)
> > create mode 100644 gcc/testsuite/c-c++-common/attr-fd.c
> > create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
> >
> > diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
> > index 8e4300b06e2..bb89c471f7e 100644
> > --- a/gcc/analyzer/sm-fd.cc
> > +++ b/gcc/analyzer/sm-fd.cc
> > @@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not see
> > #include "analyzer/analyzer-selftests.h"
> > #include "tristate.h"
> > #include "selftest.h"
> > +#include "stringpool.h"
> > +#include "attribs.h"
> > #include "analyzer/call-string.h"
> > #include "analyzer/program-point.h"
> > #include "analyzer/store.h"
> > #include "analyzer/region-model.h"
> > +#include "bitmap.h"
> >
> > #if ENABLE_ANALYZER
> >
> > @@ -59,6 +62,13 @@ enum access_mode
> > WRITE_ONLY
> > };
> >
> > +enum access_directions
> > +{
> > + DIRS_READ_WRITE,
> > + DIRS_READ,
> > + DIRS_WRITE
> > +};
> > +
> > class fd_state_machine : public state_machine
> > {
> > public:
> > @@ -146,7 +156,7 @@ private:
> > 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;
> > + enum access_directions access_fn) const;
> >
> > void make_valid_transitions_on_condition (sm_context *sm_ctxt,
> > const supernode *node,
> > @@ -156,6 +166,10 @@ private:
> > const supernode *node,
> > const gimple *stmt,
> > const svalue *lhs) const;
> > + void check_for_fd_attrs (sm_context *sm_ctxt, const supernode *node,
> > + const gimple *stmt, const tree callee_fndecl,
> Hi Immad,
> Sorry to nitpick -- I assume stmt here refers to a call stmt ?
> In that case, I suppose it'd be better to use const gcall *stmt ?
>
> Thanks,
> Prathamesh
> > + const char *attr_name,
> > + access_directions fd_attr_access_dir) const;
> > };
> >
> > /* Base diagnostic class relative to fd_state_machine. */
> > @@ -220,6 +234,68 @@ protected:
> > tree m_arg;
> > };
> >
> > +class fd_param_diagnostic : public fd_diagnostic
> > +{
> > +public:
> > + fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree
> callee_fndecl,
> > + const char *attr_name, int arg_idx)
> > + : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
> > + m_attr_name (attr_name), m_arg_idx (arg_idx)
> > + {
> > + }
> > +
> > + fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree
> callee_fndecl)
> > + : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
> > + m_attr_name (NULL), m_arg_idx (-1)
> > + {
> > + }
> > +
> > + bool
> > + subclass_equal_p (const pending_diagnostic &base_other) const override
> > + {
> > + const fd_param_diagnostic &sub_other
> > + = (const fd_param_diagnostic &)base_other;
> > + return (same_tree_p (m_arg, sub_other.m_arg)
> > + && same_tree_p (m_callee_fndecl, sub_other.m_callee_fndecl)
> > + && m_arg_idx == sub_other.m_arg_idx
> > + && ((m_attr_name) ? (m_attr_name == sub_other.m_attr_name)
> : true));
> > + }
> > +
> > + void
> > + inform_filedescriptor_attribute (access_directions fd_dir)
> > + {
> > +
> > + if (m_attr_name)
> > + switch (fd_dir)
> > + {
> > + case DIRS_READ_WRITE:
> > + inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
> > + "argument %d of %qD must be an open file descriptor,
> due to "
> > + "%<__attribute__((%s(%d)))%>",
> > + m_arg_idx + 1, m_callee_fndecl, m_attr_name,
> m_arg_idx + 1);
> > + break;
> > + case DIRS_WRITE:
> > + inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
> > + "argument %d of %qD must be a readable file
> descriptor, due "
> > + "to %<__attribute__((%s(%d)))%>",
> > + m_arg_idx + 1, m_callee_fndecl, m_attr_name,
> m_arg_idx + 1);
> > + break;
> > + case DIRS_READ:
> > + inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
> > + "argument %d of %qD must be a writable file
> descriptor, due "
> > + "to %<__attribute__((%s(%d)))%>",
> > + m_arg_idx + 1, m_callee_fndecl, m_attr_name,
> m_arg_idx + 1);
> > + break;
> > + }
> > + }
> > +
> > +protected:
> > + tree m_callee_fndecl;
> > + const char *m_attr_name;
> > + /* ARG_IDX is 0-based. */
> > + int m_arg_idx;
> > +};
> > +
> > class fd_leak : public fd_diagnostic
> > {
> > public:
> > @@ -290,18 +366,26 @@ private:
> > diagnostic_event_id_t m_open_event;
> > };
> >
> > -class fd_access_mode_mismatch : public fd_diagnostic
> > +class fd_access_mode_mismatch : public fd_param_diagnostic
> > {
> > public:
> > fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
> > - enum access_direction fd_dir,
> > - const tree callee_fndecl)
> > - : fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
> > - m_callee_fndecl (callee_fndecl)
> > + enum access_directions fd_dir,
> > + const tree callee_fndecl, const char
> *attr_name,
> > + int arg_idx)
> > + : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name,
> arg_idx),
> > + m_fd_dir (fd_dir)
> >
> > {
> > }
> >
> > + fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
> > + enum access_directions fd_dir,
> > + const tree callee_fndecl)
> > + : fd_param_diagnostic (sm, arg, callee_fndecl), m_fd_dir (fd_dir)
> > + {
> > + }
> > +
> > const char *
> > get_kind () const final override
> > {
> > @@ -317,29 +401,25 @@ public:
> > bool
> > emit (rich_location *rich_loc) final override
> > {
> > + bool warned;
> > switch (m_fd_dir)
> > {
> > - case DIR_READ:
> > - return warning_at (rich_loc, get_controlling_option (),
> > - "%qE on %<read-only%> file descriptor %qE",
> > + case DIRS_READ:
> > + warned = 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",
> > + break;
> > + case DIRS_WRITE:
> > + warned = warning_at (rich_loc, get_controlling_option (),
> > + "%qE on write-only file descriptor %qE",
> > m_callee_fndecl, m_arg);
> > + break;
> > 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_fd_dir == sub_other.m_fd_dir);
> > + if (warned)
> > + inform_filedescriptor_attribute (m_fd_dir);
> > + return warned;
> > }
> >
> > label_text
> > @@ -347,11 +427,11 @@ public:
> > {
> > switch (m_fd_dir)
> > {
> > - case DIR_READ:
> > - return ev.formatted_print ("%qE on %<read-only%> file
> descriptor %qE",
> > + case DIRS_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",
> > + case DIRS_WRITE:
> > + return ev.formatted_print ("%qE on write-only file descriptor
> %qE",
> > m_callee_fndecl, m_arg);
> > default:
> > gcc_unreachable ();
> > @@ -359,21 +439,20 @@ public:
> > }
> >
> > private:
> > - enum access_direction m_fd_dir;
> > - const tree m_callee_fndecl;
> > + enum access_directions m_fd_dir;
> > };
> >
> > -class double_close : public fd_diagnostic
> > +class fd_double_close : public fd_diagnostic
> > {
> > public:
> > - double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic
> (sm, arg)
> > + fd_double_close (const fd_state_machine &sm, tree arg) :
> fd_diagnostic (sm, arg)
> > {
> > }
> >
> > const char *
> > get_kind () const final override
> > {
> > - return "double_close";
> > + return "fd_double_close";
> > }
> >
> > int
> > @@ -418,12 +497,19 @@ private:
> > diagnostic_event_id_t m_first_close_event;
> > };
> >
> > -class fd_use_after_close : public fd_diagnostic
> > +class fd_use_after_close : public fd_param_diagnostic
> > {
> > public:
> > + fd_use_after_close (const fd_state_machine &sm, tree arg,
> > + const tree callee_fndecl, const char *attr_name,
> > + int arg_idx)
> > + : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx)
> > + {
> > + }
> > +
> > fd_use_after_close (const fd_state_machine &sm, tree arg,
> > const tree callee_fndecl)
> > - : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
> > + : fd_param_diagnostic (sm, arg, callee_fndecl)
> > {
> > }
> >
> > @@ -442,9 +528,13 @@ public:
> > bool
> > emit (rich_location *rich_loc) final override
> > {
> > - return warning_at (rich_loc, get_controlling_option (),
> > + bool warned;
> > + warned = warning_at (rich_loc, get_controlling_option (),
> > "%qE on closed file descriptor %qE",
> m_callee_fndecl,
> > m_arg);
> > + if (warned)
> > + inform_filedescriptor_attribute (DIRS_READ_WRITE);
> > + return warned;
> > }
> >
> > label_text
> > @@ -466,32 +556,38 @@ public:
> > describe_final_event (const evdesc::final_event &ev) final override
> > {
> > if (m_first_close_event.known_p ())
> > - return ev.formatted_print (
> > - "%qE on closed file descriptor %qE; %qs was at %@",
> m_callee_fndecl,
> > - m_arg, "close", &m_first_close_event);
> > - else
> > - return ev.formatted_print ("%qE on closed file descriptor %qE",
> > - m_callee_fndecl, m_arg);
> > + return ev.formatted_print (
> > + "%qE on closed file descriptor %qE; %qs was at %@",
> m_callee_fndecl,
> > + m_arg, "close", &m_first_close_event);
> > + else
> > + return ev.formatted_print ("%qE on closed file descriptor %qE",
> > + m_callee_fndecl, m_arg);
> > }
> >
> > private:
> > diagnostic_event_id_t m_first_close_event;
> > - const tree m_callee_fndecl;
> > };
> >
> > -class unchecked_use_of_fd : public fd_diagnostic
> > +class fd_use_without_check : public fd_param_diagnostic
> > {
> > public:
> > - unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
> > - const tree callee_fndecl)
> > - : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
> > + fd_use_without_check (const fd_state_machine &sm, tree arg,
> > + const tree callee_fndecl, const char *attr_name,
> > + int arg_idx)
> > + : fd_param_diagnostic (sm, arg, callee_fndecl, attr_name, arg_idx)
> > + {
> > + }
> > +
> > + fd_use_without_check (const fd_state_machine &sm, tree arg,
> > + const tree callee_fndecl)
> > + : fd_param_diagnostic (sm, arg, callee_fndecl)
> > {
> > }
> >
> > const char *
> > get_kind () const final override
> > {
> > - return "unchecked_use_of_fd";
> > + return "fd_use_without_check";
> > }
> >
> > int
> > @@ -503,18 +599,12 @@ public:
> > 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);
> > + bool warned;
> > + warned = warning_at (rich_loc, get_controlling_option (),
> > + "%qE on possibly invalid file descriptor %qE",
> > + m_callee_fndecl, m_arg);
> > + if (warned)
> > + inform_filedescriptor_attribute (DIRS_READ_WRITE);
> > }
> >
> > label_text
> > @@ -541,8 +631,7 @@ public:
> > }
> >
> > private:
> > - diagnostic_event_id_t m_first_open_event;
> > - const tree m_callee_fndecl;
> > + diagnostic_event_id_t m_first_open_event;
> > };
> >
> > fd_state_machine::fd_state_machine (logger *logger)
> > @@ -647,11 +736,117 @@ fd_state_machine::on_stmt (sm_context *sm_ctxt,
> const supernode *node,
> > on_read (sm_ctxt, node, stmt, call, callee_fndecl);
> > return true;
> > } // "read"
> > +
> > +
> > + {
> > + // Handle __attribute__((fd_arg))
> > +
> > + check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl,
> "fd_arg", DIRS_READ_WRITE);
> > +
> > + // Handle __attribute__((fd_arg_read))
> > +
> > + check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl,
> "fd_arg_read", DIRS_READ);
> > +
> > + // Handle __attribute__((fd_arg_write))
> > +
> > + check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl,
> "fd_arg_write", DIRS_WRITE);
> > +
> > + return true;
> > + }
> > +
> > }
> >
> > return false;
> > }
> >
> > +void
> > +fd_state_machine::check_for_fd_attrs (
> > + sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
> > + const tree callee_fndecl, const char *attr_name,
> > + access_directions fd_attr_access_dir) const
> > +{
> > +
> > + tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
> > + attrs = lookup_attribute (attr_name, attrs);
> > + if (!attrs)
> > + return;
> > +
> > + if (!TREE_VALUE (attrs))
> > + return;
> > +
> > + auto_bitmap argmap;
> > +
> > + for (tree idx = TREE_VALUE (attrs); idx; idx = TREE_CHAIN (idx))
> > + {
> > + unsigned int val = TREE_INT_CST_LOW (TREE_VALUE (idx)) - 1;
> > + bitmap_set_bit (argmap, val);
> > + }
> > + if (bitmap_empty_p (argmap))
> > + return;
> > +
> > + for (unsigned arg_idx = 0; arg_idx < gimple_call_num_args (stmt);
> arg_idx++)
> > + {
> > + tree arg = gimple_call_arg (stmt, arg_idx);
> > + tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > + state_t state = sm_ctxt->get_state (stmt, arg);
> > + bool bit_set = bitmap_bit_p (argmap, arg_idx);
> > + if (TREE_CODE (TREE_TYPE (arg)) != INTEGER_TYPE)
> > + continue;
> > + if (bit_set) // Check if arg_idx is marked by any of the file
> descriptor
> > + // attributes
> > + {
> > +
> > + if (is_closed_fd_p (state))
> > + {
> > +
> > + sm_ctxt->warn (node, stmt, arg,
> > + new fd_use_after_close (*this, diag_arg,
> > + callee_fndecl,
> attr_name,
> > + arg_idx));
> > + continue;
> > + }
> > +
> > + if (!(is_valid_fd_p (state) || (state == m_stop)))
> > + {
> > + if (!is_constant_fd_p (state))
> > + sm_ctxt->warn (node, stmt, arg,
> > + new fd_use_without_check (*this,
> diag_arg,
> > + callee_fndecl,
> attr_name,
> > + arg_idx));
> > + }
> > +
> > + switch (fd_attr_access_dir)
> > + {
> > + case DIRS_READ_WRITE:
> > + break;
> > + case DIRS_READ:
> > +
> > + if (is_writeonly_fd_p (state))
> > + {
> > + sm_ctxt->warn (
> > + node, stmt, arg,
> > + new fd_access_mode_mismatch (*this, diag_arg,
> DIRS_WRITE,
> > + callee_fndecl,
> attr_name, arg_idx));
> > + }
> > +
> > + break;
> > + case DIRS_WRITE:
> > +
> > + if (is_readonly_fd_p (state))
> > + {
> > + sm_ctxt->warn (
> > + node, stmt, arg,
> > + new fd_access_mode_mismatch (*this, diag_arg,
> DIRS_READ,
> > + callee_fndecl,
> attr_name, arg_idx));
> > + }
> > +
> > + break;
> > + }
> > + }
> > + }
> > +}
> > +
> > +
> > void
> > fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
> > const gimple *stmt, const gcall *call) const
> > @@ -706,7 +901,7 @@ fd_state_machine::on_close (sm_context *sm_ctxt,
> const supernode *node,
> >
> > if (is_closed_fd_p (state))
> > {
> > - sm_ctxt->warn (node, stmt, arg, new double_close (*this,
> diag_arg));
> > + sm_ctxt->warn (node, stmt, arg, new fd_double_close (*this,
> diag_arg));
> > sm_ctxt->set_next_state (stmt, arg, m_stop);
> > }
> > }
> > @@ -715,21 +910,21 @@ 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);
> > + check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl,
> DIRS_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);
> > + check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl,
> DIRS_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 callee_fndecl_dir) const
> > + enum access_directions callee_fndecl_dir) const
> > {
> > tree arg = gimple_call_arg (call, 0);
> > tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > @@ -748,30 +943,32 @@ fd_state_machine::check_for_open_fd (
> > if (!is_constant_fd_p (state))
> > sm_ctxt->warn (
> > node, stmt, arg,
> > - new unchecked_use_of_fd (*this, diag_arg,
> callee_fndecl));
> > + new fd_use_without_check (*this, diag_arg,
> callee_fndecl));
> > }
> > switch (callee_fndecl_dir)
> > {
> > - case DIR_READ:
> > + case DIRS_READ:
> > if (is_writeonly_fd_p (state))
> > {
> > tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > sm_ctxt->warn (node, stmt, arg,
> > new fd_access_mode_mismatch (
> > - *this, diag_arg, DIR_WRITE,
> callee_fndecl));
> > + *this, diag_arg, DIRS_WRITE,
> callee_fndecl));
> > }
> >
> > break;
> > - case DIR_WRITE:
> > + case DIRS_WRITE:
> >
> > if (is_readonly_fd_p (state))
> > {
> > tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> > sm_ctxt->warn (node, stmt, arg,
> > new fd_access_mode_mismatch (
> > - *this, diag_arg, DIR_READ,
> callee_fndecl));
> > + *this, diag_arg, DIRS_READ,
> callee_fndecl));
> > }
> > break;
> > + default:
> > + gcc_unreachable ();
> > }
> > }
> > }
> > diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> > index c8d96723f4c..0ad3a1a7040 100644
> > --- a/gcc/c-family/c-attribs.cc
> > +++ b/gcc/c-family/c-attribs.cc
> > @@ -173,6 +173,7 @@ static tree handle_objc_nullability_attribute (tree
> *, tree, tree, int, bool *);
> > static tree handle_signed_bool_precision_attribute (tree *, tree, tree,
> int,
> > bool *);
> > static tree handle_retain_attribute (tree *, tree, tree, int, bool *);
> > +static tree handle_fd_arg_attribute (tree *, tree, tree, int, bool *);
> >
> > /* Helper to define attribute exclusions. */
> > #define ATTR_EXCL(name, function, type, variable) \
> > @@ -555,6 +556,12 @@ const struct attribute_spec
> c_common_attribute_table[] =
> > handle_dealloc_attribute, NULL },
> > { "tainted_args", 0, 0, true, false, false, false,
> > handle_tainted_args_attribute, NULL },
> > + { "fd_arg", 1, 1, false, true, true, false,
> > + handle_fd_arg_attribute, NULL},
> > + { "fd_arg_read", 1, 1, false, true, true, false,
> > + handle_fd_arg_attribute, NULL},
> > + { "fd_arg_write", 1, 1, false, true, true, false,
> > + handle_fd_arg_attribute, NULL},
> > { NULL, 0, 0, false, false, false, false, NULL,
> NULL }
> > };
> >
> > @@ -4521,6 +4528,31 @@ handle_nonnull_attribute (tree *node, tree name,
> > return NULL_TREE;
> > }
> >
> > +/* Handle the "fd_arg", "fd_arg_read" and "fd_arg_write" attributes */
> > +
> > +static tree
> > +handle_fd_arg_attribute (tree *node, tree name, tree args,
> > + int ARG_UNUSED (flags), bool
> *no_add_attrs)
> > +{
> > + tree type = *node;
> > + if (!args)
> > + {
> > + if (!prototype_p (type))
> > + {
> > + error ("%qE attribute without arguments on a non-prototype",
> name);
> > + *no_add_attrs = true;
> > + }
> > + return NULL_TREE;
> > + }
> > +
> > + if (tree val = positional_argument (*node, name, TREE_VALUE (args),
> > + INTEGER_TYPE))
> > + return NULL_TREE;
> > +
> > + *no_add_attrs = true;
> > + return NULL_TREE;
> > +}
> > +
> > /* Handle the "nonstring" variable attribute. */
> >
> > static tree
> > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> > index dfbe33ac652..1d08324a70c 100644
> > --- a/gcc/doc/extend.texi
> > +++ b/gcc/doc/extend.texi
> > @@ -3007,6 +3007,43 @@ produced by @command{gold}.
> > For other linkers that cannot generate resolution file,
> > explicit @code{externally_visible} attributes are still necessary.
> >
> > +@item fd_arg
> > +@itemx fd_arg (@var{N})
> > +@cindex @code{fd_arg} function attribute
> > +The @code{fd_arg} attribute may be applied to a function that takes an
> open
> > +file descriptor at referenced argument @var{N}.
> > +
> > +It indicates that the passed filedescriptor must not have been closed.
> > +Therefore, when the analyzer is enabled with @option{-fanalyzer}, the
> > +analyzer may emit a @option{-Wanalyzer-fd-use-after-close} diagnostic
> > +if it detects a code path in which a function with this attribute is
> > +called with a closed file descriptor.
> > +
> > +The attribute also indicates that the file descriptor must have been
> checked for
> > +validity before usage. Therefore, analyzer may emit
> > +@option{-Wanalyzer-fd-use-without-check} diagnostic if it detects a
> code path in
> > +which a function with this attribute is called with a file descriptor
> that has
> > +not been checked for validity.
> > +
> > +@item fd_arg_read
> > +@itemx fd_arg_read (@var{N})
> > +@cindex @code{fd_arg_read} function attribute
> > +The @code{fd_arg_read} is identical to @code{fd_arg}, but with the
> additional
> > +requirement that it might read from the file descriptor, and thus, the
> file
> > +descriptor must not have been opened as write-only.
> > +
> > +The analyzer may emit a @option{-Wanalyzer-access-mode-mismatch}
> > +diagnostic if it detects a code path in which a function with this
> > +attribute is called on a file descriptor opened with @code{O_WRONLY}.
> > +
> > +@item fd_arg_write
> > +@itemx fd_arg_write (@var{N})
> > +@cindex @code{fd_arg_write} function attribute
> > +The @code{fd_arg_write} is identical to @code{fd_arg_read} except that
> the
> > +analyzer may emit a @option{-Wanalyzer-access-mode-mismatch} diagnostic
> if
> > +it detects a code path in which a function with this attribute is
> called on a
> > +file descriptor opened with @code{O_RDONLY}.
> > +
> > @item flatten
> > @cindex @code{flatten} function attribute
> > Generally, inlining into a function is limited. For a function marked
> with
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index d5ff1018372..9234275ca6d 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -9843,7 +9843,12 @@ This warning requires @option{-fanalyzer}, which
> enables it; use
> > 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
> > +@code{read} on a write-only file descriptor is attempted, or vice versa.
> > +
> > +This diagnostic also warns for code paths in a which a function with
> attribute
> > +@code{fd_arg_read (N)} is called with a file descriptor opened with
> @code{O_WRONLY}
> > +at refrenced argument @code{N} or a function with attribute
> @code{fd_arg_write (N)}
> > +is called with a file descriptor opened with @code{O_RDONLY} at
> refrenced argument @var{N},
> >
> > @item -Wno-analyzer-fd-double-close
> > @opindex Wanalyzer-fd-double-close
> > @@ -9875,6 +9880,11 @@ to disable it.
> > This diagnostic warns for paths through code in which a
> > read or write is called on a closed file descriptor.
> >
> > +This diagnostic also warns for paths through code in which
> > +a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
> > +or @code{fd_arg_write (N)} is called with a closed file descriptor at
> > +refrenced argument @code{N}.
> > +
> > @item -Wno-analyzer-fd-use-without-check
> > @opindex Wanalyzer-fd-use-without-check
> > @opindex Wno-analyzer-fd-use-without-check
> > @@ -9885,6 +9895,11 @@ to disable it.
> > This diagnostic warns for paths through code in which a
> > file descriptor is used without being checked for validity.
> >
> > +This diagnostic also warns for paths through code in which
> > +a function with attribute @code{fd_arg (N)} or @code{fd_arg_read (N)}
> > +or @code{fd_arg_write (N)} is called with a file descriptor, at
> refrenced
> > +argument @code{N}, without being checked for validity.
> > +
> > @item -Wno-analyzer-file-leak
> > @opindex Wanalyzer-file-leak
> > @opindex Wno-analyzer-file-leak
> > diff --git a/gcc/testsuite/c-c++-common/attr-fd.c
> b/gcc/testsuite/c-c++-common/attr-fd.c
> > new file mode 100644
> > index 00000000000..292c329856e
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/attr-fd.c
> > @@ -0,0 +1,18 @@
> > +
> > +int not_a_fn __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg'
> attribute only applies to function types" } */
> > +
> > +void f (char *p) __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg'
> attribute argument value '1' refers to parameter type 'char \\\*'" } */
> > +
> > +
> > +int not_a_fn_b __attribute__ ((fd_arg_read(1))); /* { dg-warning
> "'fd_arg_read' attribute only applies to function types" } */
> > +
> > +void g (char *p) __attribute__ ((fd_arg_read(1))); /* { dg-warning
> "'fd_arg_read' attribute argument value '1' refers to parameter type 'char
> \\\*'" } */
> > +
> > +
> > +int not_a_fn_c __attribute__ ((fd_arg_write(1))); /* { dg-warning
> "'fd_arg_write' attribute only applies to function types" } */
> > +
> > +void f (char *p) __attribute__ ((fd_arg_write(1))); /* { dg-warning
> "'fd_arg_write' attribute argument value '1' refers to parameter type 'char
> \\\*'" } */
> > +
> > +
> > +void fn_a (int fd) __attribute__ ((fd_arg(0))); /* { dg-warning
> "'fd_arg' attribute argument value '0' does not refer to a function
> parameter" } */
> > +void fd_a_1 (int fd) __attribute__ ((fd_arg("notint"))); /* {
> dg-warning "'fd_arg' attribute argument has type 'char\\\[7\\\]'" } */
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> > new file mode 100644
> > index 00000000000..9805f7f79c5
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> > @@ -0,0 +1,53 @@
> > +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 f (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1
> of 'f' must be an open file descriptor, due to
> '__attribute__\\(\\(fd_arg\\(1\\)\\)\\)'" } */
> > +
> > +void
> > +test_1 (const char *path)
> > +{
> > + int fd = open (path, O_RDWR);
> > + close(fd);
> > + f(fd); /* { dg-warning "'f' on closed file descriptor 'fd'" } */
> > + /* { dg-message "\\(3\\) 'f' on closed file descriptor 'fd';
> 'close' was at \\(2\\)" "" { target *-*-* } .-1 } */
> > +}
> > +
> > +void g (int fd) __attribute__((fd_arg_read(1))); /* { dg-message
> "argument 1 of 'g' must be a readable file descriptor, due to
> '__attribute__\\(\\(fd_arg_read\\(1\\)\\)\\)'" } */
> > +
> > +void
> > +test_2 (const char *path)
> > +{
> > + int fd = open (path, O_WRONLY);
> > + if (fd != -1)
> > + {
> > + g (fd); /* { dg-warning "'g' on 'write-only' file descriptor 'fd'"
> } */
> > + }
> > + close (fd);
> > +}
> > +
> > +void h (int fd) __attribute__((fd_arg_write(1))); /* { dg-message
> "argument 1 of 'h' must be a writable file descriptor, due to
> '__attribute__\\(\\(fd_arg_write\\(1\\)\\)\\)'" } */
> > +void
> > +test_3 (const char *path)
> > +{
> > + int fd = open (path, O_RDONLY);
> > + if (fd != -1)
> > + {
> > + h (fd); /* { dg-warning "'h' on 'read-only' file descriptor 'fd'" }
> */
> > + }
> > + close(fd);
> > +}
> > +
> > +void ff (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument
> 1 of 'ff' must be an open file descriptor, due to
> '__attribute__\\(\\(fd_arg\\(1\\)\\)\\)'" } */
> > +
> > +void test_4 (const char *path)
> > +{
> > + int fd = open (path, O_RDWR);
> > + ff (fd); /* { dg-warning "'ff' on possibly invalid file descriptor
> 'fd'" } */
> > + close(fd);
> > +}
> > \ No newline at end of file
> > --
> > 2.25.1
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Adding three new function attributes for static analysis of file descriptors
@ 2022-07-19 16:06 Immad Mir
2022-07-19 18:18 ` David Malcolm
0 siblings, 1 reply; 11+ messages in thread
From: Immad Mir @ 2022-07-19 16:06 UTC (permalink / raw)
To: gcc-patches; +Cc: dmalcolm, mirimnan017, Immad Mir
This patch adds three new function attributes to GCC that
are used for static analysis of usage of file descriptors:
1) __attribute__ ((fd_arg(N))): The attributes may be applied to a function that
takes on open file descriptor at refrenced argument N.
It indicates that the passed filedescriptor must not have been closed.
Therefore, when the analyzer is enabled with -fanalyzer, the
analyzer may emit a -Wanalyzer-fd-use-after-close diagnostic
if it detects a code path in which a function with this attribute is
called with a closed file descriptor.
The attribute also indicates that the file descriptor must have been checked for
validity before usage. Therefore, analyzer may emit
-Wanalyzer-fd-use-without-check diagnostic if it detects a code path in
which a function with this attribute is called with a file descriptor that has
not been checked for validity.
2) __attribute__((fd_arg_read(N))): The attribute is identical to
fd_arg, but with the additional requirement that it might read from
the file descriptor, and thus, the file descriptor must not have been opened
as write-only.
The analyzer may emit a -Wanalyzer-access-mode-mismatch
diagnostic if it detects a code path in which a function with this
attribute is called on a file descriptor opened with O_WRONLY.
3) __attribute__((fd_arg_write(N))): The attribute is identical to fd_arg_read
except that the analyzer may emit a -Wanalyzer-access-mode-mismatch diagnostic if
it detects a code path in which a function with this attribute is called on a
file descriptor opened with O_RDONLY.
gcc/analyzer/ChangeLog:
* sm-fd.cc (fd_param_diagnostic): New diagnostic class.
(fd_access_mode_mismatch): Change inheritance from fd_diagnostic
to fd_param_diagnostic. Add new overloaded constructor.
(fd_use_after_close): Likewise.
(unchecked_use_of_fd): Likewise and also change name to fd_use_without_check.
(double_close): Change name to fd_double_close.
(enum access_directions): New.
(fd_state_machine::on_stmt): Handle calls to function with the
new three function attributes.
(fd_state_machine::check_for_fd_attrs): New.
(fd_state_machine::on_open): Use the new overloaded constructors
of diagnostic classes.
gcc/c-family/ChangeLog:
* c-attribs.cc: (c_common_attribute_table): add three new attributes
namely: fd_arg, fd_arg_read and fd_arg_write.
(handle_fd_arg_attribute): New.
gcc/ChangeLog:
* doc/extend.texi: Add fd_arg, fd_arg_read and fd_arg_write under
"Common Function Attributes" section.
gcc/testsuite/ChangeLog:
* gcc.dg/analyzer/fd-5.c: New test.
* c-c++-common/attr-fd.c: New test.
Signed-off-by: Immad Mir <mirimmad@outlook.com>
---
gcc/analyzer/sm-fd.cc | 323 +++++++++++++++++++++------
gcc/c-family/c-attribs.cc | 36 ++-
gcc/doc/extend.texi | 34 +++
gcc/testsuite/c-c++-common/attr-fd.c | 18 ++
gcc/testsuite/gcc.dg/analyzer/fd-5.c | 53 +++++
5 files changed, 398 insertions(+), 66 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/attr-fd.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
index 8e4300b06e2..c577a7db7ea 100644
--- a/gcc/analyzer/sm-fd.cc
+++ b/gcc/analyzer/sm-fd.cc
@@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not see
#include "analyzer/analyzer-selftests.h"
#include "tristate.h"
#include "selftest.h"
+#include "stringpool.h"
+#include "attribs.h"
#include "analyzer/call-string.h"
#include "analyzer/program-point.h"
#include "analyzer/store.h"
#include "analyzer/region-model.h"
+#include "bitmap.h"
#if ENABLE_ANALYZER
@@ -59,6 +62,13 @@ enum access_mode
WRITE_ONLY
};
+enum access_directions
+{
+ DIRS_READ_WRITE,
+ DIRS_READ,
+ DIRS_WRITE
+};
+
class fd_state_machine : public state_machine
{
public:
@@ -146,7 +156,7 @@ private:
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;
+ enum access_directions access_fn) const;
void make_valid_transitions_on_condition (sm_context *sm_ctxt,
const supernode *node,
@@ -156,12 +166,17 @@ private:
const supernode *node,
const gimple *stmt,
const svalue *lhs) const;
+ void check_for_fd_attrs (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const tree callee_fndecl,
+ const char *attr_name,
+ access_directions fd_attr_access_dir) const;
+
};
/* Base diagnostic class relative to fd_state_machine. */
class fd_diagnostic : public pending_diagnostic
{
-public:
+public:
fd_diagnostic (const fd_state_machine &sm, tree arg) : m_sm (sm), m_arg (arg)
{
}
@@ -220,6 +235,65 @@ protected:
tree m_arg;
};
+class fd_param_diagnostic : public fd_diagnostic
+{
+public:
+ fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree callee_fndecl,
+ bool attr, int arg_idx)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl), m_attr (attr),
+ m_arg_idx (arg_idx)
+ {
+ }
+
+ fd_param_diagnostic (const fd_state_machine &sm, tree arg, tree callee_fndecl)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
+ m_attr (false), m_arg_idx (-1)
+ {
+ }
+
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ const fd_param_diagnostic &sub_other
+ = (const fd_param_diagnostic &)base_other;
+ return (same_tree_p (m_arg, sub_other.m_arg)
+ && m_callee_fndecl == sub_other.m_callee_fndecl
+ && m_attr == sub_other.m_attr
+ && m_arg_idx == sub_other.m_arg_idx);
+ }
+
+ void
+ inform_filedescriptor_attribute (access_directions fd_dir)
+ {
+
+ if (m_attr)
+ switch (fd_dir)
+ {
+ case DIRS_READ_WRITE:
+ inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
+ "argument %d of %qD must be an open file descriptor",
+ m_arg_idx + 1, m_callee_fndecl);
+ break;
+ case DIRS_WRITE:
+ inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
+ "argument %d of %qD must be a read-only file descriptor",
+ m_arg_idx + 1, m_callee_fndecl);
+ break;
+ case DIRS_READ:
+ inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
+ "argument %d of %qD must be a write-only file descriptor",
+ m_arg_idx + 1, m_callee_fndecl);
+ break;
+ }
+ }
+
+protected:
+ tree m_callee_fndecl;
+ bool m_attr;
+ /* ARG_IDX is 0-based. */
+ int m_arg_idx;
+};
+
class fd_leak : public fd_diagnostic
{
public:
@@ -290,18 +364,25 @@ private:
diagnostic_event_id_t m_open_event;
};
-class fd_access_mode_mismatch : public fd_diagnostic
+class fd_access_mode_mismatch : public fd_param_diagnostic
{
public:
fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
- enum access_direction fd_dir,
- const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
- m_callee_fndecl (callee_fndecl)
+ enum access_directions fd_dir,
+ const tree callee_fndecl, bool attr, int arg_idx)
+ : fd_param_diagnostic (sm, arg, callee_fndecl, attr, arg_idx),
+ m_fd_dir (fd_dir)
{
}
+ fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
+ enum access_directions fd_dir,
+ const tree callee_fndecl)
+ : fd_param_diagnostic (sm, arg, callee_fndecl), m_fd_dir (fd_dir)
+ {
+ }
+
const char *
get_kind () const final override
{
@@ -317,29 +398,25 @@ public:
bool
emit (rich_location *rich_loc) final override
{
+ bool warned;
switch (m_fd_dir)
{
- case DIR_READ:
- return warning_at (rich_loc, get_controlling_option (),
+ case DIRS_READ:
+ warned = 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 (),
+ break;
+ case DIRS_WRITE:
+ warned = warning_at (rich_loc, get_controlling_option (),
"%qE on %<write-only%> file descriptor %qE",
m_callee_fndecl, m_arg);
+ break;
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_fd_dir == sub_other.m_fd_dir);
+ if (warned)
+ inform_filedescriptor_attribute (m_fd_dir);
+ return warned;
}
label_text
@@ -347,10 +424,10 @@ public:
{
switch (m_fd_dir)
{
- case DIR_READ:
+ case DIRS_READ:
return ev.formatted_print ("%qE on %<read-only%> file descriptor %qE",
m_callee_fndecl, m_arg);
- case DIR_WRITE:
+ case DIRS_WRITE:
return ev.formatted_print ("%qE on %<write-only%> file descriptor %qE",
m_callee_fndecl, m_arg);
default:
@@ -359,21 +436,20 @@ public:
}
private:
- enum access_direction m_fd_dir;
- const tree m_callee_fndecl;
+ enum access_directions m_fd_dir;
};
-class double_close : public fd_diagnostic
+class fd_double_close : public fd_diagnostic
{
public:
- double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
+ fd_double_close (const fd_state_machine &sm, tree arg) : fd_diagnostic (sm, arg)
{
}
const char *
get_kind () const final override
{
- return "double_close";
+ return "fd_double_close";
}
int
@@ -418,12 +494,18 @@ private:
diagnostic_event_id_t m_first_close_event;
};
-class fd_use_after_close : public fd_diagnostic
+class fd_use_after_close : public fd_param_diagnostic
{
public:
+ fd_use_after_close (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl, bool attr, int arg_idx)
+ : fd_param_diagnostic(sm, arg, callee_fndecl, attr, arg_idx)
+ {
+ }
+
fd_use_after_close (const fd_state_machine &sm, tree arg,
const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ : fd_param_diagnostic(sm, arg, callee_fndecl)
{
}
@@ -442,9 +524,13 @@ public:
bool
emit (rich_location *rich_loc) final override
{
- return warning_at (rich_loc, get_controlling_option (),
+ bool warned;
+ warned = warning_at (rich_loc, get_controlling_option (),
"%qE on closed file descriptor %qE", m_callee_fndecl,
m_arg);
+ if (warned)
+ inform_filedescriptor_attribute (DIRS_READ_WRITE);
+ return warned;
}
label_text
@@ -466,32 +552,37 @@ public:
describe_final_event (const evdesc::final_event &ev) final override
{
if (m_first_close_event.known_p ())
- return ev.formatted_print (
- "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
- m_arg, "close", &m_first_close_event);
- else
- return ev.formatted_print ("%qE on closed file descriptor %qE",
- m_callee_fndecl, m_arg);
+ return ev.formatted_print (
+ "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
+ m_arg, "close", &m_first_close_event);
+ else
+ return ev.formatted_print ("%qE on closed file descriptor %qE",
+ m_callee_fndecl, m_arg);
}
private:
diagnostic_event_id_t m_first_close_event;
- const tree m_callee_fndecl;
};
-class unchecked_use_of_fd : public fd_diagnostic
+class fd_use_without_check : public fd_param_diagnostic
{
public:
- unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
+ fd_use_without_check (const fd_state_machine &sm, tree arg,
+ const tree callee_fndecl, bool attr, int arg_idx)
+ : fd_param_diagnostic(sm, arg, callee_fndecl, attr, arg_idx)
+ {
+ }
+
+ fd_use_without_check (const fd_state_machine &sm, tree arg,
const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ : fd_param_diagnostic(sm, arg, callee_fndecl)
{
}
const char *
get_kind () const final override
{
- return "unchecked_use_of_fd";
+ return "fd_use_without_check";
}
int
@@ -503,18 +594,12 @@ public:
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);
+ bool warned;
+ warned = warning_at (rich_loc, get_controlling_option (),
+ "%qE on possibly invalid file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ if (warned)
+ inform_filedescriptor_attribute (DIRS_READ_WRITE);
}
label_text
@@ -542,7 +627,7 @@ public:
private:
diagnostic_event_id_t m_first_open_event;
- const tree m_callee_fndecl;
+
};
fd_state_machine::fd_state_machine (logger *logger)
@@ -647,11 +732,119 @@ fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
on_read (sm_ctxt, node, stmt, call, callee_fndecl);
return true;
} // "read"
+
+
+ {
+ // Handle __attribute__((fd_arg))
+
+ check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl, "fd_arg", DIRS_READ_WRITE);
+
+ // Handle __attribute__((fd_arg_read))
+
+ check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl, "fd_arg_read", DIRS_READ);
+
+ // Handle __attribute__((fd_arg_write))
+
+ check_for_fd_attrs (sm_ctxt, node, stmt, callee_fndecl, "fd_arg_write", DIRS_WRITE);
+
+ return true;
+ }
+
}
return false;
}
+void
+fd_state_machine::check_for_fd_attrs (
+ sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
+ const tree callee_fndecl, const char *attr_name,
+ access_directions fd_attr_access_dir) const
+{
+
+ tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
+ attrs = lookup_attribute (attr_name, attrs);
+ if (!attrs)
+ return;
+
+ if (!TREE_VALUE (attrs))
+ return;
+
+ auto_bitmap argmap;
+
+ for (tree idx = TREE_VALUE (attrs); idx; idx = TREE_CHAIN (idx))
+ {
+ unsigned int val = TREE_INT_CST_LOW (TREE_VALUE (idx)) - 1;
+ bitmap_set_bit (argmap, val);
+ }
+ if (bitmap_empty_p (argmap))
+ return;
+
+ for (unsigned i = 0; i < gimple_call_num_args (stmt); i++)
+ {
+
+ unsigned int arg_idx = i;
+ tree arg = gimple_call_arg (stmt, arg_idx);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ bool bit_set = bitmap_bit_p (argmap, arg_idx);
+ if (TREE_CODE (TREE_TYPE (arg)) != INTEGER_TYPE)
+ continue;
+ if (bit_set) // Check if arg_idx is marked by any of the file descriptor
+ // attributes
+ {
+
+ if (is_closed_fd_p (state))
+ {
+
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_after_close (*this, diag_arg,
+ callee_fndecl, true,
+ arg_idx));
+ continue;
+ }
+
+ if (!(is_valid_fd_p (state) || (state == m_stop)))
+ {
+ if (!is_constant_fd_p (state))
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_without_check (*this, diag_arg,
+ callee_fndecl, true,
+ arg_idx));
+ }
+
+ switch (fd_attr_access_dir)
+ {
+ case DIRS_READ_WRITE:
+ break;
+ case DIRS_READ:
+
+ if (is_writeonly_fd_p (state))
+ {
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg, DIRS_WRITE,
+ callee_fndecl, true, i));
+ }
+
+ break;
+ case DIRS_WRITE:
+
+ if (is_readonly_fd_p (state))
+ {
+ sm_ctxt->warn (
+ node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg, DIRS_READ,
+ callee_fndecl, true, i));
+ }
+
+ break;
+ }
+ }
+ }
+}
+
+
void
fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
const gimple *stmt, const gcall *call) const
@@ -706,7 +899,7 @@ fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
if (is_closed_fd_p (state))
{
- sm_ctxt->warn (node, stmt, arg, new double_close (*this, diag_arg));
+ sm_ctxt->warn (node, stmt, arg, new fd_double_close (*this, diag_arg));
sm_ctxt->set_next_state (stmt, arg, m_stop);
}
}
@@ -715,21 +908,21 @@ 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);
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIRS_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);
+ check_for_open_fd (sm_ctxt, node, stmt, call, callee_fndecl, DIRS_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 callee_fndecl_dir) const
+ enum access_directions callee_fndecl_dir) const
{
tree arg = gimple_call_arg (call, 0);
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
@@ -748,30 +941,32 @@ fd_state_machine::check_for_open_fd (
if (!is_constant_fd_p (state))
sm_ctxt->warn (
node, stmt, arg,
- new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
+ new fd_use_without_check (*this, diag_arg, callee_fndecl));
}
switch (callee_fndecl_dir)
{
- case DIR_READ:
+ case DIRS_READ:
if (is_writeonly_fd_p (state))
{
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
sm_ctxt->warn (node, stmt, arg,
new fd_access_mode_mismatch (
- *this, diag_arg, DIR_WRITE, callee_fndecl));
+ *this, diag_arg, DIRS_WRITE, callee_fndecl));
}
break;
- case DIR_WRITE:
+ case DIRS_WRITE:
if (is_readonly_fd_p (state))
{
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
sm_ctxt->warn (node, stmt, arg,
new fd_access_mode_mismatch (
- *this, diag_arg, DIR_READ, callee_fndecl));
+ *this, diag_arg, DIRS_READ, callee_fndecl));
}
break;
+ default:
+ gcc_unreachable ();
}
}
}
diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index c8d96723f4c..a04cc541f95 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -173,6 +173,7 @@ static tree handle_objc_nullability_attribute (tree *, tree, tree, int, bool *);
static tree handle_signed_bool_precision_attribute (tree *, tree, tree, int,
bool *);
static tree handle_retain_attribute (tree *, tree, tree, int, bool *);
+static tree handle_fd_arg_attribute (tree *, tree, tree, int, bool *);
/* Helper to define attribute exclusions. */
#define ATTR_EXCL(name, function, type, variable) \
@@ -426,7 +427,7 @@ const struct attribute_spec c_common_attribute_table[] =
{ "tls_model", 1, 1, true, false, false, false,
handle_tls_model_attribute, NULL },
{ "nonnull", 0, -1, false, true, true, false,
- handle_nonnull_attribute, NULL },
+ handle_nonnull_attribute, NULL },
{ "nonstring", 0, 0, true, false, false, false,
handle_nonstring_attribute, NULL },
{ "nothrow", 0, 0, true, false, false, false,
@@ -555,7 +556,13 @@ const struct attribute_spec c_common_attribute_table[] =
handle_dealloc_attribute, NULL },
{ "tainted_args", 0, 0, true, false, false, false,
handle_tainted_args_attribute, NULL },
- { NULL, 0, 0, false, false, false, false, NULL, NULL }
+ { NULL, 0, 0, false, false, false, false, NULL, NULL },
+ { "fd_arg", 1, 1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
+ { "fd_arg_read", 1, 1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
+ { "fd_arg_write", 1, 1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
};
/* Give the specifications for the format attributes, used by C and all
@@ -4521,6 +4528,31 @@ handle_nonnull_attribute (tree *node, tree name,
return NULL_TREE;
}
+/* Handle the "fd_arg", "fd_arg_read" and "fd_arg_write" attributes */
+
+static tree
+handle_fd_arg_attribute (tree *node, tree name, tree args,
+ int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+ tree type = *node;
+ if (!args)
+ {
+ if (!prototype_p (type))
+ {
+ error ("%qE attribute without arguments on a non-prototype", name);
+ *no_add_attrs = true;
+ }
+ return NULL_TREE;
+ }
+
+ if (tree val = positional_argument (*node, name, TREE_VALUE (args),
+ INTEGER_TYPE))
+ return NULL_TREE;
+
+ *no_add_attrs = true;
+ return NULL_TREE;
+}
+
/* Handle the "nonstring" variable attribute. */
static tree
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index dfbe33ac652..7945d769e23 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3007,6 +3007,40 @@ produced by @command{gold}.
For other linkers that cannot generate resolution file,
explicit @code{externally_visible} attributes are still necessary.
+@item fd_arg
+@cindex @code{fd_arg} function attribute
+The @code{fd_arg} attribute may be applied to a function that takes an open
+file descriptor at referenced argument @var{N}.
+
+It indicates that the passed filedescriptor must not have been closed.
+Therefore, when the analyzer is enabled with @option{-fanalyzer}, the
+analyzer may emit a @option{-Wanalyzer-fd-use-after-close} diagnostic
+if it detects a code path in which a function with this attribute is
+called with a closed file descriptor.
+
+The attribute also indicates that the file descriptor must have been checked for
+validity before usage. Therefore, analyzer may emit
+@option{-Wanalyzer-fd-use-without-check} diagnostic if it detects a code path in
+which a function with this attribute is called with a file descriptor that has
+not been checked for validity.
+
+@item fd_arg_read
+@cindex @code{fd_arg_read} function attribute
+The @code{fd_arg_read} is identical to @code{fd_arg}, but with the additional
+requirement that it might read from the file descriptor, and thus, the file
+descriptor must not have been opened as write-only.
+
+The analyzer may emit a @option{-Wanalyzer-access-mode-mismatch}
+diagnostic if it detects a code path in which a function with this
+attribute is called on a file descriptor opened with @code{O_WRONLY}.
+
+@item fd_arg_write
+@cindex @code{fd_arg_write} function attribute
+The @code{fd_arg_write} is identical to @code{fd_arg_read} except that the
+analyzer may emit a @option{-Wanalyzer-access-mode-mismatch} diagnostic if
+it detects a code path in which a function with this attribute is called on a
+file descriptor opened with @code{O_RDONLY}.
+
@item flatten
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function marked with
diff --git a/gcc/testsuite/c-c++-common/attr-fd.c b/gcc/testsuite/c-c++-common/attr-fd.c
new file mode 100644
index 00000000000..292c329856e
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/attr-fd.c
@@ -0,0 +1,18 @@
+
+int not_a_fn __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute only applies to function types" } */
+
+void f (char *p) __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute argument value '1' refers to parameter type 'char \\\*'" } */
+
+
+int not_a_fn_b __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute only applies to function types" } */
+
+void g (char *p) __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute argument value '1' refers to parameter type 'char \\\*'" } */
+
+
+int not_a_fn_c __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute only applies to function types" } */
+
+void f (char *p) __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute argument value '1' refers to parameter type 'char \\\*'" } */
+
+
+void fn_a (int fd) __attribute__ ((fd_arg(0))); /* { dg-warning "'fd_arg' attribute argument value '0' does not refer to a function parameter" } */
+void fd_a_1 (int fd) __attribute__ ((fd_arg("notint"))); /* { dg-warning "'fd_arg' attribute argument has type 'char\\\[7\\\]'" } */
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-5.c b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
new file mode 100644
index 00000000000..6287ddd38fc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
@@ -0,0 +1,53 @@
+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 f (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'f' must be an open file descriptor" } */
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDWR);
+ close(fd);
+ f(fd); /* { dg-warning "'f' on closed file descriptor 'fd'" } */
+ /* { dg-message "\\(3\\) 'f' on closed file descriptor 'fd'; 'close' was at \\(2\\)" "" { target *-*-* } .-1 } */
+}
+
+void g (int fd) __attribute__((fd_arg_read(1))); /* { dg-message "argument 1 of 'g' must be a read-only file descriptor" } */
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_WRONLY);
+ if (fd != -1)
+ {
+ g (fd); /* { dg-warning "'g' on 'write-only' file descriptor 'fd'" } */
+ }
+ close (fd);
+}
+
+void h (int fd) __attribute__((fd_arg_write(1))); /* { dg-message "argument 1 of 'h' must be a write-only file descriptor" } */
+void
+test_3 (const char *path)
+{
+ int fd = open (path, O_RDONLY);
+ if (fd != -1)
+ {
+ h (fd); /* { dg-warning "'h' on 'read-only' file descriptor 'fd'" } */
+ }
+ close(fd);
+}
+
+void ff (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'ff' must be an open file descriptor" } */
+
+void test_4 (const char *path)
+{
+ int fd = open (path, O_RDWR);
+ ff (fd); /* { dg-warning "'ff' on possibly invalid file descriptor 'fd'" } */
+ close(fd);
+}
\ No newline at end of file
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Adding three new function attributes for static analysis of file descriptors
2022-07-19 16:06 Immad Mir
@ 2022-07-19 18:18 ` David Malcolm
2022-07-19 20:08 ` David Malcolm
0 siblings, 1 reply; 11+ messages in thread
From: David Malcolm @ 2022-07-19 18:18 UTC (permalink / raw)
To: mirimnan017, gcc-patches; +Cc: Immad Mir
On Tue, 2022-07-19 at 21:36 +0530, Immad Mir wrote:
[...snip...]
Thanks for the patch.
It's nearly ready for trunk; I have some review comments below, mostly
nits, but a couple of other issues...
> gcc/ChangeLog:
> * doc/extend.texi: Add fd_arg, fd_arg_read and fd_arg_write under
> "Common Function Attributes" section.
As well as these additions, please can you also update doc/invoke.texi.
Specifically, please update the description of the three warnings that
are affected by these attributes so that they refer to the new
attributes, rather than just to "read" and "write", so that as well as
the docs for the attributes referring to the warnings, the docs for the
warnings refer to the attributes.
> gcc/testsuite/ChangeLog:
> * gcc.dg/analyzer/fd-5.c: New test.
> * c-c++-common/attr-fd.c: New test.
>
> Signed-off-by: Immad Mir <mirimmad@outlook.com>
[...snip...]
> /* Base diagnostic class relative to fd_state_machine. */
> class fd_diagnostic : public pending_diagnostic
> {
> -public:
> +public:
There's what looks like an accidental change here, adding a couple of
stray spaces after the trailing colon (I think); please fix, to avoid
adding noise to the git history.
[...snip...]
> + void
> + inform_filedescriptor_attribute (access_directions fd_dir)
> + {
> +
> + if (m_attr)
> + switch (fd_dir)
> + {
> + case DIRS_READ_WRITE:
> + inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
> + "argument %d of %qD must be an open file descriptor",
> + m_arg_idx + 1, m_callee_fndecl);
> + break;
> + case DIRS_WRITE:
> + inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
> + "argument %d of %qD must be a read-only file descriptor",
> + m_arg_idx + 1, m_callee_fndecl);
> + break;
> + case DIRS_READ:
> + inform (DECL_SOURCE_LOCATION (m_callee_fndecl),
> + "argument %d of %qD must be a write-only file descriptor",
> + m_arg_idx + 1, m_callee_fndecl);
> + break;
> + }
> + }
I don't like the wording of the direction-based messages; if I'm
following the code correctly, it's not so much that the argument must
be, say, a read-only file descriptor, as that the argument must be a
*readable* file descriptor.
For example in fd-5.c test_2 you have:
void g (int fd) __attribute__((fd_arg_read(1))); /* { dg-message "argument 1 of 'g' must be a read-only file descriptor" } */
void
test_2 (const char *path)
{
int fd = open (path, O_WRONLY);
if (fd != -1)
{
g (fd); /* { dg-warning "'g' on 'write-only' file descriptor 'fd'" } */
}
close (fd);
}
so presumably with the patch as posted it emits:
warning: 'g' on 'write-only' file descriptor 'fd'
note: argument 1 of 'g' must be a read-only file descriptor
whereas I think we really mean:
warning: 'g' on write-only file descriptor 'fd'
note: argument 1 of 'g' must be a readable file descriptor
Also, it will be easier for the user to understand why these warnings
are appearing if they refer to the attribute by name.
So please add something like:
"due to %<__attribute__((fd_arg(%d)))%>",
m_arg_idx + 1
to the format strings and arguments.
So the example above might read:
warning: 'g' on write-only file descriptor 'fd'
note: argument 1 of 'g' must be a readable file descriptor, due to '__attribute__((fd_arg_read(1)))'
[...snip...]
> @@ -317,29 +398,25 @@ public:
> bool
> emit (rich_location *rich_loc) final override
> {
> + bool warned;
> switch (m_fd_dir)
> {
> - case DIR_READ:
> - return warning_at (rich_loc, get_controlling_option (),
> + case DIRS_READ:
> + warned = warning_at (rich_loc, get_controlling_option (),
> "%qE on %<read-only%> file descriptor %qE",
I hadn't noticed this before, but read-only shouldn't be in quotes in
this message.
> m_callee_fndecl, m_arg);
> - case DIR_WRITE:
> - return warning_at (rich_loc, get_controlling_option (),
> + break;
> + case DIRS_WRITE:
> + warned = warning_at (rich_loc, get_controlling_option (),
> "%qE on %<write-only%> file descriptor %qE",
Likewise.
> m_callee_fndecl, m_arg);
> + break;
> 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_fd_dir == sub_other.m_fd_dir);
> + if (warned)
> + inform_filedescriptor_attribute (m_fd_dir);
> + return warned;
> }
>
> label_text
> @@ -347,10 +424,10 @@ public:
> {
> switch (m_fd_dir)
> {
> - case DIR_READ:
> + case DIRS_READ:
> return ev.formatted_print ("%qE on %<read-only%> file descriptor %qE",
> m_callee_fndecl, m_arg);
Likewise.
> - case DIR_WRITE:
> + case DIRS_WRITE:
> return ev.formatted_print ("%qE on %<write-only%> file descriptor %qE",
> m_callee_fndecl, m_arg);
Likewise.
[...snip...]
> @@ -542,7 +627,7 @@ public:
>
> private:
> diagnostic_event_id_t m_first_open_event;
> - const tree m_callee_fndecl;
> +
We can lose this extra newline.
[...snip...]
> +void
> +fd_state_machine::check_for_fd_attrs (
> + sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
> + const tree callee_fndecl, const char *attr_name,
> + access_directions fd_attr_access_dir) const
> +{
> +
> + tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
> + attrs = lookup_attribute (attr_name, attrs);
> + if (!attrs)
> + return;
> +
> + if (!TREE_VALUE (attrs))
> + return;
> +
> + auto_bitmap argmap;
> +
> + for (tree idx = TREE_VALUE (attrs); idx; idx = TREE_CHAIN (idx))
> + {
> + unsigned int val = TREE_INT_CST_LOW (TREE_VALUE (idx)) - 1;
> + bitmap_set_bit (argmap, val);
> + }
> + if (bitmap_empty_p (argmap))
> + return;
> +
> + for (unsigned i = 0; i < gimple_call_num_args (stmt); i++)
> + {
> +
> + unsigned int arg_idx = i;
I think "i" is redundant here. Just declare and use "arg_idx" directly
in the "for" loop; it's much more descriptive than just "i".
[...snip...]
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index c8d96723f4c..a04cc541f95 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
[...snip...]
> @@ -426,7 +427,7 @@ const struct attribute_spec c_common_attribute_table[] =
> { "tls_model", 1, 1, true, false, false, false,
> handle_tls_model_attribute, NULL },
> { "nonnull", 0, -1, false, true, true, false,
> - handle_nonnull_attribute, NULL },
> + handle_nonnull_attribute, NULL },
There's a stray addition of extra whitespace at the end of this line;
please fix to avoid noise in the git history.
[...snip...]
Thanks again for the patch; hope the above make sense
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Adding three new function attributes for static analysis of file descriptors
2022-07-19 18:18 ` David Malcolm
@ 2022-07-19 20:08 ` David Malcolm
0 siblings, 0 replies; 11+ messages in thread
From: David Malcolm @ 2022-07-19 20:08 UTC (permalink / raw)
To: mirimnan017, gcc-patches; +Cc: Immad Mir
On Tue, 2022-07-19 at 14:18 -0400, David Malcolm wrote:
> On Tue, 2022-07-19 at 21:36 +0530, Immad Mir wrote:
>
[...snip...]
>
>
> > +void
> > +fd_state_machine::check_for_fd_attrs (
> > + sm_context *sm_ctxt, const supernode *node, const gimple
> > *stmt,
> > + const tree callee_fndecl, const char *attr_name,
> > + access_directions fd_attr_access_dir) const
> > +{
I had another idea about the patch: we have attr_name here, and thus I
think it's available when creating the various fd_param_diagnostic
instances, so why not convert the:
bool m_attr;
in fd_param_diagnostic to:
const char *m_attr_name; // can be NULL if no attribute involved
and then, when it's non-NULL we can use it directly in the inform
message, so that the attribute name we report is the one that the user
used. I think that's clearer, both for us and for the end-user.
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Adding three new function attributes for static analysis of file descriptors
@ 2022-07-15 15:38 Immad Mir
2022-07-15 16:57 ` David Malcolm
0 siblings, 1 reply; 11+ messages in thread
From: Immad Mir @ 2022-07-15 15:38 UTC (permalink / raw)
To: gcc-patches; +Cc: dmalcolm, mirimnan017, Immad Mir
---
gcc/analyzer/sm-fd.cc | 257 ++++++++++++++++++++++++---
gcc/c-family/c-attribs.cc | 115 ++++++++++++
gcc/doc/extend.texi | 19 ++
gcc/testsuite/gcc.dg/analyzer/fd-5.c | 53 ++++++
gcc/testsuite/gcc.dg/analyzer/fd-6.c | 14 ++
5 files changed, 431 insertions(+), 27 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-6.c
diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
index 8e4300b06e2..20018dfd12b 100644
--- a/gcc/analyzer/sm-fd.cc
+++ b/gcc/analyzer/sm-fd.cc
@@ -39,10 +39,13 @@ along with GCC; see the file COPYING3. If not see
#include "analyzer/analyzer-selftests.h"
#include "tristate.h"
#include "selftest.h"
+#include "stringpool.h"
+#include "attribs.h"
#include "analyzer/call-string.h"
#include "analyzer/program-point.h"
#include "analyzer/store.h"
#include "analyzer/region-model.h"
+#include "bitmap.h"
#if ENABLE_ANALYZER
@@ -59,6 +62,13 @@ enum access_mode
WRITE_ONLY
};
+enum fd_access_direction
+{
+ DIR_READ_WRITE,
+ DIR_READ,
+ DIR_WRITE
+};
+
class fd_state_machine : public state_machine
{
public:
@@ -104,6 +114,8 @@ public:
bool is_readonly_fd_p (state_t s) const;
bool is_writeonly_fd_p (state_t s) const;
enum access_mode get_access_mode_from_flag (int flag) const;
+ void inform_filedescriptor_attribute (tree callee_fndecl, int arg,
+ fd_access_direction fd_dir) const;
/* State for a constant file descriptor (>= 0) */
state_t m_constant_fd;
@@ -146,7 +158,7 @@ private:
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;
+ enum fd_access_direction access_fn) const;
void make_valid_transitions_on_condition (sm_context *sm_ctxt,
const supernode *node,
@@ -156,6 +168,12 @@ private:
const supernode *node,
const gimple *stmt,
const svalue *lhs) const;
+ bitmap get_fd_attrs (const char *attr_name, tree callee_fndecl) const;
+ void check_for_fd_attrs (sm_context *sm_ctxt, const supernode *node,
+ const gimple *stmt, const gcall *call,
+ const tree callee_fndecl, bitmap argmap,
+ fd_access_direction fd_attr_access_dir) const;
+
};
/* Base diagnostic class relative to fd_state_machine. */
@@ -294,10 +312,10 @@ class fd_access_mode_mismatch : public fd_diagnostic
{
public:
fd_access_mode_mismatch (const fd_state_machine &sm, tree arg,
- enum access_direction fd_dir,
- const tree callee_fndecl)
+ enum fd_access_direction fd_dir,
+ const tree callee_fndecl, bool attr, int arg_idx)
: fd_diagnostic (sm, arg), m_fd_dir (fd_dir),
- m_callee_fndecl (callee_fndecl)
+ m_callee_fndecl (callee_fndecl), m_attr (attr), m_arg_idx (arg_idx)
{
}
@@ -317,19 +335,27 @@ public:
bool
emit (rich_location *rich_loc) final override
{
+ bool warned;
switch (m_fd_dir)
{
case DIR_READ:
- return warning_at (rich_loc, get_controlling_option (),
+ warned = warning_at (rich_loc, get_controlling_option (),
"%qE on %<read-only%> file descriptor %qE",
m_callee_fndecl, m_arg);
+ break;
case DIR_WRITE:
- return warning_at (rich_loc, get_controlling_option (),
+ warned = warning_at (rich_loc, get_controlling_option (),
"%qE on %<write-only%> file descriptor %qE",
m_callee_fndecl, m_arg);
+ break;
default:
gcc_unreachable ();
}
+ if (warned && m_attr)
+ {
+ m_sm.inform_filedescriptor_attribute (m_callee_fndecl, m_arg_idx, m_fd_dir);
+ }
+ return warned;
}
bool
@@ -359,8 +385,10 @@ public:
}
private:
- enum access_direction m_fd_dir;
+ enum fd_access_direction m_fd_dir;
const tree m_callee_fndecl;
+ bool m_attr;
+ int m_arg_idx;
};
class double_close : public fd_diagnostic
@@ -422,8 +450,9 @@ class fd_use_after_close : public fd_diagnostic
{
public:
fd_use_after_close (const fd_state_machine &sm, tree arg,
- const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ const tree callee_fndecl, bool attr, int arg_idx)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl), m_attr (attr),
+ m_arg_idx (arg_idx)
{
}
@@ -439,12 +468,27 @@ public:
return OPT_Wanalyzer_fd_use_after_close;
}
+ bool
+ subclass_equal_p (const pending_diagnostic &base_other) const override
+ {
+ const fd_use_after_close &sub_other
+ = (const fd_use_after_close &)base_other;
+ return (same_tree_p (m_arg, sub_other.m_arg)
+ && m_callee_fndecl == sub_other.m_callee_fndecl
+ && m_attr == sub_other.m_attr
+ && m_arg_idx == sub_other.m_arg_idx);
+ }
+
bool
emit (rich_location *rich_loc) final override
{
- return warning_at (rich_loc, get_controlling_option (),
+ bool warned;
+ warned = warning_at (rich_loc, get_controlling_option (),
"%qE on closed file descriptor %qE", m_callee_fndecl,
m_arg);
+ if (warned && m_attr)
+ m_sm.inform_filedescriptor_attribute (m_callee_fndecl, m_arg_idx, DIR_READ_WRITE);
+ return warned;
}
label_text
@@ -466,25 +510,29 @@ public:
describe_final_event (const evdesc::final_event &ev) final override
{
if (m_first_close_event.known_p ())
- return ev.formatted_print (
- "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
- m_arg, "close", &m_first_close_event);
- else
- return ev.formatted_print ("%qE on closed file descriptor %qE",
- m_callee_fndecl, m_arg);
+ return ev.formatted_print (
+ "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
+ m_arg, "close", &m_first_close_event);
+ else
+ return ev.formatted_print ("%qE on closed file descriptor %qE",
+ m_callee_fndecl, m_arg);
}
private:
diagnostic_event_id_t m_first_close_event;
const tree m_callee_fndecl;
+ bool m_attr;
+ int m_arg_idx;
};
class unchecked_use_of_fd : public fd_diagnostic
{
public:
unchecked_use_of_fd (const fd_state_machine &sm, tree arg,
- const tree callee_fndecl)
- : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl)
+ const tree callee_fndecl, bool attr,
+ int arg_idx)
+ : fd_diagnostic (sm, arg), m_callee_fndecl (callee_fndecl),
+ m_attr (attr), m_arg_idx (arg_idx)
{
}
@@ -503,9 +551,13 @@ public:
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 warned;
+ warned = warning_at (rich_loc, get_controlling_option (),
+ "%qE on possibly invalid file descriptor %qE",
+ m_callee_fndecl, m_arg);
+ if (warned && m_attr)
+ m_sm.inform_filedescriptor_attribute (m_callee_fndecl, m_arg_idx,
+ DIR_READ_WRITE);
}
bool
@@ -514,7 +566,9 @@ public:
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);
+ && m_callee_fndecl == sub_other.m_callee_fndecl
+ && m_attr == sub_other.m_attr
+ && m_arg_idx == sub_other.m_arg_idx);
}
label_text
@@ -543,8 +597,40 @@ public:
private:
diagnostic_event_id_t m_first_open_event;
const tree m_callee_fndecl;
+ bool m_attr;
+ int m_arg_idx;
+
};
+/* Issue a note regarding misuse of a file descriptor.
+ ARG_IDX here is 0-based.
+ */
+void
+fd_state_machine::inform_filedescriptor_attribute (
+ tree callee_fndecl, int arg_idx, fd_access_direction fd_dir) const
+{
+ switch (fd_dir)
+ {
+ case READ_WRITE:
+ inform (DECL_SOURCE_LOCATION (callee_fndecl),
+ "argument %d of %qD must be an open file descriptor", arg_idx + 1,
+ callee_fndecl);
+ break;
+ case DIR_WRITE:
+ inform (DECL_SOURCE_LOCATION (callee_fndecl),
+ "argument %d of %qD must be a read-only file descriptor",
+ arg_idx + 1, callee_fndecl);
+ break;
+ case DIR_READ:
+ inform (DECL_SOURCE_LOCATION (callee_fndecl),
+ "argument %d of %qD must be a write-only file descriptor",
+ arg_idx + 1, callee_fndecl);
+ break;
+ }
+}
+
+
+
fd_state_machine::fd_state_machine (logger *logger)
: state_machine ("file-descriptor", logger),
m_constant_fd (add_state ("fd-constant")),
@@ -647,11 +733,126 @@ fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
on_read (sm_ctxt, node, stmt, call, callee_fndecl);
return true;
} // "read"
+
+
+ {
+ // Handle __attribute__((fd_arg))
+ bitmap argmap = get_fd_attrs ("fd_arg", callee_fndecl);
+ if (argmap)
+ check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl, argmap, DIR_READ_WRITE);
+
+ // Handle __attribute__((fd_arg_read))
+ bitmap read_argmap = get_fd_attrs ("fd_arg_read", callee_fndecl);
+ if(read_argmap)
+ check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl, read_argmap, DIR_READ);
+
+ // Handle __attribute__((fd_arg_write))
+ bitmap write_argmap = get_fd_attrs ("fd_arg_write", callee_fndecl);
+ if (write_argmap)
+ check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl, write_argmap, DIR_WRITE);
+
+ return true;
+ }
+
}
return false;
}
+void
+fd_state_machine::check_for_fd_attrs (sm_context *sm_ctxt,
+ const supernode *node, const gimple *stmt,
+ const gcall *call,
+ const tree callee_fndecl, bitmap argmap,
+ fd_access_direction fd_attr_access_dir) const
+{
+ for (unsigned i = 0; i < gimple_call_num_args (stmt); i++)
+ {
+ unsigned int arg_idx = i;
+ tree arg = gimple_call_arg (stmt, i);
+ tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
+ state_t state = sm_ctxt->get_state (stmt, arg);
+ if (TREE_CODE (TREE_TYPE (arg)) != INTEGER_TYPE)
+ continue;
+ if (bitmap_bit_p (argmap, arg_idx)) // check if the arg is any of the file
+ // descriptor attributes
+ {
+
+ if (is_closed_fd_p (state))
+ {
+
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_use_after_close (*this, diag_arg,
+ callee_fndecl, true,
+ arg_idx));
+ continue;
+ }
+ else
+ {
+ if (!(is_valid_fd_p (state) || (state == m_stop)))
+ {
+ if (!is_constant_fd_p (state))
+ sm_ctxt->warn (node, stmt, arg,
+ new unchecked_use_of_fd (*this, diag_arg,
+ callee_fndecl, true,
+ arg_idx));
+ }
+ }
+ }
+ switch (fd_attr_access_dir)
+ {
+ case DIR_READ:
+ if (bitmap_bit_p (argmap, arg_idx)) // check if the arg is marked by
+ // reads_from_fd attribute
+ {
+ if (is_writeonly_fd_p (state))
+ {
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_access_mode_mismatch (*this, diag_arg,
+ DIR_WRITE,
+ callee_fndecl, true, i));
+ }
+ }
+ break;
+ case DIR_WRITE:
+ if (bitmap_bit_p (argmap, arg_idx)) // check if the arg is marked by
+ // writes_to_fd attribute
+ {
+ if (is_readonly_fd_p (state))
+ {
+ sm_ctxt->warn (node, stmt, arg,
+ new fd_access_mode_mismatch (
+ *this, diag_arg, DIR_READ, callee_fndecl, true, i));
+ }
+ }
+ break;
+ }
+ }
+}
+
+bitmap
+fd_state_machine::get_fd_attrs (const char *attr_name, tree callee_fndecl) const
+{
+ bitmap argmap = NULL;
+ tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
+ attrs = lookup_attribute (attr_name, attrs);
+ if (!attrs)
+ return argmap;
+
+ if (!TREE_VALUE (attrs))
+ return argmap;
+
+ argmap = BITMAP_ALLOC (NULL);
+
+ for (tree idx = TREE_VALUE (attrs); idx; idx = TREE_CHAIN (idx))
+ {
+ unsigned int val = TREE_INT_CST_LOW (TREE_VALUE (idx)) - 1;
+ bitmap_set_bit (argmap, val);
+ }
+ return argmap;
+}
+
+
void
fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
const gimple *stmt, const gcall *call) const
@@ -729,7 +930,7 @@ 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 callee_fndecl_dir) const
+ enum fd_access_direction callee_fndecl_dir) const
{
tree arg = gimple_call_arg (call, 0);
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
@@ -738,7 +939,7 @@ fd_state_machine::check_for_open_fd (
if (is_closed_fd_p (state))
{
sm_ctxt->warn (node, stmt, arg,
- new fd_use_after_close (*this, diag_arg, callee_fndecl));
+ new fd_use_after_close (*this, diag_arg, callee_fndecl, false, -1));
}
else
@@ -748,7 +949,7 @@ fd_state_machine::check_for_open_fd (
if (!is_constant_fd_p (state))
sm_ctxt->warn (
node, stmt, arg,
- new unchecked_use_of_fd (*this, diag_arg, callee_fndecl));
+ new unchecked_use_of_fd (*this, diag_arg, callee_fndecl, false, -1));
}
switch (callee_fndecl_dir)
{
@@ -758,7 +959,7 @@ fd_state_machine::check_for_open_fd (
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
sm_ctxt->warn (node, stmt, arg,
new fd_access_mode_mismatch (
- *this, diag_arg, DIR_WRITE, callee_fndecl));
+ *this, diag_arg, DIR_WRITE, callee_fndecl, false, -1));
}
break;
@@ -769,9 +970,11 @@ fd_state_machine::check_for_open_fd (
tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
sm_ctxt->warn (node, stmt, arg,
new fd_access_mode_mismatch (
- *this, diag_arg, DIR_READ, callee_fndecl));
+ *this, diag_arg, DIR_READ, callee_fndecl, false, -1));
}
break;
+ default:
+ gcc_unreachable ();
}
}
}
diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index c8d96723f4c..1a8f91d6222 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -173,6 +173,10 @@ static tree handle_objc_nullability_attribute (tree *, tree, tree, int, bool *);
static tree handle_signed_bool_precision_attribute (tree *, tree, tree, int,
bool *);
static tree handle_retain_attribute (tree *, tree, tree, int, bool *);
+static tree handle_fd_arg_attribute (tree *, tree, tree, int, bool *);
+static tree handle_fd_arg_read_attribute (tree *, tree, tree, int, bool *);
+static tree handle_fd_arg_write_attribute (tree *, tree, tree, int, bool *);
+
/* Helper to define attribute exclusions. */
#define ATTR_EXCL(name, function, type, variable) \
@@ -427,6 +431,12 @@ const struct attribute_spec c_common_attribute_table[] =
handle_tls_model_attribute, NULL },
{ "nonnull", 0, -1, false, true, true, false,
handle_nonnull_attribute, NULL },
+ { "fd_arg", 0, -1, false, true, true, false,
+ handle_fd_arg_attribute, NULL},
+ { "fd_arg_read", 0, -1, false, true, true, false,
+ handle_fd_arg_read_attribute, NULL},
+ { "fd_arg_write", 0, -1, false, true, true, false,
+ handle_fd_arg_write_attribute, NULL},
{ "nonstring", 0, 0, true, false, false, false,
handle_nonstring_attribute, NULL },
{ "nothrow", 0, 0, true, false, false, false,
@@ -4521,6 +4531,111 @@ handle_nonnull_attribute (tree *node, tree name,
return NULL_TREE;
}
+/* Handle the "fd_arg" attribute */
+
+static tree
+handle_fd_arg_attribute (tree *node, tree name, tree args,
+ int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+ tree type = *node;
+ if (!args)
+ {
+ if (!prototype_p (type))
+ {
+ error ("%qE attribute without arguments on a non-prototype", name);
+ *no_add_attrs = true;
+ }
+ return NULL_TREE;
+ }
+
+ for (int i = 1; args; ++i)
+ {
+ tree pos = TREE_VALUE (args);
+ tree next = TREE_CHAIN (args);
+ if (tree val = positional_argument (type, name, pos, INTEGER_TYPE,
+ next || i > 1 ? i : 0))
+ TREE_VALUE (args) = val;
+ else
+ {
+ *no_add_attrs = true;
+ break;
+ }
+ args = next;
+ }
+ return NULL_TREE;
+}
+
+/* Handle the "fd_arg_read" attribute. */
+
+static tree
+handle_fd_arg_read_attribute (tree *node, tree name, tree ARG_UNUSED (args),
+ int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+ tree type = *node;
+ if (!args)
+ {
+ if (!prototype_p (type))
+ {
+ error ("%qE attribute without arguments on a non-prototype", name);
+ *no_add_attrs = true;
+ }
+ return NULL_TREE;
+ }
+
+ for (int i = 1; args; ++i)
+ {
+ tree pos = TREE_VALUE (args);
+ tree next = TREE_CHAIN (args);
+ if (tree val = positional_argument (type, name, pos, INTEGER_TYPE,
+ next || i > 1 ? i : 0))
+ TREE_VALUE (args) = val;
+ else
+ {
+ *no_add_attrs = true;
+ break;
+ }
+ args = next;
+ }
+ return NULL_TREE;
+}
+
+
+/* Handle the "fd_arg_write" attribute. */
+static tree
+handle_fd_arg_write_attribute (tree *node, tree name, tree ARG_UNUSED (args),
+ int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+ tree type = *node;
+ if (!args)
+ {
+ if (!prototype_p (type))
+ {
+ error ("%qE attribute without arguments on a non-prototype", name);
+ *no_add_attrs = true;
+ }
+ return NULL_TREE;
+ }
+
+ for (int i = 1; args; ++i)
+ {
+ tree pos = TREE_VALUE (args);
+ tree next = TREE_CHAIN (args);
+ if (tree val = positional_argument (type, name, pos, INTEGER_TYPE,
+ next || i > 1 ? i : 0))
+ TREE_VALUE (args) = val;
+ else
+ {
+ *no_add_attrs = true;
+ break;
+ }
+ args = next;
+ }
+ return NULL_TREE;
+}
+
+
+
+
/* Handle the "nonstring" variable attribute. */
static tree
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index dfbe33ac652..eba86e9f7ef 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -3007,6 +3007,25 @@ produced by @command{gold}.
For other linkers that cannot generate resolution file,
explicit @code{externally_visible} attributes are still necessary.
+@item fd_arg (@Var{N}, @Var{...})
+
+The @Var{fd_arg} attribute may be applied to a function that takes an open file
+descriptor at referenced argument @Var{N}. It indicates that the passed file
+descriptor must not have been closed.
+
+@item fd_arg_read (@Var{N}, @Var{...})
+
+The @Var{fd_arg_read} attribute may be applied to function that takes an open file
+descriptor at referenced argument @Var{N} that it might read from. It
+indicates that the passed file descriptor must not have been closed or
+not opened as write-only.
+
+@item fd_arg_write (@Var{N}, @Var{...})
+
+The @Var{fd_arg_write} attribute may be applied to a function that takes an open
+file descriptor at referenced argument @Var{N} that it might write to. It indicates
+that the passed file descriptor must not have been closed or not opened as read-only.
+
@item flatten
@cindex @code{flatten} function attribute
Generally, inlining into a function is limited. For a function marked with
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-5.c b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
new file mode 100644
index 00000000000..6287ddd38fc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
@@ -0,0 +1,53 @@
+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 f (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'f' must be an open file descriptor" } */
+
+void
+test_1 (const char *path)
+{
+ int fd = open (path, O_RDWR);
+ close(fd);
+ f(fd); /* { dg-warning "'f' on closed file descriptor 'fd'" } */
+ /* { dg-message "\\(3\\) 'f' on closed file descriptor 'fd'; 'close' was at \\(2\\)" "" { target *-*-* } .-1 } */
+}
+
+void g (int fd) __attribute__((fd_arg_read(1))); /* { dg-message "argument 1 of 'g' must be a read-only file descriptor" } */
+
+void
+test_2 (const char *path)
+{
+ int fd = open (path, O_WRONLY);
+ if (fd != -1)
+ {
+ g (fd); /* { dg-warning "'g' on 'write-only' file descriptor 'fd'" } */
+ }
+ close (fd);
+}
+
+void h (int fd) __attribute__((fd_arg_write(1))); /* { dg-message "argument 1 of 'h' must be a write-only file descriptor" } */
+void
+test_3 (const char *path)
+{
+ int fd = open (path, O_RDONLY);
+ if (fd != -1)
+ {
+ h (fd); /* { dg-warning "'h' on 'read-only' file descriptor 'fd'" } */
+ }
+ close(fd);
+}
+
+void ff (int fd) __attribute__((fd_arg(1))); /* { dg-message "argument 1 of 'ff' must be an open file descriptor" } */
+
+void test_4 (const char *path)
+{
+ int fd = open (path, O_RDWR);
+ ff (fd); /* { dg-warning "'ff' on possibly invalid file descriptor 'fd'" } */
+ close(fd);
+}
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-6.c b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
new file mode 100644
index 00000000000..7efb9e69b58
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
@@ -0,0 +1,14 @@
+
+int not_a_fn __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute only applies to function types" } */
+
+void f (char *p) __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute argument value '1' refers to parameter type 'char \\\*'" } */
+
+
+int not_a_fn_b __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute only applies to function types" } */
+
+void g (char *p) __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute argument value '1' refers to parameter type 'char \\\*'" } */
+
+
+int not_a_fn_c __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute only applies to function types" } */
+
+void f (char *p) __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute argument value '1' refers to parameter type 'char \\\*'" } */
\ No newline at end of file
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] Adding three new function attributes for static analysis of file descriptors
2022-07-15 15:38 Immad Mir
@ 2022-07-15 16:57 ` David Malcolm
0 siblings, 0 replies; 11+ messages in thread
From: David Malcolm @ 2022-07-15 16:57 UTC (permalink / raw)
To: mirimnan017, gcc-patches; +Cc: Immad Mir
On Fri, 2022-07-15 at 21:08 +0530, Immad Mir wrote:
Thanks for the patch.
Various review comments:
The patch is missing a ChangeLog.
> ---
> gcc/analyzer/sm-fd.cc | 257 ++++++++++++++++++++++++---
> gcc/c-family/c-attribs.cc | 115 ++++++++++++
> gcc/doc/extend.texi | 19 ++
> gcc/testsuite/gcc.dg/analyzer/fd-5.c | 53 ++++++
> gcc/testsuite/gcc.dg/analyzer/fd-6.c | 14 ++
> 5 files changed, 431 insertions(+), 27 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-5.c
> create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-6.c
>
> diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
> index 8e4300b06e2..20018dfd12b 100644
> --- a/gcc/analyzer/sm-fd.cc
> +++ b/gcc/analyzer/sm-fd.cc
[...snip...]
> +enum fd_access_direction
> +{
> + DIR_READ_WRITE,
> + DIR_READ,
> + DIR_WRITE
> +};
Don't we already have DIR_READ and DIR_WRITE in enum access_direction
in analyzer.h? I wonder why this isn't an error due to the naming
collision?
The new enum refers to a *set* of valid access directions, so maybe
rename the new enum to "enum access_directions" (note the plural), and
rename the elements from DIR_ to DIRS_. Please carefully check all
usage of DIR_* in sm-fd.cc.
Maybe instead we should convert the values in enum access_direction
from indexes into bitfield masks.
[...snip...]
> @@ -317,19 +335,27 @@ public:
> bool
> emit (rich_location *rich_loc) final override
> {
> + bool warned;
> switch (m_fd_dir)
> {
> case DIR_READ:
> - return warning_at (rich_loc, get_controlling_option (),
> + warned = warning_at (rich_loc, get_controlling_option (),
> "%qE on %<read-only%> file descriptor %qE",
> m_callee_fndecl, m_arg);
> + break;
> case DIR_WRITE:
> - return warning_at (rich_loc, get_controlling_option (),
> + warned = warning_at (rich_loc, get_controlling_option (),
> "%qE on %<write-only%> file descriptor %qE",
> m_callee_fndecl, m_arg);
> + break;
...i.e. which DIR_READ/DIR_WRITE values are these cases using?
> default:
> gcc_unreachable ();
> }
> + if (warned && m_attr)
> + {
> + m_sm.inform_filedescriptor_attribute (m_callee_fndecl, m_arg_idx, m_fd_dir);
> + }
Redundant braces here ^^^
> + return warned;
> }
>
> bool
> @@ -359,8 +385,10 @@ public:
> }
>
> private:
> - enum access_direction m_fd_dir;
> + enum fd_access_direction m_fd_dir;
> const tree m_callee_fndecl;
> + bool m_attr;
> + int m_arg_idx;
> };
Most (all?) of the concrete subclasses seem to be adding the two fields
m_attr and m_arg_idx, so can you add them to class fd_diagnostic, or
introduce a common subclass, rather than repeating them in each
subclass?
I suspect the code would be simpler if inform_filedescriptor_attribute
were a method of fd_diagnostic, rather than of the state_machine: you
could move the (&& m_attr) part of the conditional into there, rather
than having every emit vfunc do the same test, and having to pass the
same fields to it each time (except perhaps the access dir?).
[...snip...]
> @@ -466,25 +510,29 @@ public:
> describe_final_event (const evdesc::final_event &ev) final override
> {
> if (m_first_close_event.known_p ())
> - return ev.formatted_print (
> - "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
> - m_arg, "close", &m_first_close_event);
> - else
> - return ev.formatted_print ("%qE on closed file descriptor %qE",
> - m_callee_fndecl, m_arg);
> + return ev.formatted_print (
> + "%qE on closed file descriptor %qE; %qs was at %@", m_callee_fndecl,
> + m_arg, "close", &m_first_close_event);
> + else
> + return ev.formatted_print ("%qE on closed file descriptor %qE",
> + m_callee_fndecl, m_arg);
What changed here? Is this just whitespace changes?
[...snip...]
> fd_state_machine::fd_state_machine (logger *logger)
> : state_machine ("file-descriptor", logger),
> m_constant_fd (add_state ("fd-constant")),
> @@ -647,11 +733,126 @@ fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
> on_read (sm_ctxt, node, stmt, call, callee_fndecl);
> return true;
> } // "read"
> +
> +
> + {
> + // Handle __attribute__((fd_arg))
> + bitmap argmap = get_fd_attrs ("fd_arg", callee_fndecl);
> + if (argmap)
> + check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl, argmap, DIR_READ_WRITE);
> +
> + // Handle __attribute__((fd_arg_read))
> + bitmap read_argmap = get_fd_attrs ("fd_arg_read", callee_fndecl);
> + if(read_argmap)
> + check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl, read_argmap, DIR_READ);
> +
> + // Handle __attribute__((fd_arg_write))
> + bitmap write_argmap = get_fd_attrs ("fd_arg_write", callee_fndecl);
> + if (write_argmap)
> + check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl, write_argmap, DIR_WRITE);
I think there are three memory leaks here, anytime the bitmap is non-
null you need a
BITMAP_FREE (foo_argmap);
on it.
Rather than repeating that logic three times, it's probably much
cleaner to combine get_fd_attrs and check_for_fd_attrs into a single
function that:
- builds the bitmap
- bails out early for the common case where there's no attribute
- use the bitmap
- frees it, or, better, uses auto_bitmap rather than bitmap so that
the cleanup happens implicitly
Then the above logic could read:
handle_any_fd_attrs (callee_fndecl, sm_ctxt, node, stmt, call,
"fd_arg", DIR_READ_WRITE);
handle_any_fd_attrs (callee_fndecl, sm_ctxt, node, stmt, call,
"fd_arg_read", DIR_READ);
handle_any_fd_attrs (callee_fndecl, sm_ctxt, node, stmt, call,
"fd_arg_write", DIR_WRITE);
or similar.
> +
> + return true;
> + }
> +
> }
>
> return false;
> }
[...snip...]
> +bitmap
> +fd_state_machine::get_fd_attrs (const char *attr_name, tree callee_fndecl) const
> +{
> + bitmap argmap = NULL;
> + tree attrs = TYPE_ATTRIBUTES (TREE_TYPE (callee_fndecl));
> + attrs = lookup_attribute (attr_name, attrs);
> + if (!attrs)
> + return argmap;
> +
> + if (!TREE_VALUE (attrs))
> + return argmap;
> +
> + argmap = BITMAP_ALLOC (NULL);
> +
> + for (tree idx = TREE_VALUE (attrs); idx; idx = TREE_CHAIN (idx))
> + {
> + unsigned int val = TREE_INT_CST_LOW (TREE_VALUE (idx)) - 1;
We need to be sure that the attribute-validation logic in c-attribs.cc
rejects argument numbers that aren't >= 1. I think the above will
crash if the user supplies an argument number that's 0, negative, or
not an integer. I *think* positional_argument checks for all this and
will reject such attributes, but let's have coverage for each of these
in the test suite.
> + bitmap_set_bit (argmap, val);
> + }
> + return argmap;
> +}
> +
> +
> void
> fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
> const gimple *stmt, const gcall *call) const
> @@ -729,7 +930,7 @@ 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 callee_fndecl_dir) const
> + enum fd_access_direction callee_fndecl_dir) const
> {
> tree arg = gimple_call_arg (call, 0);
> tree diag_arg = sm_ctxt->get_diagnostic_tree (arg);
> @@ -738,7 +939,7 @@ fd_state_machine::check_for_open_fd (
> if (is_closed_fd_p (state))
> {
> sm_ctxt->warn (node, stmt, arg,
> - new fd_use_after_close (*this, diag_arg, callee_fndecl));
> + new fd_use_after_close (*this, diag_arg, callee_fndecl, false, -1));
I'd prefer overloaded constructors for these fd_diagnostic subclasses,
one for the for the hardcoded case, another for the attribute case, so
that the -1 for the "this is meaningless" attribute argno is only in
the fd_diagnostic constructor.
[...snip...]
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
[...snip...]
> +/* Handle the "fd_arg" attribute */
> +
> +static tree
> +handle_fd_arg_attribute (tree *node, tree name, tree args,
> + int ARG_UNUSED (flags), bool *no_add_attrs)
> +{
Am I right in thinking that all three of the handle_fd_arg*_attribute
callback functions are identical? If so, let's just have a single
callback shared by all three entries in the array.
> + tree type = *node;
> + if (!args)
> + {
> + if (!prototype_p (type))
> + {
> + error ("%qE attribute without arguments on a non-prototype", name);
> + *no_add_attrs = true;
> + }
> + return NULL_TREE;
> + }
> +
> + for (int i = 1; args; ++i)
> + {
> + tree pos = TREE_VALUE (args);
> + tree next = TREE_CHAIN (args);
> + if (tree val = positional_argument (type, name, pos, INTEGER_TYPE,
> + next || i > 1 ? i : 0))
> + TREE_VALUE (args) = val;
> + else
> + {
> + *no_add_attrs = true;
> + break;
> + }
> + args = next;
> + }
Looks like you've copied and pasted the above loop from
handle_alloc_size_attribute, which can take multiple args.
The new attributes only take a single argument, so I think
handle_alloc_align_attribute is a better model.
> + return NULL_TREE;
> +}
[...snip...]
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index dfbe33ac652..eba86e9f7ef 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3007,6 +3007,25 @@ produced by @command{gold}.
> For other linkers that cannot generate resolution file,
> explicit @code{externally_visible} attributes are still necessary.
>
> +@item fd_arg (@Var{N}, @Var{...})
The @var directives should have lower-case "v".
You should add a @cindex line below the @item for each of these; see
the existing function attributes for how to do that.
> +
> +The @Var{fd_arg} attribute may be applied to a function that takes an open file
@Var should be @code here.
> +descriptor at referenced argument @Var{N}. It indicates that the passed file
> +descriptor must not have been closed.
Please spell out which analyzer warnings are affected by this
attribute.
> +
> +@item fd_arg_read (@Var{N}, @Var{...})
Aha: you're supporting 1 or more multiple arguments, rather than just
1?
I don't think we need this, almost every API I can think of takes a
single FD (or is a special-case, like dup2).
So for simplicity, let's just support a single argument per attribute;
if there's a rare case where an API takes two fd_arg_read, the user can
simply supply the attribute twice.
> +
> +The @Var{fd_arg_read} attribute may be applied to function that takes an open file
> +descriptor at referenced argument @Var{N} that it might read from. It
> +indicates that the passed file descriptor must not have been closed or
> +not opened as write-only.
It's probably simplest to document this as: "This attribute is
identical to @code{fd_arg}, but with the additional requirement
that..." or somesuch.
> +
> +@item fd_arg_write (@Var{N}, @Var{...})
> +
> +The @Var{fd_arg_write} attribute may be applied to a function that takes an open
> +file descriptor at referenced argument @Var{N} that it might write to. It indicates
> +that the passed file descriptor must not have been closed or not opened as read-only.
Likewise.
[...snip...]
> diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-6.c b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
> new file mode 100644
> index 00000000000..7efb9e69b58
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
> @@ -0,0 +1,14 @@
> +
> +int not_a_fn __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute only applies to function types" } */
> +
> +void f (char *p) __attribute__ ((fd_arg(1))); /* { dg-warning "'fd_arg' attribute argument value '1' refers to parameter type 'char \\\*'" } */
> +
> +
> +int not_a_fn_b __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute only applies to function types" } */
> +
> +void g (char *p) __attribute__ ((fd_arg_read(1))); /* { dg-warning "'fd_arg_read' attribute argument value '1' refers to parameter type 'char \\\*'" } */
> +
> +
> +int not_a_fn_c __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute only applies to function types" } */
> +
> +void f (char *p) __attribute__ ((fd_arg_write(1))); /* { dg-warning "'fd_arg_write' attribute argument value '1' refers to parameter type 'char \\\*'" } */
> \ No newline at end of file
The attribute-parsing code isn't analyzer-specific, so move this test
file to c-c++-common/attr-fd.c
Hope the above makes sense; thanks again for the patch
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-07-22 18:27 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-22 15:55 [PATCH] Adding three new function attributes for static analysis of file descriptors Immad Mir
2022-07-22 18:27 ` David Malcolm
-- strict thread matches above, loose matches on Subject: below --
2022-07-20 17:59 Immad Mir
2022-07-20 18:23 ` David Malcolm
2022-07-20 18:28 ` Prathamesh Kulkarni
2022-07-20 18:39 ` Mir Immad
2022-07-19 16:06 Immad Mir
2022-07-19 18:18 ` David Malcolm
2022-07-19 20:08 ` David Malcolm
2022-07-15 15:38 Immad Mir
2022-07-15 16:57 ` 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).