[gcc r13-5733] analyzer: fix -Wanalyzer-use-of-uninitialized-value false +ve on "read" [PR108661]

[gcc r13-5733] analyzer: fix -Wanalyzer-use-of-uninitialized-value false +ve on "read" [PR108661]

[David Malcolm]

https://gcc.gnu.org/g:c300e251f5b22d15b126f3c643cd55a119994e48

commit r13-5733-gc300e251f5b22d15b126f3c643cd55a119994e48
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Tue Feb 7 16:10:20 2023 -0500

    analyzer: fix -Wanalyzer-use-of-uninitialized-value false +ve on "read" [PR108661]
    
    My integration testing shows many false positives from
    -Wanalyzer-use-of-uninitialized-value.
    
    One cause turns out to be that as of r13-1404-g97baacba963c06
    fd_state_machine::on_stmt recognizes calls to "read", and returns true,
    so that region_model::on_call_post doesn't call handle_unrecognized_call
    on them, and so the analyzer erroneously "thinks" that the buffer
    pointed to by "read" is never touched by the "read" call.
    
    This works for "fread" because sm-file.cc implements kf_fread, which
    handles calls to "fread" by clobbering the buffer pointed to.  In the
    long term we should probably be smarter about this and bifurcate the
    analysis to consider e.g. errors vs full reads vs partial reads, etc
    (which I'm tracking in PR analyzer/108689).
    
    In the meantime, this patch adds a kf_read for "read" analogous to the
    one for "fread", fixing 6 false positives seen in git-2.39.0 and
    2 in haproxy-2.7.1.
    
    gcc/analyzer/ChangeLog:
            PR analyzer/108661
            * sm-fd.cc (class kf_read): New.
            (register_known_fd_functions): Register "read".
            * sm-file.cc (class kf_fread): Update comment.
    
    gcc/testsuite/ChangeLog:
            PR analyzer/108661
            * gcc.dg/analyzer/fread-pr108661.c: New test.
            * gcc.dg/analyzer/read-pr108661.c: New test.
    
    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

Diff:
---
 gcc/analyzer/sm-fd.cc                          | 33 +++++++++++++++++++++
 gcc/analyzer/sm-file.cc                        | 10 ++++++-
 gcc/testsuite/gcc.dg/analyzer/fread-pr108661.c | 40 ++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/analyzer/read-pr108661.c  | 33 +++++++++++++++++++++
 4 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc
index 494d802a1d4..d107390bc00 100644
--- a/gcc/analyzer/sm-fd.cc
+++ b/gcc/analyzer/sm-fd.cc
@@ -2659,6 +2659,38 @@ private:
   unsigned m_num_args;
 };
 
+/* Handler for "read".
+     ssize_t read(int fildes, void *buf, size_t nbyte);
+   See e.g. https://man7.org/linux/man-pages/man2/read.2.html   */
+
+class kf_read : public known_function
+{
+public:
+  bool matches_call_types_p (const call_details &cd) const final override
+  {
+    return (cd.num_args () == 3
+	    && cd.arg_is_pointer_p (1)
+	    && cd.arg_is_size_p (2));
+  }
+
+  /* For now, assume that any call to "read" fully clobbers the buffer
+     passed in.  This isn't quite correct (e.g. errors, partial reads;
+     see PR analyzer/108689), but at least stops us falsely complaining
+     about the buffer being uninitialized.  */
+  void impl_call_pre (const call_details &cd) const final override
+  {
+    region_model *model = cd.get_model ();
+    const svalue *ptr_sval = cd.get_arg_svalue (1);
+    if (const region *reg = ptr_sval->maybe_get_region ())
+      {
+	const region *base_reg = reg->get_base_region ();
+	const svalue *new_sval = cd.get_or_create_conjured_svalue (base_reg);
+	model->set_value (base_reg, new_sval, cd.get_ctxt ());
+      }
+  }
+};
+
+
 /* Populate KFM with instances of known functions relating to
    file descriptors.  */
 
@@ -2672,6 +2704,7 @@ register_known_fd_functions (known_function_manager &kfm)
   kfm.add ("listen", make_unique<kf_listen> ());
   kfm.add ("pipe", make_unique<kf_pipe> (1));
   kfm.add ("pipe2", make_unique<kf_pipe> (2));
+  kfm.add ("read", make_unique<kf_read> ());
   kfm.add ("socket", make_unique<kf_socket> ());
 }
 
diff --git a/gcc/analyzer/sm-file.cc b/gcc/analyzer/sm-file.cc
index 9cb4e32e286..d99a09b76c4 100644
--- a/gcc/analyzer/sm-file.cc
+++ b/gcc/analyzer/sm-file.cc
@@ -560,7 +560,11 @@ public:
   }
 };
 
-/* Handler for "fread"".  */
+/* Handler for "fread".
+     size_t fread(void *restrict buffer, size_t size, size_t count,
+		  FILE *restrict stream);
+   See e.g. https://en.cppreference.com/w/c/io/fread
+   and https://www.man7.org/linux/man-pages/man3/fread.3.html */
 
 class kf_fread : public known_function
 {
@@ -574,6 +578,10 @@ public:
 	    && cd.arg_is_pointer_p (3));
   }
 
+  /* For now, assume that any call to "fread" fully clobbers the buffer
+     passed in.  This isn't quite correct (e.g. errors, partial reads;
+     see PR analyzer/108689), but at least stops us falsely complaining
+     about the buffer being uninitialized.  */
   void impl_call_pre (const call_details &cd) const final override
   {
     region_model *model = cd.get_model ();
diff --git a/gcc/testsuite/gcc.dg/analyzer/fread-pr108661.c b/gcc/testsuite/gcc.dg/analyzer/fread-pr108661.c
new file mode 100644
index 00000000000..b51cf41ec2a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fread-pr108661.c
@@ -0,0 +1,40 @@
+typedef __SIZE_TYPE__ size_t;
+
+extern size_t fread (void *, size_t, size_t, void *);
+
+struct ring
+{
+  char buf[1024];
+};
+
+int
+test_one_large_item (void *fp)
+{
+  struct ring ring;
+  int ret;
+
+  ret = fread(&ring, sizeof(ring), 1, fp);
+
+  if (ret != 1)
+    return 1;
+
+  if (ring.buf[0] > 1) /* { dg-bogus "use of uninitialized value" } */
+    return 2;
+  return 3;
+}
+
+int
+test_many_small_items (void *fp)
+{
+  struct ring ring;
+  int ret;
+
+  ret = fread(&ring, 1, sizeof(ring), fp);
+
+  if (ret != sizeof(ring))
+    return 1;
+
+  if (ring.buf[0] > 1) /* { dg-bogus "use of uninitialized value" } */
+    return 2;
+  return 3;
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/read-pr108661.c b/gcc/testsuite/gcc.dg/analyzer/read-pr108661.c
new file mode 100644
index 00000000000..70335e6fef1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/read-pr108661.c
@@ -0,0 +1,33 @@
+typedef long int ssize_t;
+typedef long unsigned int size_t;
+
+extern int open(const char* __file, int __oflag, ...) __attribute__((__nonnull__(1)));
+extern int close(int __fd);
+extern ssize_t read(int __fd, void* __buf, size_t __nbytes);
+
+struct ring
+{
+  char buf[1024];
+};
+
+int
+test(const char* name)
+{
+  struct ring ring;
+  int fd;
+  int ret;
+
+  fd = open(name, 00);
+  if (fd < 0)
+    return 0;
+
+  ret = read(fd, &ring, sizeof(ring));
+  close(fd);
+
+  if (ret != sizeof(ring))
+    return 1;
+
+  if (ring.buf[0] > 1) /* { dg-bogus "use of uninitialized value" } */
+    return 2;
+  return 3;
+}