* [PATCH] filedescriptor attribute
@ 2022-07-12 17:31 Immad Mir
2022-07-12 17:33 ` Mir Immad
0 siblings, 1 reply; 16+ messages in thread
From: Immad Mir @ 2022-07-12 17:31 UTC (permalink / raw)
To: gcc; +Cc: dmalcolm, mirimnan017, Immad Mir
---
gcc/analyzer/sm-fd.cc | 231 ++++++++++++++++++++++++---
gcc/c-family/c-attribs.cc | 63 ++++++++
gcc/testsuite/gcc.dg/analyzer/fd-5.c | 44 +++++
gcc/testsuite/gcc.dg/analyzer/fd-6.c | 4 +
4 files changed, 322 insertions(+), 20 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..bdc807160cc 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,17 +510,19 @@ 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
@@ -543,8 +589,38 @@ public:
private:
diagnostic_event_id_t m_first_open_event;
const tree m_callee_fndecl;
+
};
+/* 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 +723,124 @@ 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__((filedescriptor))
+ {
+ bitmap argmap = get_fd_attrs ("accesses_fd", callee_fndecl);
+ if (argmap)
+ check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl, argmap, DIR_READ_WRITE);
+
+ bitmap read_argmap = get_fd_attrs ("reads_from_fd", callee_fndecl);
+ if(read_argmap)
+ check_for_fd_attrs (sm_ctxt, node, stmt, call, callee_fndecl, read_argmap, DIR_READ);
+
+ bitmap write_argmap = get_fd_attrs ("writes_to_fd", 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));
+ }
+ }
+ }
+ 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;
+ default:
+ gcc_unreachable ();
+ }
+ }
+}
+
+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 +918,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 +927,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
@@ -758,7 +947,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 +958,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..84f29987bc6 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_accesses_fd_attribute (tree *, tree, tree, int, bool *);
+static tree handle_reads_from_fd_attribute (tree *, tree, tree, int, bool *);
+static tree handle_writes_to_fd_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 },
+ { "accesses_fd", 0, -1, false, true, true, false,
+ handle_accesses_fd_attribute, NULL},
+ { "reads_from_fd", 0, -1, false, true, true, false,
+ handle_reads_from_fd_attribute, NULL},
+ { "writes_to_fd", 0, -1, false, true, true, false,
+ handle_writes_to_fd_attribute, NULL},
{ "nonstring", 0, 0, true, false, false, false,
handle_nonstring_attribute, NULL },
{ "nothrow", 0, 0, true, false, false, false,
@@ -4521,6 +4531,59 @@ handle_nonnull_attribute (tree *node, tree name,
return NULL_TREE;
}
+/* Handle the "accesses_fd" attribute */
+static tree
+handle_accesses_fd_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;
+ }
+}
+
+
+
+static tree
+handle_reads_from_fd_attribute (tree *node, tree name, tree ARG_UNUSED (args),
+ int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+ // TODO: valiadate usage of this attribute
+ return NULL_TREE;
+}
+
+static tree
+handle_writes_to_fd_attribute (tree *node, tree name, tree ARG_UNUSED (args),
+ int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+ // TODO: validate usage of this attribute
+ return NULL_TREE;
+}
+
+
+
+
/* Handle the "nonstring" variable attribute. */
static tree
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..e597ba2367a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
@@ -0,0 +1,44 @@
+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__((accesses_fd(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__((reads_from_fd(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__((writes_to_fd(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);
+}
\ 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..9c0b55086a9
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
@@ -0,0 +1,4 @@
+
+int not_a_fn __attribute__ ((accesses_fd(1))); /* { dg-warning "'accesses_fd' attribute only applies to function types" } */
+
+void f (char *p) __attribute__ ((accesses_fd(1))); /* { dg-warning "'accesses_fd' attribute argument value '1' refers to parameter type 'char \\\*'" } */
\ No newline at end of file
--
2.25.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] filedescriptor attribute
2022-07-12 17:31 [PATCH] filedescriptor attribute Immad Mir
@ 2022-07-12 17:33 ` Mir Immad
2022-07-12 22:16 ` Adding file descriptor attribute(s) to gcc and glibc David Malcolm
0 siblings, 1 reply; 16+ messages in thread
From: Mir Immad @ 2022-07-12 17:33 UTC (permalink / raw)
Cc: gcc, David Malcolm
Hi everyone,
This is an in-progress patch for adding three new function attributes to
GCC for static analysis of file descriptor APIs (which is a part of my GSoC
project)
1) __attribute__((accesses_fd(N)))
This attribute expected argument N of a function to be an open file
descriptor. (see test_1 of fd-5.c)
2) __attribute__((reads_from_fd(N)))
This attribute expects arguments N of a function to not be a write-only FD.
(see test_2 of fd-5.c)
3) __attribute__((writes_to_fd(N)))
This attribute expects arguments N of a function to not be a read-only FD.
(see test_3 of fd-6.c)
Thanks.
Immad
On Tue, Jul 12, 2022 at 11:02 PM Immad Mir <mirimmad@outlook.com> wrote:
> ---
> gcc/analyzer/sm-fd.cc | 231 ++++++++++++++++++++++++---
> gcc/c-family/c-attribs.cc | 63 ++++++++
> gcc/testsuite/gcc.dg/analyzer/fd-5.c | 44 +++++
> gcc/testsuite/gcc.dg/analyzer/fd-6.c | 4 +
> 4 files changed, 322 insertions(+), 20 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..bdc807160cc 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,17 +510,19 @@ 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
> @@ -543,8 +589,38 @@ public:
> private:
> diagnostic_event_id_t m_first_open_event;
> const tree m_callee_fndecl;
> +
> };
>
> +/* 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 +723,124 @@ 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__((filedescriptor))
> + {
> + bitmap argmap = get_fd_attrs ("accesses_fd", callee_fndecl);
> + if (argmap)
> + check_for_fd_attrs (sm_ctxt, node, stmt, call,
> callee_fndecl, argmap, DIR_READ_WRITE);
> +
> + bitmap read_argmap = get_fd_attrs ("reads_from_fd",
> callee_fndecl);
> + if(read_argmap)
> + check_for_fd_attrs (sm_ctxt, node, stmt, call,
> callee_fndecl, read_argmap, DIR_READ);
> +
> + bitmap write_argmap = get_fd_attrs ("writes_to_fd",
> 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));
> + }
> + }
> + }
> + 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;
> + default:
> + gcc_unreachable ();
> + }
> + }
> +}
> +
> +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 +918,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 +927,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
> @@ -758,7 +947,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 +958,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..84f29987bc6 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_accesses_fd_attribute (tree *, tree, tree, int, bool
> *);
> +static tree handle_reads_from_fd_attribute (tree *, tree, tree, int, bool
> *);
> +static tree handle_writes_to_fd_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 },
> + { "accesses_fd", 0, -1, false, true, true, false,
> + handle_accesses_fd_attribute, NULL},
> + { "reads_from_fd", 0, -1, false, true, true, false,
> + handle_reads_from_fd_attribute, NULL},
> + { "writes_to_fd", 0, -1, false, true, true, false,
> + handle_writes_to_fd_attribute, NULL},
> { "nonstring", 0, 0, true, false, false, false,
> handle_nonstring_attribute, NULL },
> { "nothrow", 0, 0, true, false, false, false,
> @@ -4521,6 +4531,59 @@ handle_nonnull_attribute (tree *node, tree name,
> return NULL_TREE;
> }
>
> +/* Handle the "accesses_fd" attribute */
> +static tree
> +handle_accesses_fd_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;
> + }
> +}
> +
> +
> +
> +static tree
> +handle_reads_from_fd_attribute (tree *node, tree name, tree ARG_UNUSED
> (args),
> + int ARG_UNUSED (flags), bool *no_add_attrs)
> +{
> + // TODO: valiadate usage of this attribute
> + return NULL_TREE;
> +}
> +
> +static tree
> +handle_writes_to_fd_attribute (tree *node, tree name, tree ARG_UNUSED
> (args),
> + int ARG_UNUSED (flags), bool *no_add_attrs)
> +{
> + // TODO: validate usage of this attribute
> + return NULL_TREE;
> +}
> +
> +
> +
> +
> /* Handle the "nonstring" variable attribute. */
>
> static tree
> 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..e597ba2367a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> @@ -0,0 +1,44 @@
> +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__((accesses_fd(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__((reads_from_fd(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__((writes_to_fd(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);
> +}
> \ 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..9c0b55086a9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
> @@ -0,0 +1,4 @@
> +
> +int not_a_fn __attribute__ ((accesses_fd(1))); /* { dg-warning
> "'accesses_fd' attribute only applies to function types" } */
> +
> +void f (char *p) __attribute__ ((accesses_fd(1))); /* { dg-warning
> "'accesses_fd' attribute argument value '1' refers to parameter type 'char
> \\\*'" } */
> \ No newline at end of file
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Adding file descriptor attribute(s) to gcc and glibc
2022-07-12 17:33 ` Mir Immad
@ 2022-07-12 22:16 ` David Malcolm
2022-07-12 22:25 ` David Malcolm
0 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2022-07-12 22:16 UTC (permalink / raw)
To: Mir Immad; +Cc: gcc, libc-alpha
On Tue, 2022-07-12 at 23:03 +0530, Mir Immad wrote:
[cross-posting to the glibc development mailing list; updating subject
accordingly]
> Hi everyone,
Hi Immad, GCC developers, and glibc developers.
glibc developers: Immad is a GSoC student, he's been adding checks for
file-descriptor-based APIs to gcc's -fanalyzer:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=97baacba963c06e3d0e33cde04e7e687671e60e7
>
> This is an in-progress patch for adding three new function attributes
> to
> GCC for static analysis of file descriptor APIs (which is a part of my
> GSoC
> project)
As you say, the patch is "in-progress".
Immad and I discussed this off-list and considered a few different
ideas about this; I'm especially hoping for feedback from glibc
developers on this idea, as I think the attributes could potentially be
used in dozens of places in glibc's headers.
>
> 1) __attribute__((accesses_fd(N)))
> This attribute expected argument N of a function to be an open file
> descriptor. (see test_1 of fd-5.c)
The patch is missing a few things, of which I think the most important
for a discussion is documentation: what do we want these attributes to
mean?
Presumably: __attribute__((accesses_fd(N))) means something like:
This function attribute documents to human and automated readers of the
code that the function expects the argument with the given index to be
a valid, open file-descriptor.
If -fanalyzer is enabled, it will complain with:
* -Wanalyzer-fd-use-without-check if the given argument is the result
of an "open" call that hasn't yet been checked for validity
* -Wanalyzer-fd-use-after-close if the given argument is a closed FD
The argument must be of "int" type.
...or somesuch.
>
> 2) __attribute__((reads_from_fd(N)))
> This attribute expects arguments N of a function to not be a write-only
> FD.
> (see test_2 of fd-5.c)
Maybe reword to: this is equivalent to "accesses_fd", but with the
additional check that the file descriptor must not have been opened
write-only; the analyzer will complain with -Wanalyzer-fd-access-mode-
mismatch if this is the case.
>
> 3) __attribute__((writes_to_fd(N)))
> This attribute expects arguments N of a function to not be a read-only
> FD.
> (see test_3 of fd-6.c)
As above, but must not have been opened read-only.
FWIW I'm not sure whether I prefer having three attributes, or one
attribute with an optional access direction parameter.
One drawback of the "three attributes" approach is that they're
alphabetically distant from each other, obscuring their close
relationship.
GCC's attribute syntax here:
https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
allows for a parenthesized list of parameters for the attribute, which
can be:
(a) An identifier
(b) An identifier followed by a comma and a non-empty comma-separated
list of expressions
(c) A possibly empty comma-separated list of expressions
I'd hoped to have an argument number, with an optional extra param
describing the direction of the access, but syntax (b) puts the
identifier first, alas.
Here's one possible way of doing it with a single attribute, via syntax
(b):
e.g.
__attribute__((fd_argument (access, 1))
__attribute__((fd_argument (read, 1))
__attribute__((fd_argument (write, 1))
meaning that argument 1 of the function is expected to be an open file-
descriptor, and that it must be possible to read from/write to that fd
for cases 2 and 3.
Here are some possible examples of how glibc might use this syntax:
int dup (int oldfd)
__attribute((fd_argument (access, 1));
int ftruncate (int fd, off_t length)
__attribute((fd_argument (access, 1));
ssize_t pread(int fd, void *buf, size_t count, off_t offset)
__attribute((fd_argument (read, 1));
ssize_t pwrite(int fd, const void *buf, size_t count,
off_t offset);
__attribute((fd_argument (write, 1));
...but as I said, I'm most interested in input from glibc developers on
this.
Hope this is constructive
Dave
>
> Thanks.
> Immad
>
>
>
>
>
> On Tue, Jul 12, 2022 at 11:02 PM Immad Mir <mirimmad@outlook.com>
> wrote:
>
> > ---
> > gcc/analyzer/sm-fd.cc | 231 ++++++++++++++++++++++++-
> > --
> > gcc/c-family/c-attribs.cc | 63 ++++++++
> > gcc/testsuite/gcc.dg/analyzer/fd-5.c | 44 +++++
> > gcc/testsuite/gcc.dg/analyzer/fd-6.c | 4 +
> > 4 files changed, 322 insertions(+), 20 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..bdc807160cc 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,17 +510,19 @@ 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
> > @@ -543,8 +589,38 @@ public:
> > private:
> > diagnostic_event_id_t m_first_open_event;
> > const tree m_callee_fndecl;
> > +
> > };
> >
> > +/* 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 +723,124 @@ 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__((filedescriptor))
> > + {
> > + bitmap argmap = get_fd_attrs ("accesses_fd",
> > callee_fndecl);
> > + if (argmap)
> > + check_for_fd_attrs (sm_ctxt, node, stmt, call,
> > callee_fndecl, argmap, DIR_READ_WRITE);
> > +
> > + bitmap read_argmap = get_fd_attrs ("reads_from_fd",
> > callee_fndecl);
> > + if(read_argmap)
> > + check_for_fd_attrs (sm_ctxt, node, stmt, call,
> > callee_fndecl, read_argmap, DIR_READ);
> > +
> > + bitmap write_argmap = get_fd_attrs ("writes_to_fd",
> > 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));
> > + }
> > + }
> > + }
> > + 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;
> > + default:
> > + gcc_unreachable ();
> > + }
> > + }
> > +}
> > +
> > +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 +918,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 +927,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
> > @@ -758,7 +947,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 +958,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..84f29987bc6 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_accesses_fd_attribute (tree *, tree, tree, int,
> > bool
> > *);
> > +static tree handle_reads_from_fd_attribute (tree *, tree, tree, int,
> > bool
> > *);
> > +static tree handle_writes_to_fd_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 },
> > + { "accesses_fd", 0, -1, false, true, true, false,
> > + handle_accesses_fd_attribute, NULL},
> > + { "reads_from_fd", 0, -1, false, true, true, false,
> > + handle_reads_from_fd_attribute, NULL},
> > + { "writes_to_fd", 0, -1, false, true, true, false,
> > + handle_writes_to_fd_attribute, NULL},
> > { "nonstring", 0, 0, true, false, false, false,
> > handle_nonstring_attribute, NULL },
> > { "nothrow", 0, 0, true, false, false, false,
> > @@ -4521,6 +4531,59 @@ handle_nonnull_attribute (tree *node, tree
> > name,
> > return NULL_TREE;
> > }
> >
> > +/* Handle the "accesses_fd" attribute */
> > +static tree
> > +handle_accesses_fd_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;
> > + }
> > +}
> > +
> > +
> > +
> > +static tree
> > +handle_reads_from_fd_attribute (tree *node, tree name, tree
> > ARG_UNUSED
> > (args),
> > + int ARG_UNUSED (flags), bool *no_add_attrs)
> > +{
> > + // TODO: valiadate usage of this attribute
> > + return NULL_TREE;
> > +}
> > +
> > +static tree
> > +handle_writes_to_fd_attribute (tree *node, tree name, tree
> > ARG_UNUSED
> > (args),
> > + int ARG_UNUSED (flags), bool *no_add_attrs)
> > +{
> > + // TODO: validate usage of this attribute
> > + return NULL_TREE;
> > +}
> > +
> > +
> > +
> > +
> > /* Handle the "nonstring" variable attribute. */
> >
> > static tree
> > 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..e597ba2367a
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> > @@ -0,0 +1,44 @@
> > +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__((accesses_fd(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__((reads_from_fd(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__((writes_to_fd(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);
> > +}
> > \ 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..9c0b55086a9
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
> > @@ -0,0 +1,4 @@
> > +
> > +int not_a_fn __attribute__ ((accesses_fd(1))); /* { dg-warning
> > "'accesses_fd' attribute only applies to function types" } */
> > +
> > +void f (char *p) __attribute__ ((accesses_fd(1))); /* { dg-warning
> > "'accesses_fd' attribute argument value '1' refers to parameter type
> > 'char
> > \\\*'" } */
> > \ No newline at end of file
> > --
> > 2.25.1
> >
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-12 22:16 ` Adding file descriptor attribute(s) to gcc and glibc David Malcolm
@ 2022-07-12 22:25 ` David Malcolm
2022-07-13 8:37 ` Szabolcs Nagy
0 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2022-07-12 22:25 UTC (permalink / raw)
To: Mir Immad; +Cc: gcc, libc-alpha
On Tue, 2022-07-12 at 18:16 -0400, David Malcolm wrote:
> On Tue, 2022-07-12 at 23:03 +0530, Mir Immad wrote:
>
> [cross-posting to the glibc development mailing list; updating subject
> accordingly]
>
> > Hi everyone,
>
> Hi Immad, GCC developers, and glibc developers.
>
> glibc developers: Immad is a GSoC student, he's been adding checks for
> file-descriptor-based APIs to gcc's -fanalyzer:
>
> https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=97baacba963c06e3d0e33cde04e7e687671e60e7
>
> >
> > This is an in-progress patch for adding three new function attributes
> > to
> > GCC for static analysis of file descriptor APIs (which is a part of
> > my
> > GSoC
> > project)
>
> As you say, the patch is "in-progress".
>
> Immad and I discussed this off-list and considered a few different
> ideas about this; I'm especially hoping for feedback from glibc
> developers on this idea, as I think the attributes could potentially be
> used in dozens of places in glibc's headers.
>
> >
> > 1) __attribute__((accesses_fd(N)))
> > This attribute expected argument N of a function to be an open file
> > descriptor. (see test_1 of fd-5.c)
>
> The patch is missing a few things, of which I think the most important
> for a discussion is documentation: what do we want these attributes to
> mean?
>
> Presumably: __attribute__((accesses_fd(N))) means something like:
>
> This function attribute documents to human and automated readers of the
> code that the function expects the argument with the given index to be
> a valid, open file-descriptor.
>
> If -fanalyzer is enabled, it will complain with:
> * -Wanalyzer-fd-use-without-check if the given argument is the result
> of an "open" call that hasn't yet been checked for validity
> * -Wanalyzer-fd-use-after-close if the given argument is a closed FD
>
> The argument must be of "int" type.
>
>
> ...or somesuch.
>
> >
> > 2) __attribute__((reads_from_fd(N)))
> > This attribute expects arguments N of a function to not be a write-
> > only
> > FD.
> > (see test_2 of fd-5.c)
>
> Maybe reword to: this is equivalent to "accesses_fd", but with the
> additional check that the file descriptor must not have been opened
> write-only; the analyzer will complain with -Wanalyzer-fd-access-mode-
> mismatch if this is the case.
>
>
> >
> > 3) __attribute__((writes_to_fd(N)))
> > This attribute expects arguments N of a function to not be a read-
> > only
> > FD.
> > (see test_3 of fd-6.c)
>
> As above, but must not have been opened read-only.
>
>
> FWIW I'm not sure whether I prefer having three attributes, or one
> attribute with an optional access direction parameter.
>
> One drawback of the "three attributes" approach is that they're
> alphabetically distant from each other, obscuring their close
> relationship.
>
> GCC's attribute syntax here:
> https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
> allows for a parenthesized list of parameters for the attribute, which
> can be:
> (a) An identifier
> (b) An identifier followed by a comma and a non-empty comma-separated
> list of expressions
> (c) A possibly empty comma-separated list of expressions
>
> I'd hoped to have an argument number, with an optional extra param
> describing the direction of the access, but syntax (b) puts the
> identifier first, alas.
>
> Here's one possible way of doing it with a single attribute, via syntax
> (b):
> e.g.
> __attribute__((fd_argument (access, 1))
> __attribute__((fd_argument (read, 1))
> __attribute__((fd_argument (write, 1))
>
> meaning that argument 1 of the function is expected to be an open file-
> descriptor, and that it must be possible to read from/write to that fd
> for cases 2 and 3.
>
> Here are some possible examples of how glibc might use this syntax:
>
> int dup (int oldfd)
> __attribute((fd_argument (access, 1));
>
> int ftruncate (int fd, off_t length)
> __attribute((fd_argument (access, 1));
>
> ssize_t pread(int fd, void *buf, size_t count, off_t offset)
> __attribute((fd_argument (read, 1));
>
> ssize_t pwrite(int fd, const void *buf, size_t count,
> off_t offset);
> __attribute((fd_argument (write, 1));
>
> ...but as I said, I'm most interested in input from glibc developers on
> this.
I just realized that the attribute could accept both the single integer
argument number (syntax (c)) for the "don't care about access
direction" case, or the ({read|write}, N) of syntax (b) above, giving
e.g.:
int dup (int oldfd)
__attribute((fd_argument (1));
int ftruncate (int fd, off_t length)
__attribute((fd_argument (1));
ssize_t pread(int fd, void *buf, size_t count, off_t offset)
__attribute((fd_argument (read, 1));
ssize_t pwrite(int fd, const void *buf, size_t count,
off_t offset);
__attribute((fd_argument (write, 1));
for the above examples.
How does that look?
Dave
>
> Hope this is constructive
> Dave
>
> >
> > Thanks.
> > Immad
> >
> >
> >
> >
> >
> > On Tue, Jul 12, 2022 at 11:02 PM Immad Mir <mirimmad@outlook.com>
> > wrote:
> >
> > > ---
> > > gcc/analyzer/sm-fd.cc | 231
> > > ++++++++++++++++++++++++-
> > > --
> > > gcc/c-family/c-attribs.cc | 63 ++++++++
> > > gcc/testsuite/gcc.dg/analyzer/fd-5.c | 44 +++++
> > > gcc/testsuite/gcc.dg/analyzer/fd-6.c | 4 +
> > > 4 files changed, 322 insertions(+), 20 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..bdc807160cc 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,17 +510,19 @@ 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
> > > @@ -543,8 +589,38 @@ public:
> > > private:
> > > diagnostic_event_id_t m_first_open_event;
> > > const tree m_callee_fndecl;
> > > +
> > > };
> > >
> > > +/* 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 +723,124 @@ 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__((filedescriptor))
> > > + {
> > > + bitmap argmap = get_fd_attrs ("accesses_fd",
> > > callee_fndecl);
> > > + if (argmap)
> > > + check_for_fd_attrs (sm_ctxt, node, stmt, call,
> > > callee_fndecl, argmap, DIR_READ_WRITE);
> > > +
> > > + bitmap read_argmap = get_fd_attrs ("reads_from_fd",
> > > callee_fndecl);
> > > + if(read_argmap)
> > > + check_for_fd_attrs (sm_ctxt, node, stmt, call,
> > > callee_fndecl, read_argmap, DIR_READ);
> > > +
> > > + bitmap write_argmap = get_fd_attrs ("writes_to_fd",
> > > 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));
> > > + }
> > > + }
> > > + }
> > > + 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;
> > > + default:
> > > + gcc_unreachable ();
> > > + }
> > > + }
> > > +}
> > > +
> > > +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 +918,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 +927,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
> > > @@ -758,7 +947,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 +958,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..84f29987bc6 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_accesses_fd_attribute (tree *, tree, tree, int,
> > > bool
> > > *);
> > > +static tree handle_reads_from_fd_attribute (tree *, tree, tree,
> > > int,
> > > bool
> > > *);
> > > +static tree handle_writes_to_fd_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 },
> > > + { "accesses_fd", 0, -1, false, true, true, false,
> > > + handle_accesses_fd_attribute, NULL},
> > > + { "reads_from_fd", 0, -1, false, true, true, false,
> > > + handle_reads_from_fd_attribute, NULL},
> > > + { "writes_to_fd", 0, -1, false, true, true, false,
> > > + handle_writes_to_fd_attribute, NULL},
> > > { "nonstring", 0, 0, true, false, false, false,
> > > handle_nonstring_attribute, NULL },
> > > { "nothrow", 0, 0, true, false, false, false,
> > > @@ -4521,6 +4531,59 @@ handle_nonnull_attribute (tree *node, tree
> > > name,
> > > return NULL_TREE;
> > > }
> > >
> > > +/* Handle the "accesses_fd" attribute */
> > > +static tree
> > > +handle_accesses_fd_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;
> > > + }
> > > +}
> > > +
> > > +
> > > +
> > > +static tree
> > > +handle_reads_from_fd_attribute (tree *node, tree name, tree
> > > ARG_UNUSED
> > > (args),
> > > + int ARG_UNUSED (flags), bool *no_add_attrs)
> > > +{
> > > + // TODO: valiadate usage of this attribute
> > > + return NULL_TREE;
> > > +}
> > > +
> > > +static tree
> > > +handle_writes_to_fd_attribute (tree *node, tree name, tree
> > > ARG_UNUSED
> > > (args),
> > > + int ARG_UNUSED (flags), bool *no_add_attrs)
> > > +{
> > > + // TODO: validate usage of this attribute
> > > + return NULL_TREE;
> > > +}
> > > +
> > > +
> > > +
> > > +
> > > /* Handle the "nonstring" variable attribute. */
> > >
> > > static tree
> > > 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..e597ba2367a
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-5.c
> > > @@ -0,0 +1,44 @@
> > > +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__((accesses_fd(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__((reads_from_fd(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__((writes_to_fd(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);
> > > +}
> > > \ 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..9c0b55086a9
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
> > > @@ -0,0 +1,4 @@
> > > +
> > > +int not_a_fn __attribute__ ((accesses_fd(1))); /* { dg-warning
> > > "'accesses_fd' attribute only applies to function types" } */
> > > +
> > > +void f (char *p) __attribute__ ((accesses_fd(1))); /* { dg-warning
> > > "'accesses_fd' attribute argument value '1' refers to parameter
> > > type
> > > 'char
> > > \\\*'" } */
> > > \ No newline at end of file
> > > --
> > > 2.25.1
> > >
> > >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-12 22:25 ` David Malcolm
@ 2022-07-13 8:37 ` Szabolcs Nagy
2022-07-13 8:46 ` Andreas Schwab
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Szabolcs Nagy @ 2022-07-13 8:37 UTC (permalink / raw)
To: David Malcolm; +Cc: Mir Immad, gcc, libc-alpha
The 07/12/2022 18:25, David Malcolm via Libc-alpha wrote:
> On Tue, 2022-07-12 at 18:16 -0400, David Malcolm wrote:
> > On Tue, 2022-07-12 at 23:03 +0530, Mir Immad wrote:
> > GCC's attribute syntax here:
> > https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
> > allows for a parenthesized list of parameters for the attribute, which
> > can be:
> > (a) An identifier
> > (b) An identifier followed by a comma and a non-empty comma-separated
> > list of expressions
> > (c) A possibly empty comma-separated list of expressions
> >
> > I'd hoped to have an argument number, with an optional extra param
> > describing the direction of the access, but syntax (b) puts the
> > identifier first, alas.
> >
> > Here's one possible way of doing it with a single attribute, via syntax
> > (b):
> > e.g.
> > __attribute__((fd_argument (access, 1))
> > __attribute__((fd_argument (read, 1))
> > __attribute__((fd_argument (write, 1))
> >
> > meaning that argument 1 of the function is expected to be an open file-
> > descriptor, and that it must be possible to read from/write to that fd
> > for cases 2 and 3.
> >
> > Here are some possible examples of how glibc might use this syntax:
> >
> > int dup (int oldfd)
> > __attribute((fd_argument (access, 1));
> >
> > int ftruncate (int fd, off_t length)
> > __attribute((fd_argument (access, 1));
> >
> > ssize_t pread(int fd, void *buf, size_t count, off_t offset)
> > __attribute((fd_argument (read, 1));
> >
> > ssize_t pwrite(int fd, const void *buf, size_t count,
> > off_t offset);
> > __attribute((fd_argument (write, 1));
> >
> > ...but as I said, I'm most interested in input from glibc developers on
> > this.
note that glibc headers have to be namespace clean so it
would be more like
__attribute__((__fd_argument (__access, 1)))
__attribute__((__fd_argument (__read, 1)))
__attribute__((__fd_argument (__write, 1)))
so it would be even shorter to write
__attribute__((__fd_argument_access (1)))
__attribute__((__fd_argument_read (1)))
__attribute__((__fd_argument_write (1)))
>
> I just realized that the attribute could accept both the single integer
> argument number (syntax (c)) for the "don't care about access
> direction" case, or the ({read|write}, N) of syntax (b) above, giving
> e.g.:
>
> int dup (int oldfd)
> __attribute((fd_argument (1));
>
> int ftruncate (int fd, off_t length)
> __attribute((fd_argument (1));
>
> ssize_t pread(int fd, void *buf, size_t count, off_t offset)
> __attribute((fd_argument (read, 1));
>
> ssize_t pwrite(int fd, const void *buf, size_t count,
> off_t offset);
> __attribute((fd_argument (write, 1));
>
> for the above examples.
>
> How does that look?
> Dave
i think fd in ftruncate should be open for writing.
to be honest, i'd expect interesting fd bugs to be
dynamic and not easy to statically analyze.
the use-after-unchecked-open maybe useful. i would
not expect the access direction to catch many bugs.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-13 8:37 ` Szabolcs Nagy
@ 2022-07-13 8:46 ` Andreas Schwab
2022-07-13 12:05 ` Florian Weimer
2022-07-13 12:57 ` David Malcolm
2 siblings, 0 replies; 16+ messages in thread
From: Andreas Schwab @ 2022-07-13 8:46 UTC (permalink / raw)
To: Szabolcs Nagy via Libc-alpha; +Cc: David Malcolm, Szabolcs Nagy, gcc, Mir Immad
On Jul 13 2022, Szabolcs Nagy via Libc-alpha wrote:
> note that glibc headers have to be namespace clean so it
> would be more like
>
> __attribute__((__fd_argument (__access, 1)))
> __attribute__((__fd_argument (__read, 1)))
> __attribute__((__fd_argument (__write, 1)))
>
> so it would be even shorter to write
>
> __attribute__((__fd_argument_access (1)))
> __attribute__((__fd_argument_read (1)))
> __attribute__((__fd_argument_write (1)))
The attribute will need to be hidden behind a macro anyway, like it is
done with most other attributes now.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-13 8:37 ` Szabolcs Nagy
2022-07-13 8:46 ` Andreas Schwab
@ 2022-07-13 12:05 ` Florian Weimer
2022-07-13 13:33 ` David Malcolm
2022-07-13 12:57 ` David Malcolm
2 siblings, 1 reply; 16+ messages in thread
From: Florian Weimer @ 2022-07-13 12:05 UTC (permalink / raw)
To: Szabolcs Nagy via Gcc; +Cc: David Malcolm, Szabolcs Nagy, libc-alpha
* Szabolcs Nagy via Gcc:
> to be honest, i'd expect interesting fd bugs to be
> dynamic and not easy to statically analyze.
> the use-after-unchecked-open maybe useful. i would
> not expect the access direction to catch many bugs.
You might be right. But I think the annotations could help to catch
use-after-close errors.
By the way, I think it would help us if we didn't have to special-case
AT_FDCWD using inline wrappers.
Thanks,
Florian
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-13 8:37 ` Szabolcs Nagy
2022-07-13 8:46 ` Andreas Schwab
2022-07-13 12:05 ` Florian Weimer
@ 2022-07-13 12:57 ` David Malcolm
2 siblings, 0 replies; 16+ messages in thread
From: David Malcolm @ 2022-07-13 12:57 UTC (permalink / raw)
To: Szabolcs Nagy; +Cc: Mir Immad, gcc, libc-alpha
On Wed, 2022-07-13 at 09:37 +0100, Szabolcs Nagy wrote:
> The 07/12/2022 18:25, David Malcolm via Libc-alpha wrote:
> > On Tue, 2022-07-12 at 18:16 -0400, David Malcolm wrote:
> > > On Tue, 2022-07-12 at 23:03 +0530, Mir Immad wrote:
> > > GCC's attribute syntax here:
> > > https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
> > > allows for a parenthesized list of parameters for the attribute,
> > > which
> > > can be:
> > > (a) An identifier
> > > (b) An identifier followed by a comma and a non-empty comma-
> > > separated
> > > list of expressions
> > > (c) A possibly empty comma-separated list of expressions
> > >
> > > I'd hoped to have an argument number, with an optional extra param
> > > describing the direction of the access, but syntax (b) puts the
> > > identifier first, alas.
> > >
> > > Here's one possible way of doing it with a single attribute, via
> > > syntax
> > > (b):
> > > e.g.
> > > __attribute__((fd_argument (access, 1))
> > > __attribute__((fd_argument (read, 1))
> > > __attribute__((fd_argument (write, 1))
> > >
> > > meaning that argument 1 of the function is expected to be an open
> > > file-
> > > descriptor, and that it must be possible to read from/write to that
> > > fd
> > > for cases 2 and 3.
> > >
> > > Here are some possible examples of how glibc might use this syntax:
> > >
> > > int dup (int oldfd)
> > > __attribute((fd_argument (access, 1));
> > >
> > > int ftruncate (int fd, off_t length)
> > > __attribute((fd_argument (access, 1));
> > >
> > > ssize_t pread(int fd, void *buf, size_t count, off_t offset)
> > > __attribute((fd_argument (read, 1));
> > >
> > > ssize_t pwrite(int fd, const void *buf, size_t count,
> > > off_t offset);
> > > __attribute((fd_argument (write, 1));
> > >
> > > ...but as I said, I'm most interested in input from glibc
> > > developers on
> > > this.
>
> note that glibc headers have to be namespace clean so it
> would be more like
>
> __attribute__((__fd_argument (__access, 1)))
> __attribute__((__fd_argument (__read, 1)))
> __attribute__((__fd_argument (__write, 1)))
>
> so it would be even shorter to write
>
> __attribute__((__fd_argument_access (1)))
> __attribute__((__fd_argument_read (1)))
> __attribute__((__fd_argument_write (1)))
As I understand it, you'd use a macro for this, but this made me think
of the following attributes that GCC could provide:
__attribute__ ((fd_arg(N)))
__attribute__ ((fd_arg_read(N)))
__attribute__ ((fd_arg_write(N)))
(since GCC already has "__attribute__((format_arg(N)))")
It looks like you define your attribute macros in:
https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=misc/sys/cdefs.h;hb=HEAD
which presumably could be extended to add something like:
#if __GNUC_PREREQ (13, 0)
# define __attr_fd_arg(argno) __attribute__ ((fd_arg(argno)))
# define __attr_fd_arg_read(argno) __attribute__ ((fd_arg_read(argno)))
# define __attr_fd_arg_write(argno) __attribute__ ((fd_arg_write(argno)))
#else
# define __attr_fd_arg(argno)
# define __attr_fd_arg_read(argno)
# define __attr_fd_arg_write(argno)
#endif
if I've got my syntax correct.
(Or maybe "readable" and "writable"?)
>
> I just realized that the attribute could accept both the single integer
> argument number (syntax (c)) for the "don't care about access
> direction" case, or the ({read|write}, N) of syntax (b) above, giving
> e.g.:
>
> int dup (int oldfd)
> __attribute((fd_argument (1));
>
> int ftruncate (int fd, off_t length)
> __attribute((fd_argument (1));
>
> ssize_t pread(int fd, void *buf, size_t count, off_t offset)
> __attribute((fd_argument (read, 1));
>
> ssize_t pwrite(int fd, const void *buf, size_t count,
> off_t offset);
> __attribute((fd_argument (write, 1));
>
> for the above examples.
>
> How does that look?
> Dave
i think fd in ftruncate should be open for writing.
Agreed.
So with the above macros, this might look like:
int dup (int oldfd)
__attr_fd_arg(1);
int ftruncate (int fd, off_t length)
__attr_fd_arg_write(1);
ssize_t pread(int fd, void *buf, size_t count, off_t offset)
__attr_fd_arg_read(1);
ssize_t pwrite(int fd, const void *buf, size_t count,
off_t offset);
__attr_fd_arg_write(1);
to be honest, i'd expect interesting fd bugs to be
dynamic and not easy to statically analyze.
the use-after-unchecked-open maybe useful. i would
not expect the access direction to catch many bugs.
One goal of -fanalyzer is to help detect problems as code is written,
before it ever leaves the developer's workstation. So for instance it
might save a few seconds helping catch silly bugs where a developer is
working with two different FDs and gets the read and write FDs the
wrong way around.
Dave
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-13 12:05 ` Florian Weimer
@ 2022-07-13 13:33 ` David Malcolm
2022-07-13 14:01 ` Florian Weimer
0 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2022-07-13 13:33 UTC (permalink / raw)
To: Florian Weimer, Szabolcs Nagy via Gcc
Cc: Szabolcs Nagy, libc-alpha, Mir Immad
On Wed, 2022-07-13 at 14:05 +0200, Florian Weimer wrote:
> * Szabolcs Nagy via Gcc:
[adding Immad back to the CC list]
>
> > to be honest, i'd expect interesting fd bugs to be
> > dynamic and not easy to statically analyze.
> > the use-after-unchecked-open maybe useful. i would
> > not expect the access direction to catch many bugs.
>
> You might be right. But I think the annotations could help to catch
> use-after-close errors.
>
> By the way, I think it would help us if we didn't have to special-
> case
> AT_FDCWD using inline wrappers.
Florian: I confess I wasn't familiar with AT_FDCWD until I read your
email and did a little reading a few minutes ago; it seems to be a
"magic number" for an FD that has special meaning; on my system it has
the value -100.
GCC's current implementation of the various -Wanalyzer-fd-* warnings
will track state for constant integer values as well as symbolic
values; it doesn't have any special meanings for specific integer
values. So e.g. it doesn't assume that 0, 1, and 2 have specific
meaning or are opened with specific flags (the analysis doesn't
necessarily begin its execution path at the start of "main", so there's
no guarantee that the standard FDs have their standard meaning).
Presumably if someone attempts
close (AT_FDCWD);
they'll get -1 and errno set to EBADFD, right? I don't think GCC's -
fanalyzer needs to check for that.
-fanalyzer's filedescriptor support doesn't yet have a concept of
"directory filedescriptors". Should it? (similarly, it doesn't yet
know about sockets)
A possible way to annotate "openat":
int openat(int dirfd, const char *pathname, int flags)
__attr_fd_arg(1);
Dave
>
> Thanks,
> Florian
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-13 13:33 ` David Malcolm
@ 2022-07-13 14:01 ` Florian Weimer
2022-07-13 16:55 ` David Malcolm
2022-07-13 16:56 ` Mir Immad
0 siblings, 2 replies; 16+ messages in thread
From: Florian Weimer @ 2022-07-13 14:01 UTC (permalink / raw)
To: David Malcolm; +Cc: Szabolcs Nagy via Gcc, Szabolcs Nagy, libc-alpha, Mir Immad
* David Malcolm:
> On Wed, 2022-07-13 at 14:05 +0200, Florian Weimer wrote:
>> * Szabolcs Nagy via Gcc:
>
> [adding Immad back to the CC list]
>
>>
>> > to be honest, i'd expect interesting fd bugs to be
>> > dynamic and not easy to statically analyze.
>> > the use-after-unchecked-open maybe useful. i would
>> > not expect the access direction to catch many bugs.
>>
>> You might be right. But I think the annotations could help to catch
>> use-after-close errors.
>>
>> By the way, I think it would help us if we didn't have to special-
>> case
>> AT_FDCWD using inline wrappers.
>
> Florian: I confess I wasn't familiar with AT_FDCWD until I read your
> email and did a little reading a few minutes ago; it seems to be a
> "magic number" for an FD that has special meaning; on my system it has
> the value -100.
>
> GCC's current implementation of the various -Wanalyzer-fd-* warnings
> will track state for constant integer values as well as symbolic
> values; it doesn't have any special meanings for specific integer
> values. So e.g. it doesn't assume that 0, 1, and 2 have specific
> meaning or are opened with specific flags (the analysis doesn't
> necessarily begin its execution path at the start of "main", so there's
> no guarantee that the standard FDs have their standard meaning).
Ahh. It might be useful to detect close (-1) etc. as a form of
double-close, and ther AT_FDCWD is exceptional.
> Presumably if someone attempts
> close (AT_FDCWD);
> they'll get -1 and errno set to EBADFD, right?
Correct
> I don't think GCC's -fanalyzer needs to check for that.
Not sure …
> -fanalyzer's filedescriptor support doesn't yet have a concept of
> "directory filedescriptors". Should it? (similarly, it doesn't yet
> know about sockets)
>
> A possible way to annotate "openat":
>
> int openat(int dirfd, const char *pathname, int flags)
> __attr_fd_arg(1);
openat is not the most general interface in this regard. We have other
*at functions which accept an O_PATH descriptor (or maybe even a
different kind of non-directory descriptor) with pathname == "" and
AT_EMPTY_PATH. I'm not sure if modeling all this is beneficial.
Thanks,
Florian
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-13 14:01 ` Florian Weimer
@ 2022-07-13 16:55 ` David Malcolm
2022-07-14 8:30 ` Szabolcs Nagy
2022-07-13 16:56 ` Mir Immad
1 sibling, 1 reply; 16+ messages in thread
From: David Malcolm @ 2022-07-13 16:55 UTC (permalink / raw)
To: Florian Weimer
Cc: Szabolcs Nagy via Gcc, Szabolcs Nagy, libc-alpha, Mir Immad
On Wed, 2022-07-13 at 16:01 +0200, Florian Weimer wrote:
> * David Malcolm:
>
> > On Wed, 2022-07-13 at 14:05 +0200, Florian Weimer wrote:
> > > * Szabolcs Nagy via Gcc:
> >
> > [adding Immad back to the CC list]
> >
> > >
> > > > to be honest, i'd expect interesting fd bugs to be
> > > > dynamic and not easy to statically analyze.
> > > > the use-after-unchecked-open maybe useful. i would
> > > > not expect the access direction to catch many bugs.
> > >
> > > You might be right. But I think the annotations could help to
> > > catch
> > > use-after-close errors.
> > >
> > > By the way, I think it would help us if we didn't have to special-
> > > case
> > > AT_FDCWD using inline wrappers.
> >
> > Florian: I confess I wasn't familiar with AT_FDCWD until I read your
> > email and did a little reading a few minutes ago; it seems to be a
> > "magic number" for an FD that has special meaning; on my system it
> > has
> > the value -100.
> >
> > GCC's current implementation of the various -Wanalyzer-fd-* warnings
> > will track state for constant integer values as well as symbolic
> > values; it doesn't have any special meanings for specific integer
> > values. So e.g. it doesn't assume that 0, 1, and 2 have specific
> > meaning or are opened with specific flags (the analysis doesn't
> > necessarily begin its execution path at the start of "main", so
> > there's
> > no guarantee that the standard FDs have their standard meaning).
>
> Ahh. It might be useful to detect close (-1) etc. as a form of
> double-close, and ther AT_FDCWD is exceptional.
It turns out we don't warn for that.
GCC trunk's -fanalyzer implements the new warnings via a state machine
for file-descriptor values; it currently has rules for handling "open",
"close", "read", and "write", and these functions are currently hard-
coded inside the analyzer.
Here are some examples on Compiler Explorer of what it can/cannot
detect:
https://godbolt.org/z/nqPadvM4f
Probably the most important one IMHO is the leak detection.
Would it be helpful to have some kind of attribute for "returns a new
open FD"? Are there other ways to close a FD other than calling
"close" on it? (Would converting that to some kind of "closes"
attribute be a good idea?)
> > Presumably if someone attempts
> > close (AT_FDCWD);
> > they'll get -1 and errno set to EBADFD, right?
>
> Correct
>
> > I don't think GCC's -fanalyzer needs to check for that.
>
> Not sure …
Are there any other "magic" values for file-descriptors we should be
aware of?
>
> > -fanalyzer's filedescriptor support doesn't yet have a concept of
> > "directory filedescriptors". Should it? (similarly, it doesn't
> > yet
> > know about sockets)
> >
> > A possible way to annotate "openat":
> >
> > int openat(int dirfd, const char *pathname, int flags)
> > __attr_fd_arg(1);
>
> openat is not the most general interface in this regard. We have
> other
> *at functions which accept an O_PATH descriptor (or maybe even a
> different kind of non-directory descriptor) with pathname == "" and
> AT_EMPTY_PATH. I'm not sure if modeling all this is beneficial.
Yeah, I don't think it's worth modeling things in that level of detail.
In particular, I don't want to get into modeling paths in the
filesystem, since that would be a huge scope-creep for the project.
Thanks
Dave
>
> Thanks,
> Florian
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-13 14:01 ` Florian Weimer
2022-07-13 16:55 ` David Malcolm
@ 2022-07-13 16:56 ` Mir Immad
2022-07-13 19:29 ` David Malcolm
1 sibling, 1 reply; 16+ messages in thread
From: Mir Immad @ 2022-07-13 16:56 UTC (permalink / raw)
To: Florian Weimer
Cc: David Malcolm, Szabolcs Nagy via Gcc, Szabolcs Nagy, libc-alpha
Hi everyone,
Reading through the thread, I feel the following attributes look good and
similar to what I've done:
__attribute__ ((fd_arg(N)))
__attribute__ ((fd_arg_read(N)))
__attribute__ ((fd_arg_write(N)))
I believe how to annotate the glibc headers is a separate discussion.
Dave: From the GCC side of things, these three attributes are basic
functionalities that can be useful for any piece of code that passes around
file descriptors.
Thanks
Immad
On Wed, Jul 13, 2022 at 7:31 PM Florian Weimer <fweimer@redhat.com> wrote:
> * David Malcolm:
>
> > On Wed, 2022-07-13 at 14:05 +0200, Florian Weimer wrote:
> >> * Szabolcs Nagy via Gcc:
> >
> > [adding Immad back to the CC list]
> >
> >>
> >> > to be honest, i'd expect interesting fd bugs to be
> >> > dynamic and not easy to statically analyze.
> >> > the use-after-unchecked-open maybe useful. i would
> >> > not expect the access direction to catch many bugs.
> >>
> >> You might be right. But I think the annotations could help to catch
> >> use-after-close errors.
> >>
> >> By the way, I think it would help us if we didn't have to special-
> >> case
> >> AT_FDCWD using inline wrappers.
> >
> > Florian: I confess I wasn't familiar with AT_FDCWD until I read your
> > email and did a little reading a few minutes ago; it seems to be a
> > "magic number" for an FD that has special meaning; on my system it has
> > the value -100.
> >
> > GCC's current implementation of the various -Wanalyzer-fd-* warnings
> > will track state for constant integer values as well as symbolic
> > values; it doesn't have any special meanings for specific integer
> > values. So e.g. it doesn't assume that 0, 1, and 2 have specific
> > meaning or are opened with specific flags (the analysis doesn't
> > necessarily begin its execution path at the start of "main", so there's
> > no guarantee that the standard FDs have their standard meaning).
>
> Ahh. It might be useful to detect close (-1) etc. as a form of
> double-close, and ther AT_FDCWD is exceptional.
>
> > Presumably if someone attempts
> > close (AT_FDCWD);
> > they'll get -1 and errno set to EBADFD, right?
>
> Correct
>
> > I don't think GCC's -fanalyzer needs to check for that.
>
> Not sure …
>
> > -fanalyzer's filedescriptor support doesn't yet have a concept of
> > "directory filedescriptors". Should it? (similarly, it doesn't yet
> > know about sockets)
> >
> > A possible way to annotate "openat":
> >
> > int openat(int dirfd, const char *pathname, int flags)
> > __attr_fd_arg(1);
>
> openat is not the most general interface in this regard. We have other
> *at functions which accept an O_PATH descriptor (or maybe even a
> different kind of non-directory descriptor) with pathname == "" and
> AT_EMPTY_PATH. I'm not sure if modeling all this is beneficial.
>
> Thanks,
> Florian
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-13 16:56 ` Mir Immad
@ 2022-07-13 19:29 ` David Malcolm
0 siblings, 0 replies; 16+ messages in thread
From: David Malcolm @ 2022-07-13 19:29 UTC (permalink / raw)
To: Mir Immad, Florian Weimer
Cc: Szabolcs Nagy via Gcc, Szabolcs Nagy, libc-alpha
On Wed, 2022-07-13 at 22:26 +0530, Mir Immad wrote:
> Hi everyone,
>
> Reading through the thread, I feel the following attributes look good
> and
> similar to what I've done:
>
> __attribute__ ((fd_arg(N)))
> __attribute__ ((fd_arg_read(N)))
> __attribute__ ((fd_arg_write(N)))
>
> I believe how to annotate the glibc headers is a separate discussion.
>
> Dave: From the GCC side of things, these three attributes are basic
> functionalities that can be useful for any piece of code that passes
> around
> file descriptors.
Immad: sounds good. Please use the above names/syntax for the next
iteration of your patch to -fanalyzer.
Thanks!
Dave
>
> Thanks
> Immad
>
>
> On Wed, Jul 13, 2022 at 7:31 PM Florian Weimer <fweimer@redhat.com>
> wrote:
>
> > * David Malcolm:
> >
> > > On Wed, 2022-07-13 at 14:05 +0200, Florian Weimer wrote:
> > > > * Szabolcs Nagy via Gcc:
> > >
> > > [adding Immad back to the CC list]
> > >
> > > >
> > > > > to be honest, i'd expect interesting fd bugs to be
> > > > > dynamic and not easy to statically analyze.
> > > > > the use-after-unchecked-open maybe useful. i would
> > > > > not expect the access direction to catch many bugs.
> > > >
> > > > You might be right. But I think the annotations could help to
> > > > catch
> > > > use-after-close errors.
> > > >
> > > > By the way, I think it would help us if we didn't have to
> > > > special-
> > > > case
> > > > AT_FDCWD using inline wrappers.
> > >
> > > Florian: I confess I wasn't familiar with AT_FDCWD until I read
> > > your
> > > email and did a little reading a few minutes ago; it seems to be
> > > a
> > > "magic number" for an FD that has special meaning; on my system
> > > it has
> > > the value -100.
> > >
> > > GCC's current implementation of the various -Wanalyzer-fd-*
> > > warnings
> > > will track state for constant integer values as well as symbolic
> > > values; it doesn't have any special meanings for specific integer
> > > values. So e.g. it doesn't assume that 0, 1, and 2 have specific
> > > meaning or are opened with specific flags (the analysis doesn't
> > > necessarily begin its execution path at the start of "main", so
> > > there's
> > > no guarantee that the standard FDs have their standard meaning).
> >
> > Ahh. It might be useful to detect close (-1) etc. as a form of
> > double-close, and ther AT_FDCWD is exceptional.
> >
> > > Presumably if someone attempts
> > > close (AT_FDCWD);
> > > they'll get -1 and errno set to EBADFD, right?
> >
> > Correct
> >
> > > I don't think GCC's -fanalyzer needs to check for that.
> >
> > Not sure …
> >
> > > -fanalyzer's filedescriptor support doesn't yet have a concept of
> > > "directory filedescriptors". Should it? (similarly, it doesn't
> > > yet
> > > know about sockets)
> > >
> > > A possible way to annotate "openat":
> > >
> > > int openat(int dirfd, const char *pathname, int flags)
> > > __attr_fd_arg(1);
> >
> > openat is not the most general interface in this regard. We have
> > other
> > *at functions which accept an O_PATH descriptor (or maybe even a
> > different kind of non-directory descriptor) with pathname == "" and
> > AT_EMPTY_PATH. I'm not sure if modeling all this is beneficial.
> >
> > Thanks,
> > Florian
> >
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-13 16:55 ` David Malcolm
@ 2022-07-14 8:30 ` Szabolcs Nagy
2022-07-14 15:22 ` David Malcolm
0 siblings, 1 reply; 16+ messages in thread
From: Szabolcs Nagy @ 2022-07-14 8:30 UTC (permalink / raw)
To: David Malcolm
Cc: Florian Weimer, Szabolcs Nagy via Gcc, libc-alpha, Mir Immad
The 07/13/2022 12:55, David Malcolm wrote:
> On Wed, 2022-07-13 at 16:01 +0200, Florian Weimer wrote:
> > * David Malcolm:
> GCC trunk's -fanalyzer implements the new warnings via a state machine
> for file-descriptor values; it currently has rules for handling "open",
> "close", "read", and "write", and these functions are currently hard-
> coded inside the analyzer.
>
> Here are some examples on Compiler Explorer of what it can/cannot
> detect:
> https://godbolt.org/z/nqPadvM4f
>
> Probably the most important one IMHO is the leak detection.
nice.
> Would it be helpful to have some kind of attribute for "returns a new
> open FD"? Are there other ways to close a FD other than calling
> "close" on it? (Would converting that to some kind of "closes"
> attribute be a good idea?)
dup2(oldfd, newfd)
dup3(oldfd, newfd, flags)
closes newfd (and also opens it to be a dup of oldfd)
unless the call fails.
close_range(first, last, flags)
fclose(fdopen(fd, mode))
but users can write all sorts of wrappers around close too.
>
> Are there any other "magic" values for file-descriptors we should be
> aware of?
>
mmap may require fd==-1 for anonymous maps.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-14 8:30 ` Szabolcs Nagy
@ 2022-07-14 15:22 ` David Malcolm
2022-07-14 17:07 ` Paul Eggert
0 siblings, 1 reply; 16+ messages in thread
From: David Malcolm @ 2022-07-14 15:22 UTC (permalink / raw)
To: Szabolcs Nagy
Cc: Florian Weimer, Szabolcs Nagy via Gcc, libc-alpha, Mir Immad
On Thu, 2022-07-14 at 09:30 +0100, Szabolcs Nagy wrote:
> The 07/13/2022 12:55, David Malcolm wrote:
> > On Wed, 2022-07-13 at 16:01 +0200, Florian Weimer wrote:
> > > * David Malcolm:
> > GCC trunk's -fanalyzer implements the new warnings via a state
> > machine
> > for file-descriptor values; it currently has rules for handling
> > "open",
> > "close", "read", and "write", and these functions are currently hard-
> > coded inside the analyzer.
> >
> > Here are some examples on Compiler Explorer of what it can/cannot
> > detect:
> > https://godbolt.org/z/nqPadvM4f
> >
> > Probably the most important one IMHO is the leak detection.
>
> nice.
>
> > Would it be helpful to have some kind of attribute for "returns a new
> > open FD"? Are there other ways to close a FD other than calling
> > "close" on it? (Would converting that to some kind of "closes"
> > attribute be a good idea?)
Thanks, lots of good ideas here; I've filed various RFEs about these;
I'm posting the details below for reference.
>
> dup2(oldfd, newfd)
> dup3(oldfd, newfd, flags)
>
> closes newfd (and also opens it to be a dup of oldfd)
> unless the call fails.
dup and friends probably need special-casing; I've filed this as:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106298
>
> close_range(first, last, flags)
close_range and closefrom would need special-casing, already filed as:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106283
>
> fclose(fdopen(fd, mode))
The analyzer now attempts to track both file descriptors and stdio
streams, so we probably need to special-case fdopen to capture the
various possible interactions between these two leak detectors; I've
filed this as:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106299
with an implementation idea there.
>
> but users can write all sorts of wrappers around close too.
Yeah. If the -fanalyzer leak detectors see a value "escape" into
unknown code, then they don't report leaks; see e.g.:
https://godbolt.org/z/n8fMhGTP5
where we don't report in test_2 about fd leaking due to the call to:
might_close (fd);
which is extern, and so we conservatively assume that fd doesn't leak.
>
> >
> > Are there any other "magic" values for file-descriptors we should be
> > aware of?
> >
>
> mmap may require fd==-1 for anonymous maps.
mmap is its own can of worms, which I've filed as:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106301
You also reminded me that we need to track other ways in which the user
could obtain an fd that could leak, which I've filed as:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106300
(covering creat, pipe and friends, dup and friends, fcntl, and socket).
I've added all of these to the top-level RFE for -fanalyzer tracking
file descriptors:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=analyzer-fd
which is now a tracker-bug:
https://gcc.gnu.org/bugzilla/showdependencytree.cgi?id=analyzer-fd
Thanks again for the ideas
Dave
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Adding file descriptor attribute(s) to gcc and glibc
2022-07-14 15:22 ` David Malcolm
@ 2022-07-14 17:07 ` Paul Eggert
0 siblings, 0 replies; 16+ messages in thread
From: Paul Eggert @ 2022-07-14 17:07 UTC (permalink / raw)
To: David Malcolm; +Cc: Szabolcs Nagy via Gcc, libc-alpha
On 7/14/22 08:22, David Malcolm via Libc-alpha wrote:
> The analyzer now attempts to track both file descriptors and stdio
> streams, so we probably need to special-case fdopen
You probably also need to special-case fileno, which goes the opposite
direction.
If you're handling DIR *, fdopendir and dirfd would need similar treatment.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-07-14 17:07 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-12 17:31 [PATCH] filedescriptor attribute Immad Mir
2022-07-12 17:33 ` Mir Immad
2022-07-12 22:16 ` Adding file descriptor attribute(s) to gcc and glibc David Malcolm
2022-07-12 22:25 ` David Malcolm
2022-07-13 8:37 ` Szabolcs Nagy
2022-07-13 8:46 ` Andreas Schwab
2022-07-13 12:05 ` Florian Weimer
2022-07-13 13:33 ` David Malcolm
2022-07-13 14:01 ` Florian Weimer
2022-07-13 16:55 ` David Malcolm
2022-07-14 8:30 ` Szabolcs Nagy
2022-07-14 15:22 ` David Malcolm
2022-07-14 17:07 ` Paul Eggert
2022-07-13 16:56 ` Mir Immad
2022-07-13 19:29 ` David Malcolm
2022-07-13 12: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).