diff mbox series

analyzer: support for creat, dup, dup2 and dup3 in sm-fd.cc [PR106300]

Message ID CY4PR1801MB1910B4E2168360FFB118E69FC6999@CY4PR1801MB1910.namprd18.prod.outlook.com
State New
Headers show
Series analyzer: support for creat, dup, dup2 and dup3 in sm-fd.cc [PR106300] | expand

Commit Message

Immad Mir July 29, 2022, 4:29 p.m. UTC 
This patch extends the state machine in sm-fd.cc to support
creat, dup, dup2 and dup3 functions.

Lightly tested on x86_64 Linux.

gcc/analyzer/ChangeLog:
	PR analyzer/106300
	* sm-fd.cc (fd_state_machine::on_open): Add
	creat, dup, dup2 and dup3 functions.
	(enum dup): New.
	(fd_state_machine::valid_to_unchecked_state): New.
	(fd_state_machine::on_creat): New.
	(fd_state_machine::on_dup): New.

gcc/testsuite/ChangeLog:
	PR analyzer/106300
	* gcc.dg/analyzer/fd-1.c: Add tests for 'creat'.
	* gcc.dg/analyzer/fd-2.c: Likewise.
	* gcc.dg/analyzer/fd-4.c: Likewise.
	* gcc.dg/analyzer/fd-6.c: New tests.

Signed-off-by: Immad Mir <mirimmad@outlook.com>
---
 gcc/analyzer/sm-fd.cc                | 117 ++++++++++++++++++-
 gcc/testsuite/gcc.dg/analyzer/fd-1.c |  21 ++++
 gcc/testsuite/gcc.dg/analyzer/fd-2.c |  15 +++
 gcc/testsuite/gcc.dg/analyzer/fd-4.c |  31 ++++-
 gcc/testsuite/gcc.dg/analyzer/fd-6.c | 168 +++++++++++++++++++++++++++
 5 files changed, 350 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/analyzer/fd-6.c

Comments

David Malcolm July 29, 2022, 11:54 p.m. UTC | #1 
On Fri, 2022-07-29 at 21:59 +0530, Immad Mir wrote:
> This patch extends the state machine in sm-fd.cc to support
> creat, dup, dup2 and dup3 functions.

Thanks for the patch.

Please can you use PR 106298 for this (in the ChangeLog and subject
line), rather than 106300, as it's more specific.

It's always a battle to keep the subject line a reasonable length,
especially with an "analyzer: " prefix and a " [PRnnnnnn]" suffix.  I
think you can leave off the " in sm-fd.cc" from the subject line, which
will help.

I think the patch is close to ready; review comments inline...

> 
> Lightly tested on x86_64 Linux.
> 
> gcc/analyzer/ChangeLog:
>         PR analyzer/106300
>         * sm-fd.cc (fd_state_machine::on_open): Add
>         creat, dup, dup2 and dup3 functions.
>         (enum dup): New.
>         (fd_state_machine::valid_to_unchecked_state): New.
>         (fd_state_machine::on_creat): New.
>         (fd_state_machine::on_dup): New.
> 
> gcc/testsuite/ChangeLog:
>         PR analyzer/106300
>         * gcc.dg/analyzer/fd-1.c: Add tests for 'creat'.
>         * gcc.dg/analyzer/fd-2.c: Likewise.
>         * gcc.dg/analyzer/fd-4.c: Likewise.
>         * gcc.dg/analyzer/fd-6.c: New tests.

At some point we'll want to add documentation to invoke.texi about what
functions we're special-casing in -fanalyzer, but you can postpone the
sm-fd.cc part of that to a follow-up patch if you like.

[...snip...]

> --- a/gcc/analyzer/sm-fd.cc
> +++ b/gcc/analyzer/sm-fd.cc
> @@ -69,6 +69,13 @@ enum access_directions
>    DIRS_WRITE
>  };
>  
> +enum dup
> +{
> +  DUP_1,
> +  DUP_2,
> +  DUP_3
> +};

Please add a comment documenting this enum.

[...snip...]

> +void
> +fd_state_machine::check_for_dup (sm_context *sm_ctxt, const
> supernode *node,
> +                                const gimple *stmt, const gcall
> *call,
> +                                const tree callee_fndecl, enum dup
> kind) const
> +{
> +  tree lhs = gimple_call_lhs (call);
> +  tree arg_1 = gimple_call_arg (call, 0);
> +  state_t state_arg_1 = sm_ctxt->get_state (stmt, arg_1);
> +  tree diag_arg_1 = sm_ctxt->get_diagnostic_tree (arg_1);
> +  if (state_arg_1 == m_stop)
> +    return;
> +  if (!(is_constant_fd_p (state_arg_1) || is_valid_fd_p
> (state_arg_1)))
> +    {
> +      sm_ctxt->warn (
> +         node, stmt, arg_1,
> +         new fd_use_without_check (*this, diag_arg_1,
> callee_fndecl));
> +      if (kind == DUP_1)
> +       return;
> +    }

I don't see test coverage for dup of a closed fd; I think we'd want to
warn for that with an fd_use_after_close.  That ought to be tested for
here, but if I'm reading it right, the check is missing.  Can you reuse
fd_state_machine::check_for_open_fd here, rather than duplicating the
logic for use-without-check and for use-after-close ? (since I think
the code is almost the same, apart, perhaps from that early return when
kind is DUP_1).

> +  switch (kind)
> +    {
> +    case DUP_1:
> +      if (!is_constant_fd_p (state_arg_1))
> +       if (lhs)
> +         sm_ctxt->set_next_state (stmt, lhs,
> +                                  valid_to_unchecked_state
> (state_arg_1));
> +      break;

What happens on dup() of an integer constant?
My understanding is that:
* in terms of POSIX: dup will return either a new FD, or -1.
* as for this patch, the LHS won't be checked (for validity, or leaks).
Shouldn't we transition the LHS to m_unchecked_read_write?  Or am I
missing something?

I don't see test coverage for dup of integer constants - please add
some.


> +
> +    case DUP_2:
> +    case DUP_3:
> +      tree arg_2 = gimple_call_arg (call, 1);
> +      state_t state_arg_2 = sm_ctxt->get_state (stmt, arg_2);
> +      tree diag_arg_2 = sm_ctxt->get_diagnostic_tree (arg_2);
> +      if (state_arg_2 == m_stop)
> +       return;
> +      if (!(is_constant_fd_p (state_arg_2) || is_valid_fd_p
> (state_arg_2)))
> +       {
> +         sm_ctxt->warn (
> +             node, stmt, arg_2,
> +             new fd_use_without_check (*this, diag_arg_2,
> callee_fndecl));
> +         return;
> +       }
> +
> +      if (!is_constant_fd_p (state_arg_2))
> +       {
> +         if (lhs)
> +           sm_ctxt->set_next_state (stmt, lhs,
> +                                    valid_to_unchecked_state
> (state_arg_2));

I got a bit confused by the logic here, but I think it's correct. 
Maybe add a comment clarifying that we want to make sure we don't pass
-1 as the 2nd argument, and therefor we want to check for
is_valid_fd_p.

We're not yet modeling that lhs == arg_2 on success, but maybe we don't
need to.  Maybe add a comment to that effect.

[...snip...]

> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c

I get a bit lost when there are lots of numbered test files.  Perhaps
you could renaming this, say, to fd-dup-1.c, to reflect the theme of
what's being tested.  But that's up to you.

[...snip...]

Thanks again for the patch.

Dave
diff mbox series

Patch

diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
index ed923ade100..7906034599c 100644
--- a/gcc/analyzer/sm-fd.cc
+++ b/gcc/analyzer/sm-fd.cc
@@ -69,6 +69,13 @@  enum access_directions
   DIRS_WRITE
 };
 
+enum dup
+{
+  DUP_1,
+  DUP_2,
+  DUP_3
+};
+
 class fd_state_machine : public state_machine
 {
 public:
@@ -114,7 +121,9 @@  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;
-
+  /* Function for one-to-one correspondence between valid
+     and unchecked states.  */
+  state_t valid_to_unchecked_state (state_t state) const;
   /* State for a constant file descriptor (>= 0) */
   state_t m_constant_fd;
 
@@ -147,6 +156,8 @@  public:
 private:
   void on_open (sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
 		const gcall *call) const;
+  void on_creat (sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
+		const gcall *call) const;
   void on_close (sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
 		 const gcall *call) const;
   void on_read (sm_context *sm_ctxt, const supernode *node, const gimple *stmt,
@@ -170,6 +181,9 @@  private:
 			   const gimple *stmt, const gcall *call,
 			   const tree callee_fndecl, const char *attr_name,
 			   access_directions fd_attr_access_dir) const;
+  void check_for_dup (sm_context *sm_ctxt, const supernode *node,
+       const gimple *stmt, const gcall *call, const tree callee_fndecl,
+       enum dup kind) const;
 };
 
 /* Base diagnostic class relative to fd_state_machine.  */
@@ -723,6 +737,20 @@  fd_state_machine::is_constant_fd_p (state_t state) const
   return (state == m_constant_fd);
 }
 
+fd_state_machine::state_t
+fd_state_machine::valid_to_unchecked_state (state_t state) const
+{
+  if (state == m_valid_read_write)
+    return m_unchecked_read_write;
+  else if (state == m_valid_write_only)
+    return m_unchecked_write_only;
+  else if (state == m_valid_read_only)
+    return m_unchecked_read_only;
+  else
+    gcc_unreachable ();
+  return NULL;
+}
+
 bool
 fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
 			   const gimple *stmt) const
@@ -736,6 +764,11 @@  fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
 	    return true;
 	  } //  "open"
 
+	if (is_named_call_p (callee_fndecl, "creat", call, 2))
+	  {
+	    on_creat (sm_ctxt, node, stmt, call);
+	  } // "creat"
+
 	if (is_named_call_p (callee_fndecl, "close", call, 1))
 	  {
 	    on_close (sm_ctxt, node, stmt, call);
@@ -754,6 +787,23 @@  fd_state_machine::on_stmt (sm_context *sm_ctxt, const supernode *node,
 	    return true;
 	  } // "read"
 
+	if (is_named_call_p (callee_fndecl, "dup", call, 1))
+	  {
+	    check_for_dup (sm_ctxt, node, stmt, call, callee_fndecl, DUP_1);
+	    return true;
+	  }
+
+	if (is_named_call_p (callee_fndecl, "dup2", call, 2))
+	  {
+	    check_for_dup (sm_ctxt, node, stmt, call, callee_fndecl, DUP_2);
+	    return true;
+	  }
+
+	if (is_named_call_p (callee_fndecl, "dup3", call, 3))
+	  {
+	    check_for_dup (sm_ctxt, node, stmt, call, callee_fndecl, DUP_3);
+	    return true;
+	  }
 
 	{
 	  // Handle __attribute__((fd_arg))
@@ -899,6 +949,71 @@  fd_state_machine::on_open (sm_context *sm_ctxt, const supernode *node,
     }
 }
 
+void
+fd_state_machine::on_creat (sm_context *sm_ctxt, const supernode *node,
+			    const gimple *stmt, const gcall *call) const
+{
+  tree lhs = gimple_call_lhs (call);
+  if (lhs)
+    sm_ctxt->on_transition (node, stmt, lhs, m_start, m_unchecked_write_only);
+  else
+    sm_ctxt->warn (node, stmt, NULL_TREE, new fd_leak (*this, NULL_TREE));
+}
+
+void
+fd_state_machine::check_for_dup (sm_context *sm_ctxt, const supernode *node,
+				 const gimple *stmt, const gcall *call,
+				 const tree callee_fndecl, enum dup kind) const
+{
+  tree lhs = gimple_call_lhs (call);
+  tree arg_1 = gimple_call_arg (call, 0);
+  state_t state_arg_1 = sm_ctxt->get_state (stmt, arg_1);
+  tree diag_arg_1 = sm_ctxt->get_diagnostic_tree (arg_1);
+  if (state_arg_1 == m_stop)
+    return;
+  if (!(is_constant_fd_p (state_arg_1) || is_valid_fd_p (state_arg_1)))
+    {
+      sm_ctxt->warn (
+	  node, stmt, arg_1,
+	  new fd_use_without_check (*this, diag_arg_1, callee_fndecl));
+      if (kind == DUP_1)
+	return;
+    }
+  switch (kind)
+    {
+    case DUP_1:
+      if (!is_constant_fd_p (state_arg_1))
+	if (lhs)
+	  sm_ctxt->set_next_state (stmt, lhs,
+				   valid_to_unchecked_state (state_arg_1));
+      break;
+
+    case DUP_2:
+    case DUP_3:
+      tree arg_2 = gimple_call_arg (call, 1);
+      state_t state_arg_2 = sm_ctxt->get_state (stmt, arg_2);
+      tree diag_arg_2 = sm_ctxt->get_diagnostic_tree (arg_2);
+      if (state_arg_2 == m_stop)
+	return;
+      if (!(is_constant_fd_p (state_arg_2) || is_valid_fd_p (state_arg_2)))
+	{
+	  sm_ctxt->warn (
+	      node, stmt, arg_2,
+	      new fd_use_without_check (*this, diag_arg_2, callee_fndecl));
+	  return;
+	}
+
+      if (!is_constant_fd_p (state_arg_2))
+	{
+	  if (lhs)
+	    sm_ctxt->set_next_state (stmt, lhs,
+				     valid_to_unchecked_state (state_arg_2));
+	}
+
+      break;
+    }
+}
+
 void
 fd_state_machine::on_close (sm_context *sm_ctxt, const supernode *node,
 			    const gimple *stmt, const gcall *call) const
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-1.c b/gcc/testsuite/gcc.dg/analyzer/fd-1.c
index 8a72e63833c..5b85a3316e8 100644
--- a/gcc/testsuite/gcc.dg/analyzer/fd-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-1.c
@@ -3,6 +3,13 @@  int open(const char *, int mode);
 #define O_WRONLY 1
 #define O_RDWR 2
 
+typedef enum {
+  S_IRWXU
+  // etc
+} mode_t;
+
+int creat (const char *, mode_t mode);
+
 void
 test_1 (const char *path)
 {
@@ -37,3 +44,17 @@  void test_4 (const char *path)
   /* { dg-message "\\(1\\) leaks here" "" { target *-*-* } .-1 } */
 }
 
+void 
+test_5 (const char *path, mode_t mode)
+{
+  creat (path, mode); /* { dg-warning "leak of file descriptor \\\[CWE-775\\\]" } */
+}
+
+void
+test_6 (const char *path, mode_t mode)
+{
+  int fd = creat (path, mode);
+  return; /* { dg-warning "leak of file descriptor 'fd' \\\[CWE-775\\\]" } */
+}
+
+
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-2.c b/gcc/testsuite/gcc.dg/analyzer/fd-2.c
index d794b460a2e..10c9ecdb09d 100644
--- a/gcc/testsuite/gcc.dg/analyzer/fd-2.c
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-2.c
@@ -5,6 +5,13 @@  void close(int fd);
 #define O_RDWR 2
 #define STDIN 0
 
+typedef enum {
+  S_IRWXU
+  // etc
+} mode_t;
+
+int creat (const char *, mode_t mode);
+
 void 
 test_1 (const char *path)
 {
@@ -46,4 +53,12 @@  test_4 ()
     int fd = -1;
     close(fd);
     close(fd);
+}
+
+void
+test_5 (const char *path, mode_t mode)
+{
+    int fd = creat (path, mode);
+    close(fd);
+    close(fd); /* { dg-warning "double 'close' of file descriptor 'fd' \\\[CWE-1341\\\]" "warning" } */
 }
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-4.c b/gcc/testsuite/gcc.dg/analyzer/fd-4.c
index ecd787caff7..6b8fca5408f 100644
--- a/gcc/testsuite/gcc.dg/analyzer/fd-4.c
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-4.c
@@ -9,6 +9,12 @@  int read (int fd, void *buf, int nbytes);
 #define O_WRONLY 1
 #define O_RDWR 2
 
+typedef enum {
+  S_IRWXU
+  // etc
+} mode_t;
+
+int creat (const char *, mode_t mode);
 
 void
 test_1 (const char *path, void *buf)
@@ -69,4 +75,27 @@  test_5 (const char *path)
     int fd = open (path, O_RDWR);
     close(fd);
     printf("%d", fd); /* { dg-bogus "'printf' on a closed file descriptor 'fd'" } */
-}
\ No newline at end of file
+}
+
+
+void
+test_6 (const char *path, mode_t mode, void *buf)
+{
+  int fd = creat (path, mode);
+  if (fd != -1)
+  {
+    read (fd, buf, 1); /* { dg-warning "'read' on write-only file descriptor 'fd'" } */
+    close(fd);
+  }
+}
+
+void
+test_7 (const char *path, mode_t mode, void *buf)
+{
+  int fd = creat (path, mode);
+  if (fd != -1)
+  {
+    write (fd, buf, 1);
+    close(fd);
+  }
+}
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..5c58cd1519c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-6.c
@@ -0,0 +1,168 @@ 
+int open(const char *, int mode);
+void close(int fd);
+int dup (int old_fd);
+int dup2 (int old_fd, int new_fd);
+int dup3 (int old_fd, int new_fd, int flags);
+int write (int fd, void *buf, int nbytes);
+int read (int fd, void *buf, int nbytes);
+#define O_RDONLY 0
+#define O_WRONLY 1
+#define O_RDWR 2
+
+void test_1 (const char *path)
+{
+    int old_fd = open (path, O_RDWR);
+    int new_fd = dup (old_fd); /* { dg-warning "'dup' on possibly invalid file descriptor 'old_fd'" } */
+    close(old_fd);
+    close(new_fd);
+}
+
+void test_2 (const char *path)
+{
+    int old_fd = open (path, O_RDWR);
+    if (old_fd != -1)
+    {
+        int new_fd = dup (old_fd); 
+        close(old_fd);
+        return; /* { dg-warning "leak of file descriptor 'new_fd' \\\[CWE-775\\\]" } */
+    }
+}
+
+void test_3 (const char *path, void *buf)
+{
+    int old_fd = open (path, O_RDWR);
+    if (old_fd != -1)
+    {
+        int new_fd = dup (old_fd);
+        write (new_fd, buf, 1); /* { dg-warning "'write' on possibly invalid file descriptor 'new_fd'" } */
+        close (new_fd);
+        close(old_fd);
+    }
+}
+
+
+void test_5 (const char *path, void *buf)
+{
+    int old_fd = open (path, O_RDWR);
+    if (old_fd != -1)
+    {
+        int new_fd = dup (old_fd);
+        if (new_fd != -1)
+        {
+            write (new_fd, buf, 1); 
+            close (new_fd);
+            
+        }
+        close(old_fd);
+    }
+}
+
+
+void test_7 (const char *path)
+{
+    int old_fd = open (path, O_RDWR);
+    dup2 (old_fd, 4); /* { dg-warning "'dup2' on possibly invalid file descriptor 'old_fd'" } */
+    close(old_fd);
+}
+
+void test_8 (const char *path)
+{
+    int old_fd = open (path, O_RDWR);
+    int new_fd = open (path, O_RDWR);
+    if (old_fd != -1)
+    {
+        dup2 (old_fd, new_fd); /* { dg-warning "'dup2' on possibly invalid file descriptor 'new_fd'" } */
+        close (old_fd);
+    }
+    close (new_fd);
+}
+
+void test_9 (const char *path, void *buf)
+{
+    int old_fd = open (path, O_RDWR);
+    
+    if (old_fd != -1)
+    {
+        int new_fd = open (path, O_RDWR);
+        if (new_fd != -1)
+        {
+            int lhs = dup2 (old_fd, new_fd);
+            write (lhs, buf, 1); /* { dg-warning "'write' on possibly invalid file descriptor 'lhs'" } */
+            close(new_fd);
+            close(lhs);
+    }
+        close(old_fd);        
+    }
+}
+
+void test_10 (const char *path, int flags)
+{
+    int old_fd = open (path, O_RDWR);
+    int new_fd = open (path, O_RDWR);
+    if (old_fd != -1)
+    {
+        dup3 (old_fd, new_fd, flags); /* { dg-warning "'dup3' on possibly invalid file descriptor 'new_fd'" } */
+        close(old_fd);
+        
+    }
+    close(new_fd);
+}
+
+void test_11 (const char *path, int flags)
+{
+    int old_fd = open (path, O_RDWR);
+    int new_fd = open (path, O_RDWR);
+    if (new_fd != -1)
+    {
+        dup3 (old_fd, new_fd, flags); /* { dg-warning "'dup3' on possibly invalid file descriptor 'old_fd'" } */
+        close(new_fd);
+        
+    }
+    close(old_fd);
+}
+
+void test_12 (const char *path, void *buf)
+{
+    int old_fd = open (path, O_RDONLY);
+    if (old_fd != -1)
+    {
+        int new_fd = dup (old_fd);
+        if (new_fd != -1)
+        {
+            write (new_fd, buf, 1); /* { dg-warning "'write' on read-only file descriptor 'new_fd'" } */
+            close(new_fd);
+        }
+        close(old_fd);
+    }
+}
+
+void test_13 (const char *path, void *buf)
+{
+    int old_fd = open (path, O_WRONLY);
+    if (old_fd != -1)
+    {
+        int new_fd = dup (old_fd);
+        if (new_fd != -1)
+        {
+            read (new_fd, buf, 1); /* { dg-warning "'read' on write-only file descriptor 'new_fd'" } */
+            close(new_fd);
+        }
+        close(old_fd);
+    }
+}
+
+void test_14 (const char *path, void *buf)
+{
+    int old_fd = open (path, O_RDWR);
+    if (old_fd != -1)
+    {
+        int new_fd = dup (old_fd);
+        if (new_fd != -1)
+        {
+            write (new_fd, buf, 1);
+            read (new_fd, buf, 1);
+            close(new_fd);
+        }
+        close(old_fd);
+    }
+}
\ No newline at end of file