analyzer: new warning: -Wanalyzer-unterminated-string [PR105899]

This patch adds new functions to the analyzer for checking that
an argument at a callsite is a pointer to a valid null-terminated
string, and uses this for the following known functions:

- error (param 3, the format string)
- error_at_line (param 5, the format string)
- putenv
- strchr (1st param)
- strcpy (2nd param)
- strdup

Currently the check merely detects pointers to unterminated string
constants, and adds a new -Wanalyzer-unterminated-string to complain
about that.  I'm experimenting with detecting other ways in which
a buffer can fail to be null-terminated, and for other problems with
such buffers, but this patch at least adds the framework for wiring
up the check to specific parameters of known_functions.

gcc/analyzer/ChangeLog:
	PR analyzer/105899
	* analyzer.opt (Wanalyzer-unterminated-string): New.
	* call-details.cc
	(call_details::check_for_null_terminated_string_arg): New.
	* call-details.h
	(call_details::check_for_null_terminated_string_arg): New decl.
	* kf-analyzer.cc (class kf_analyzer_get_strlen): New.
	(register_known_analyzer_functions): Register it.
	* kf.cc (kf_error::impl_call_pre): Check that format arg is a
	valid null-terminated string.
	(kf_putenv::impl_call_pre): Likewise for the sole param.
	(kf_strchr::impl_call_pre): Likewise for the first param.
	(kf_strcpy::impl_call_pre): Likewise for the second param.
	(kf_strdup::impl_call_pre): Likewise for the sole param.
	* region-model.cc (get_strlen): New.
	(struct call_arg_details): New.
	(inform_about_expected_null_terminated_string_arg): New.
	(class unterminated_string_arg): New.
	(region_model::check_for_null_terminated_string_arg): New.
	* region-model.h
	(region_model::check_for_null_terminated_string_arg): New decl.

gcc/ChangeLog:
	PR analyzer/105899
	* doc/analyzer.texi (__analyzer_get_strlen): New.
	* doc/invoke.texi: Add -Wanalyzer-unterminated-string.

gcc/testsuite/ChangeLog:
	PR analyzer/105899
	* gcc.dg/analyzer/analyzer-decls.h (__analyzer_get_strlen): New.
	* gcc.dg/analyzer/error-1.c (test_error_unterminated): New.
	(test_error_at_line_unterminated): New.
	* gcc.dg/analyzer/null-terminated-strings-1.c: New test.
	* gcc.dg/analyzer/putenv-1.c (test_unterminated): New.
	* gcc.dg/analyzer/strchr-1.c (test_unterminated): New.
	* gcc.dg/analyzer/strcpy-1.c (test_unterminated): New.
	* gcc.dg/analyzer/strdup-1.c (test_unterminated): New.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>

16 files changed, 301 insertions, 0 deletions

diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
index 2760aaa..a9bac40 100644
--- a/gcc/analyzer/analyzer.opt
+++ b/gcc/analyzer/analyzer.opt
@@ -214,6 +214,10 @@ Wanalyzer-tainted-size
 Common Var(warn_analyzer_tainted_size) Init(1) Warning
 Warn about code paths in which an unsanitized value is used as a size.
 
+Wanalyzer-unterminated-string
+Common Var(warn_analyzer_unterminated_string) Init(1) Warning
+Warn about code paths which attempt to find the length of an unterminated string.
+
 Wanalyzer-use-after-free
 Common Var(warn_analyzer_use_after_free) Init(1) Warning
 Warn about code paths in which a freed value is used.
diff --git a/gcc/analyzer/call-details.cc b/gcc/analyzer/call-details.cc
index 93f4846..fa86f55 100644
--- a/gcc/analyzer/call-details.cc
+++ b/gcc/analyzer/call-details.cc
@@ -376,6 +376,13 @@ call_details::lookup_function_attribute (const char *attr_name) const
   return lookup_attribute (attr_name, TYPE_ATTRIBUTES (allocfntype));
 }
 
+void
+call_details::check_for_null_terminated_string_arg (unsigned arg_idx) const
+{
+  region_model *model = get_model ();
+  model->check_for_null_terminated_string_arg (*this, arg_idx);
+}
+
 } // namespace ana
 
 #endif /* #if ENABLE_ANALYZER */
diff --git a/gcc/analyzer/call-details.h b/gcc/analyzer/call-details.h
index bf26011..0622cab 100644
--- a/gcc/analyzer/call-details.h
+++ b/gcc/analyzer/call-details.h
@@ -71,6 +71,8 @@ public:
 
   tree lookup_function_attribute (const char *attr_name) const;
 
+  void check_for_null_terminated_string_arg (unsigned arg_idx) const;
+
 private:
   const gcall *m_call;
   region_model *m_model;
diff --git a/gcc/analyzer/kf-analyzer.cc b/gcc/analyzer/kf-analyzer.cc
index 5aed007..1a0c940 100644
--- a/gcc/analyzer/kf-analyzer.cc
+++ b/gcc/analyzer/kf-analyzer.cc
@@ -358,6 +358,22 @@ public:
   }
 };
 
+/* Handler for "__analyzer_get_strlen".  */
+
+class kf_analyzer_get_strlen : public known_function
+{
+public:
+  bool matches_call_types_p (const call_details &cd) const final override
+  {
+    return cd.num_args () == 1 && cd.arg_is_pointer_p (0);
+  }
+  void impl_call_pre (const call_details &cd) const final override
+  {
+    cd.check_for_null_terminated_string_arg (0);
+    cd.set_any_lhs_with_defaults ();
+  }
+};
+
 /* Populate KFM with instances of known functions used for debugging the
    analyzer and for writing DejaGnu tests, all with a "__analyzer_" prefix.  */
 
@@ -379,6 +395,8 @@ register_known_analyzer_functions (known_function_manager &kfm)
   kfm.add ("__analyzer_eval", make_unique<kf_analyzer_eval> ());
   kfm.add ("__analyzer_get_unknown_ptr",
 	   make_unique<kf_analyzer_get_unknown_ptr> ());
+  kfm.add ("__analyzer_get_strlen",
+	   make_unique<kf_analyzer_get_strlen> ());
 }
 
 } // namespace ana
diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
index b9ee2e4..6b2db86 100644
--- a/gcc/analyzer/kf.cc
+++ b/gcc/analyzer/kf.cc
@@ -414,6 +414,10 @@ kf_error::impl_call_pre (const call_details &cd) const
   if (!model->add_constraint (status, EQ_EXPR, integer_zero_node, ctxt))
     if (ctxt)
       ctxt->terminate_path ();
+
+  /* Check "format" arg.  */
+  const int fmt_arg_idx = (m_min_args == 3) ? 2 : 4;
+  model->check_for_null_terminated_string_arg (cd, fmt_arg_idx);
 }
 
 /* Handler for "free", after sm-handling.
@@ -674,6 +678,7 @@ public:
     gcc_assert (fndecl);
     region_model_context *ctxt = cd.get_ctxt ();
     region_model *model = cd.get_model ();
+    model->check_for_null_terminated_string_arg (cd, 0);
     const svalue *ptr_sval = cd.get_arg_svalue (0);
     const region *reg
       = model->deref_rvalue (ptr_sval, cd.get_arg_tree (0), ctxt);
@@ -949,6 +954,10 @@ public:
   {
     return (cd.num_args () == 2 && cd.arg_is_pointer_p (0));
   }
+  void impl_call_pre (const call_details &cd) const final override
+  {
+    cd.check_for_null_terminated_string_arg (0);
+  }
   void impl_call_post (const call_details &cd) const final override;
 };
 
@@ -1109,6 +1118,7 @@ kf_strcpy::impl_call_pre (const call_details &cd) const
 					cd.get_ctxt ());
   const svalue *src_contents_sval = model->get_store_value (src_reg,
 							    cd.get_ctxt ());
+  cd.check_for_null_terminated_string_arg (1);
 
   cd.maybe_set_lhs (dest_sval);
 
@@ -1136,6 +1146,7 @@ public:
   {
     region_model *model = cd.get_model ();
     region_model_manager *mgr = cd.get_manager ();
+    cd.check_for_null_terminated_string_arg (0);
     /* Ideally we'd get the size here, and simulate copying the bytes.  */
     const region *new_reg
       = model->get_or_create_region_for_heap_alloc (NULL, cd.get_ctxt ());
diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index aa9fe00..494a9cd 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -3175,6 +3175,169 @@ region_model::set_value (tree lhs, tree rhs, region_model_context *ctxt)
   set_value (lhs_reg, rhs_sval, ctxt);
 }
 
+/* Look for the first 0 byte within STRING_CST.
+   If there is one, write its index to *OUT and return true.
+   Otherwise, return false.  */
+
+static bool
+get_strlen (tree string_cst, int *out)
+{
+  gcc_assert (TREE_CODE (string_cst) == STRING_CST);
+
+  if (const void *p = memchr (TREE_STRING_POINTER (string_cst),
+			      0,
+			      TREE_STRING_LENGTH (string_cst)))
+    {
+      *out = (const char *)p - TREE_STRING_POINTER (string_cst);
+      return true;
+    }
+  else
+    return false;
+}
+
+/* A bundle of information about a problematic argument at a callsite
+   for use by pending_diagnostic subclasses for reporting and
+   for deduplication.  */
+
+struct call_arg_details
+{
+public:
+  call_arg_details (const call_details &cd, unsigned arg_idx)
+  : m_call (cd.get_call_stmt ()),
+    m_called_fndecl (cd.get_fndecl_for_call ()),
+    m_arg_idx (arg_idx),
+    m_arg_expr (cd.get_arg_tree (arg_idx))
+  {
+  }
+
+  bool operator== (const call_arg_details &other) const
+  {
+    return (m_call == other.m_call
+	    && m_called_fndecl == other.m_called_fndecl
+	    && m_arg_idx == other.m_arg_idx
+	    && pending_diagnostic::same_tree_p (m_arg_expr, other.m_arg_expr));
+  }
+
+  const gcall *m_call;
+  tree m_called_fndecl;
+  unsigned m_arg_idx; // 0-based
+  tree m_arg_expr;
+};
+
+/* Issue a note specifying that a particular function parameter is expected
+   to be a valid null-terminated string.  */
+
+static void
+inform_about_expected_null_terminated_string_arg (const call_arg_details &ad)
+{
+  // TODO: ideally we'd underline the param here
+  inform (DECL_SOURCE_LOCATION (ad.m_called_fndecl),
+	  "argument %d of %qD must be a pointer to a null-terminated string",
+	  ad.m_arg_idx + 1, ad.m_called_fndecl);
+}
+
+/* A subclass of pending_diagnostic for complaining about uses
+   of unterminated strings (thus accessing beyond the bounds
+   of a buffer).  */
+
+class unterminated_string_arg
+: public pending_diagnostic_subclass<unterminated_string_arg>
+{
+public:
+  unterminated_string_arg (const call_arg_details arg_details)
+  : m_arg_details (arg_details)
+  {
+    gcc_assert (m_arg_details.m_called_fndecl);
+  }
+
+  const char *get_kind () const final override
+  {
+    return "unterminated_string_arg";
+  }
+
+  bool operator== (const unterminated_string_arg &other) const
+  {
+    return m_arg_details == other.m_arg_details;
+  }
+
+  int get_controlling_option () const final override
+  {
+    return OPT_Wanalyzer_unterminated_string;
+  }
+
+  bool emit (rich_location *rich_loc, logger *) final override
+  {
+    auto_diagnostic_group d;
+    bool warned;
+    if (m_arg_details.m_arg_expr)
+      warned = warning_at (rich_loc, get_controlling_option (),
+			   "passing pointer to unterminated string %qE"
+			   " as argument %i of %qE",
+			   m_arg_details.m_arg_expr,
+			   m_arg_details.m_arg_idx + 1,
+			   m_arg_details.m_called_fndecl);
+    else
+      warned = warning_at (rich_loc, get_controlling_option (),
+			   "passing pointer to unterminated string"
+			   " as argument %i of %qE",
+			   m_arg_details.m_arg_idx + 1,
+			   m_arg_details.m_called_fndecl);
+    if (warned)
+      inform_about_expected_null_terminated_string_arg (m_arg_details);
+    return warned;
+  }
+
+  label_text describe_final_event (const evdesc::final_event &ev) final override
+  {
+    return ev.formatted_print
+      ("passing pointer to unterminated buffer as argument %i of %qE"
+       " would lead to read past the end of the buffer",
+       m_arg_details.m_arg_idx + 1,
+       m_arg_details.m_called_fndecl);
+  }
+
+private:
+  const call_arg_details m_arg_details;
+};
+
+/* Check that argument ARG_IDX (0-based) to the call described by CD
+   is a pointer to a valid null-terminated string.
+
+   Complain if the buffer pointed to isn't null-terminated.
+
+   TODO: we should also complain if:
+   - the pointer is NULL (or could be)
+   - the buffer pointed to is uninitalized before any 0-terminator
+   - the 0-terminator is within the bounds of the underlying base region
+
+   We're checking that the called function could validly iterate through
+   the buffer reading it until it finds a 0 byte (such as by calling
+   strlen, or equivalent code).  */
+
+void
+region_model::check_for_null_terminated_string_arg (const call_details &cd,
+						    unsigned arg_idx)
+{
+  region_model_context *ctxt = cd.get_ctxt ();
+
+  const svalue *arg_sval = cd.get_arg_svalue (arg_idx);
+  const region *buf_reg
+    = deref_rvalue (arg_sval, cd.get_arg_tree (arg_idx), ctxt);
+
+  const svalue *contents_sval = get_store_value (buf_reg, ctxt);
+
+  if (tree cst = contents_sval->maybe_get_constant ())
+    if (TREE_CODE (cst) == STRING_CST)
+      {
+	int cst_strlen;
+	if (!get_strlen (cst, &cst_strlen))
+	  {
+	    call_arg_details arg_details (cd, arg_idx);
+	    ctxt->warn (make_unique<unterminated_string_arg> (arg_details));
+	  }
+      }
+}
+
 /* Remove all bindings overlapping REG within the store.  */
 
 void
diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
index a8acad8..4f09f2e 100644
--- a/gcc/analyzer/region-model.h
+++ b/gcc/analyzer/region-model.h
@@ -502,6 +502,9 @@ class region_model
 			       const svalue *sval_hint,
 			       region_model_context *ctxt) const;
 
+  void check_for_null_terminated_string_arg (const call_details &cd,
+					     unsigned idx);
+
 private:
   const region *get_lvalue_1 (path_var pv, region_model_context *ctxt) const;
   const svalue *get_rvalue_1 (path_var pv, region_model_context *ctxt) const;
diff --git a/gcc/doc/analyzer.texi b/gcc/doc/analyzer.texi
index 2692b0e..c50d7eb 100644
--- a/gcc/doc/analyzer.texi
+++ b/gcc/doc/analyzer.texi
@@ -652,6 +652,14 @@ __analyzer_get_unknown_ptr ();
 @end smallexample
 will obtain an unknown @code{void *}.
 
+@item __analyzer_get_strlen
+@smallexample
+__analyzer_get_strlen (buf);
+@end smallexample
+will emit a warning if PTR doesn't point to a null-terminated string.
+TODO: eventually get the strlen of the buffer (without the
+optimizer touching it).
+
 @end table
 
 @subsection Other Debugging Techniques
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 5bfdd6e..10f3466 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -486,6 +486,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wno-analyzer-tainted-size
 -Wanalyzer-too-complex
 -Wno-analyzer-unsafe-call-within-signal-handler
+-Wno-analyzer-unterminated-string
 -Wno-analyzer-use-after-free
 -Wno-analyzer-use-of-pointer-in-stale-stack-frame
 -Wno-analyzer-use-of-uninitialized-value
@@ -10319,6 +10320,7 @@ Enabling this option effectively enables the following warnings:
 -Wanalyzer-shift-count-overflow
 -Wanalyzer-stale-setjmp-buffer
 -Wanalyzer-unsafe-call-within-signal-handler
+-Wanalyzer-unterminated-string
 -Wanalyzer-use-after-free
 -Wanalyzer-use-of-pointer-in-stale-stack-frame
 -Wanalyzer-use-of-uninitialized-value
@@ -10908,6 +10910,17 @@ called from a signal handler.
 
 See @uref{https://cwe.mitre.org/data/definitions/479.html, CWE-479: Signal Handler Use of a Non-reentrant Function}.
 
+@opindex Wanalyzer-unterminated-string
+@opindex Wno-analyzer-unterminated-string
+@item -Wno-analyzer-unterminated-string
+This warning requires @option{-fanalyzer}, which enables it; use
+@option{-Wno-analyzer-unterminated-string} to disable it.
+
+This diagnostic warns about code paths which attempt to find the length
+of an unterminated string.  For example, passing a pointer to an unterminated
+buffer to @code{strlen} would lead to accesses beyond the end of the buffer
+whilst attempting to find the terminating zero character.
+
 @opindex Wanalyzer-use-after-free
 @opindex Wno-analyzer-use-after-free
 @item -Wno-analyzer-use-after-free
diff --git a/gcc/testsuite/gcc.dg/analyzer/analyzer-decls.h b/gcc/testsuite/gcc.dg/analyzer/analyzer-decls.h
index d9a32ed..a626728 100644
--- a/gcc/testsuite/gcc.dg/analyzer/analyzer-decls.h
+++ b/gcc/testsuite/gcc.dg/analyzer/analyzer-decls.h
@@ -53,4 +53,9 @@ extern void __analyzer_eval (int);
 /* Obtain an "unknown" void *.  */
 extern void *__analyzer_get_unknown_ptr (void);
 
+/* Complain if PTR doesn't point to a null-terminated string.
+   TODO: eventually get the strlen of the buffer (without the
+   optimizer touching it).  */
+extern __SIZE_TYPE__ __analyzer_get_strlen (const char *ptr);
+
 #endif /* #ifndef ANALYZER_DECLS_H.  */
diff --git a/gcc/testsuite/gcc.dg/analyzer/error-1.c b/gcc/testsuite/gcc.dg/analyzer/error-1.c
index f82a4cd..491d615 100644
--- a/gcc/testsuite/gcc.dg/analyzer/error-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/error-1.c
@@ -64,3 +64,15 @@ void test_6 (int st, const char *str)
   error (st, errno, "test: %s", str);
   __analyzer_eval (st == 0); /* { dg-warning "TRUE" } */
 }
+
+char *test_error_unterminated (int st)
+{
+  char fmt[3] = "abc";
+  error (st, errno, fmt); /* { dg-warning "passing pointer to unterminated string '&fmt' as argument 3 of 'error'" } */
+}
+
+char *test_error_at_line_unterminated (int st, int errno)
+{
+  char fmt[3] = "abc";
+  error_at_line (st, errno, __FILE__, __LINE__, fmt); /* { dg-warning "passing pointer to unterminated string '&fmt' as argument 5 of 'error_at_line'" } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/null-terminated-strings-1.c b/gcc/testsuite/gcc.dg/analyzer/null-terminated-strings-1.c
new file mode 100644
index 0000000..3379870
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/null-terminated-strings-1.c
@@ -0,0 +1,30 @@
+#include "analyzer-decls.h"
+
+#define NULL ((void *)0)
+typedef __SIZE_TYPE__ size_t;
+
+void test_terminated (void)
+{
+  __analyzer_get_strlen ("abc"); /* { dg-bogus "" } */
+}
+
+void test_unterminated (void)
+{
+  char buf[3] = "abc";
+  __analyzer_get_strlen (buf); /* { dg-warning "passing pointer to unterminated string '&buf' as argument 1 of '__analyzer_get_strlen'" } */
+}
+
+void test_embedded_nul (void)
+{
+  char buf[3] = "a\0c";
+  __analyzer_get_strlen (buf); /* { dg-bogus "" } */
+}
+
+void test_fully_initialized_but_unterminated (void)
+{
+  char buf[3];
+  buf[0] = 'a';
+  buf[1] = 'b';
+  buf[2] = 'c';
+  __analyzer_get_strlen (buf); /* { dg-warning "passing pointer to unterminated string '&buf' as argument 1 of '__analyzer_get_strlen'" "" { xfail *-*-* } } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/putenv-1.c b/gcc/testsuite/gcc.dg/analyzer/putenv-1.c
index 5431212..5fa2033 100644
--- a/gcc/testsuite/gcc.dg/analyzer/putenv-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/putenv-1.c
@@ -108,3 +108,10 @@ void test_outer (void)
   char arr_outer[] = "NAME=VALUE"; /* { dg-message "'arr_outer' declared on stack here" } */
   __analyzer_test_inner (arr_outer);
 }
+
+void test_unterminated (void)
+{
+  char buf[3] = "abc";
+  putenv (buf); /* { dg-warning "passing pointer to unterminated string" } */
+  /* { dg-warning "'putenv' on a pointer to automatic variable 'buf'" "POS34-C" { target *-*-* } .-1 } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/strchr-1.c b/gcc/testsuite/gcc.dg/analyzer/strchr-1.c
index bfa4891..2fb6c76 100644
--- a/gcc/testsuite/gcc.dg/analyzer/strchr-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/strchr-1.c
@@ -25,3 +25,9 @@ void test_3 (const char *s, int c)
   char *p = strchr (s, c); /* { dg-message "when 'strchr' returns NULL"} */
   *p = 'A'; /* { dg-warning "dereference of NULL 'p'" "null deref" } */
 }
+
+void test_unterminated (int c)
+{
+  char buf[3] = "abc";
+  strchr (buf, c); /* { dg-warning "passing pointer to unterminated string '&buf' as argument 1 of 'strchr'" } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/strcpy-1.c b/gcc/testsuite/gcc.dg/analyzer/strcpy-1.c
index ed5bab9..f23dd69 100644
--- a/gcc/testsuite/gcc.dg/analyzer/strcpy-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/strcpy-1.c
@@ -16,3 +16,9 @@ test_1a (char *dst, char *src)
   __analyzer_eval (result == dst); /* { dg-warning "TRUE" } */
   return result;
 }
+
+char *test_unterminated (char *dst)
+{
+  char buf[3] = "abc";
+  return strcpy (dst, buf); /* { dg-warning "passing pointer to unterminated string '&buf' as argument 2 of 'strcpy'" } */
+}
diff --git a/gcc/testsuite/gcc.dg/analyzer/strdup-1.c b/gcc/testsuite/gcc.dg/analyzer/strdup-1.c
index 9ac3921..682bfb9 100644
--- a/gcc/testsuite/gcc.dg/analyzer/strdup-1.c
+++ b/gcc/testsuite/gcc.dg/analyzer/strdup-1.c
@@ -38,3 +38,9 @@ void test_6 (const char *s)
   char *p = __builtin_strdup (s); /* { dg-message "this call could return NULL" } */
   requires_nonnull (p); /* { dg-warning "use of possibly-NULL 'p'" } */
 }
+
+char *test_unterminated (void)
+{
+  char buf[3] = "abc";
+  return strdup (buf); /* { dg-warning "passing pointer to unterminated string '&buf' as argument 1 of 'strdup'" } */
+}