From: Mir Immad <mirimnan017@gmail.com>
Cc: gcc@gcc.gnu.org, David Malcolm <dmalcolm@redhat.com>
Subject: Re: [PATCH] filedescriptor attribute
Date: Tue, 12 Jul 2022 23:03:23 +0530 [thread overview]
Message-ID: <CAE1-7oyLmZJAue2S39fsvvJPH3KabPDNqhL6jjwTv4RybeAQEQ@mail.gmail.com> (raw)
In-Reply-To: <CY4PR1801MB1910129FD314E45D7AF2CC70C6869@CY4PR1801MB1910.namprd18.prod.outlook.com>
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
>
>
next prev parent reply other threads:[~2022-07-12 17:33 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-12 17:31 Immad Mir
2022-07-12 17:33 ` Mir Immad [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAE1-7oyLmZJAue2S39fsvvJPH3KabPDNqhL6jjwTv4RybeAQEQ@mail.gmail.com \
--to=mirimnan017@gmail.com \
--cc=dmalcolm@redhat.com \
--cc=gcc@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).