Re: [committed] analyzer: fix missing -Wanalyzer-write-to-const [PR102695]

[Martin Sebor <msebor@gmail.com>]

On 11/16/21 7:05 PM, David Malcolm via Gcc-patches wrote:
> This patch fixes -Wanalyzer-write-to-const so that it will complain
> about attempts to write to functions, to labels.
> It also "teaches" the analyzer about strchr, in that strchr can either
> return a pointer into the input area (and thus -Wanalyzer-write-to-const
> can now complain about writes into a string literal seen this way),
> or return NULL (and thus the analyzer can complain about NULL
> dereferences if the result is used without a check).

Fow what it's worth, I used strchr in the test case as an example.
There are a few other built-ins like it, including index, rindex,
memchr, strrchr, and strstr (just going through the switch
statements in my code).

At least some of these built-ins have an attribute "fn spec" that
describes some of their properties (like what argument they read
from; see builtin_fnspec in builtins.c).  But it doesn't look
like attr_fnspec has a way of encoding a function that returns
a pointer argument plus some offset.  That seems like a useful
enhancement both for our work and also for optimizers.  It would
let us avoid having to hardcode these properties in duplicate
case and switch statements in multiple places.

Martin

> 
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> Pushed to trunk as r12-5330-g111fd515f2894d7cddf62f80c69765c43ae18577.
> 
> gcc/analyzer/ChangeLog:
> 	PR analyzer/102695
> 	* region-model-impl-calls.cc (region_model::impl_call_strchr): New.
> 	* region-model-manager.cc
> 	(region_model_manager::maybe_fold_unaryop): Simplify cast to
> 	pointer type of an existing pointer to a region.
> 	* region-model.cc (region_model::on_call_pre): Handle
> 	BUILT_IN_STRCHR and "strchr".
> 	(write_to_const_diagnostic::emit): Add auto_diagnostic_group.  Add
> 	alternate wordings for functions and labels.
> 	(write_to_const_diagnostic::describe_final_event): Add alternate
> 	wordings for functions and labels.
> 	(region_model::check_for_writable_region): Handle RK_FUNCTION and
> 	RK_LABEL.
> 	* region-model.h (region_model::impl_call_strchr): New decl.
> 
> gcc/testsuite/ChangeLog:
> 	PR analyzer/102695
> 	* gcc.dg/analyzer/pr102695.c: New test.
> 	* gcc.dg/analyzer/strchr-1.c: New test.
> 
> Signed-off-by: David Malcolm <dmalcolm@redhat.com>
> ---
>   gcc/analyzer/region-model-impl-calls.cc  | 69 ++++++++++++++++++++++++
>   gcc/analyzer/region-model-manager.cc     |  7 +++
>   gcc/analyzer/region-model.cc             | 52 ++++++++++++++++--
>   gcc/analyzer/region-model.h              |  1 +
>   gcc/testsuite/gcc.dg/analyzer/pr102695.c | 44 +++++++++++++++
>   gcc/testsuite/gcc.dg/analyzer/strchr-1.c | 26 +++++++++
>   6 files changed, 196 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr102695.c
>   create mode 100644 gcc/testsuite/gcc.dg/analyzer/strchr-1.c
> 
> diff --git a/gcc/analyzer/region-model-impl-calls.cc b/gcc/analyzer/region-model-impl-calls.cc
> index 90d4cf9c2db..ae50e69542e 100644
> --- a/gcc/analyzer/region-model-impl-calls.cc
> +++ b/gcc/analyzer/region-model-impl-calls.cc
> @@ -678,6 +678,75 @@ region_model::impl_call_realloc (const call_details &cd)
>       }
>   }
>   
> +/* Handle the on_call_pre part of "strchr" and "__builtin_strchr".  */
> +
> +void
> +region_model::impl_call_strchr (const call_details &cd)
> +{
> +  class strchr_call_info : public call_info
> +  {
> +  public:
> +    strchr_call_info (const call_details &cd, bool found)
> +    : call_info (cd), m_found (found)
> +    {
> +    }
> +
> +    label_text get_desc (bool can_colorize) const FINAL OVERRIDE
> +    {
> +      if (m_found)
> +	return make_label_text (can_colorize,
> +				"when %qE returns non-NULL",
> +				get_fndecl ());
> +      else
> +	return make_label_text (can_colorize,
> +				"when %qE returns NULL",
> +				get_fndecl ());
> +    }
> +
> +    bool update_model (region_model *model,
> +		       const exploded_edge *,
> +		       region_model_context *ctxt) const FINAL OVERRIDE
> +    {
> +      const call_details cd (get_call_details (model, ctxt));
> +      if (tree lhs_type = cd.get_lhs_type ())
> +	{
> +	  region_model_manager *mgr = model->get_manager ();
> +	  const svalue *result;
> +	  if (m_found)
> +	    {
> +	      const svalue *str_sval = cd.get_arg_svalue (0);
> +	      const region *str_reg
> +		= model->deref_rvalue (str_sval, cd.get_arg_tree (0),
> +				       cd.get_ctxt ());
> +	      /* We want str_sval + OFFSET for some unknown OFFSET.
> +		 Use a conjured_svalue to represent the offset,
> +		 using the str_reg as the id of the conjured_svalue.  */
> +	      const svalue *offset
> +		= mgr->get_or_create_conjured_svalue (size_type_node,
> +						      cd.get_call_stmt (),
> +						      str_reg);
> +	      result = mgr->get_or_create_binop (lhs_type, POINTER_PLUS_EXPR,
> +						 str_sval, offset);
> +	    }
> +	  else
> +	    result = mgr->get_or_create_int_cst (lhs_type, 0);
> +	  cd.maybe_set_lhs (result);
> +	}
> +      return true;
> +    }
> +  private:
> +    bool m_found;
> +  };
> +
> +  /* Bifurcate state, creating a "not found" out-edge.  */
> +  if (cd.get_ctxt ())
> +    cd.get_ctxt ()->bifurcate (new strchr_call_info (cd, false));
> +
> +  /* The "unbifurcated" state is the "found" case.  */
> +  strchr_call_info found (cd, true);
> +  found.update_model (this, NULL, cd.get_ctxt ());
> +}
> +
>   /* Handle the on_call_pre part of "strcpy" and "__builtin_strcpy_chk".  */
>   
>   void
> diff --git a/gcc/analyzer/region-model-manager.cc b/gcc/analyzer/region-model-manager.cc
> index 1cdec1bd230..fdf32122dea 100644
> --- a/gcc/analyzer/region-model-manager.cc
> +++ b/gcc/analyzer/region-model-manager.cc
> @@ -380,6 +380,13 @@ region_model_manager::maybe_fold_unaryop (tree type, enum tree_code op,
>   		    == boolean_true_node))
>   	      return maybe_fold_unaryop (type, op, innermost_arg);
>   	  }
> +	/* Avoid creating symbolic regions for pointer casts by
> +	   simplifying (T*)(&REGION) to ((T*)&REGION).  */
> +	if (const region_svalue *region_sval = arg->dyn_cast_region_svalue ())
> +	  if (POINTER_TYPE_P (type)
> +	      && region_sval->get_type ()
> +	      && POINTER_TYPE_P (region_sval->get_type ()))
> +	    return get_ptr_svalue (type, region_sval->get_pointee ());
>         }
>         break;
>       case TRUTH_NOT_EXPR:
> diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
> index 416a5ac7249..bbb15abdfe2 100644
> --- a/gcc/analyzer/region-model.cc
> +++ b/gcc/analyzer/region-model.cc
> @@ -1133,6 +1133,9 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt,
>   	    break;
>   	  case BUILT_IN_REALLOC:
>   	    return false;
> +	  case BUILT_IN_STRCHR:
> +	    impl_call_strchr (cd);
> +	    return false;
>   	  case BUILT_IN_STRCPY:
>   	  case BUILT_IN_STRCPY_CHK:
>   	    impl_call_strcpy (cd);
> @@ -1225,6 +1228,12 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt,
>   	  impl_call_memset (cd);
>   	  return false;
>   	}
> +      else if (is_named_call_p (callee_fndecl, "strchr", call, 2)
> +	       && POINTER_TYPE_P (cd.get_arg_type (0)))
> +	{
> +	  impl_call_strchr (cd);
> +	  return false;
> +	}
>         else if (is_named_call_p (callee_fndecl, "strlen", call, 1)
>   	       && POINTER_TYPE_P (cd.get_arg_type (0)))
>   	{
> @@ -2161,8 +2170,23 @@ public:
>   
>     bool emit (rich_location *rich_loc) FINAL OVERRIDE
>     {
> -    bool warned = warning_at (rich_loc, OPT_Wanalyzer_write_to_const,
> -			      "write to %<const%> object %qE", m_decl);
> +    auto_diagnostic_group d;
> +    bool warned;
> +    switch (m_reg->get_kind ())
> +      {
> +      default:
> +	warned = warning_at (rich_loc, OPT_Wanalyzer_write_to_const,
> +			     "write to %<const%> object %qE", m_decl);
> +	break;
> +      case RK_FUNCTION:
> +	warned = warning_at (rich_loc, OPT_Wanalyzer_write_to_const,
> +			     "write to function %qE", m_decl);
> +	break;
> +      case RK_LABEL:
> +	warned = warning_at (rich_loc, OPT_Wanalyzer_write_to_const,
> +			     "write to label %qE", m_decl);
> +	break;
> +      }
>       if (warned)
>         inform (DECL_SOURCE_LOCATION (m_decl), "declared here");
>       return warned;
> @@ -2170,7 +2194,15 @@ public:
>   
>     label_text describe_final_event (const evdesc::final_event &ev) FINAL OVERRIDE
>     {
> -    return ev.formatted_print ("write to %<const%> object %qE here", m_decl);
> +    switch (m_reg->get_kind ())
> +      {
> +      default:
> +	return ev.formatted_print ("write to %<const%> object %qE here", m_decl);
> +      case RK_FUNCTION:
> +	return ev.formatted_print ("write to function %qE here", m_decl);
> +      case RK_LABEL:
> +	return ev.formatted_print ("write to label %qE here", m_decl);
> +      }
>     }
>   
>   private:
> @@ -2231,6 +2263,20 @@ region_model::check_for_writable_region (const region* dest_reg,
>       {
>       default:
>         break;
> +    case RK_FUNCTION:
> +      {
> +	const function_region *func_reg = as_a <const function_region *> (base_reg);
> +	tree fndecl = func_reg->get_fndecl ();
> +	ctxt->warn (new write_to_const_diagnostic (func_reg, fndecl));
> +      }
> +      break;
> +    case RK_LABEL:
> +      {
> +	const label_region *label_reg = as_a <const label_region *> (base_reg);
> +	tree label = label_reg->get_label ();
> +	ctxt->warn (new write_to_const_diagnostic (label_reg, label));
> +      }
> +      break;
>       case RK_DECL:
>         {
>   	const decl_region *decl_reg = as_a <const decl_region *> (base_reg);
> diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h
> index 13e8109aa51..543401167ae 100644
> --- a/gcc/analyzer/region-model.h
> +++ b/gcc/analyzer/region-model.h
> @@ -586,6 +586,7 @@ class region_model
>     void impl_call_memcpy (const call_details &cd);
>     void impl_call_memset (const call_details &cd);
>     void impl_call_realloc (const call_details &cd);
> +  void impl_call_strchr (const call_details &cd);
>     void impl_call_strcpy (const call_details &cd);
>     void impl_call_strlen (const call_details &cd);
>     void impl_call_operator_new (const call_details &cd);
> diff --git a/gcc/testsuite/gcc.dg/analyzer/pr102695.c b/gcc/testsuite/gcc.dg/analyzer/pr102695.c
> new file mode 100644
> index 00000000000..2ca988254fe
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/pr102695.c
> @@ -0,0 +1,44 @@
> +extern void* malloc (__SIZE_TYPE__);
> +
> +const char* write_strchr_literal (int x)
> +{
> +  char *p = __builtin_strchr ("123", x);
> +  *p = 0; /* { dg-warning "dereference of NULL 'p'" "null deref" } */
> +  /* { dg-warning "write to string literal" "string literal" { target *-*-* } .-1 } */
> +  return p;
> +}
> +
> +const char* write_strchr_const_array (int x)
> +{
> +  static const char a[] = "123";
> +  char *p = __builtin_strchr (a, x);
> +  *p = 0; /* { dg-warning "dereference of NULL 'p'" "null deref" } */
> +  /* { dg-warning "write to 'const' object 'a'" "write to const" { target *-*-* } .-1 } */
> +  return a;
> +}
> +
> +char* write_function (void)
> +{
> +  char *p = (char*)malloc /* forgot arguments */;
> +  p[1] = 'a'; /* { dg-warning "write to function 'malloc'" } */
> +  __builtin_strcpy (p, "123");  /* { dg-warning "write to function 'malloc'" } */
> +  return p;
> +}
> +
> +char* write_label (void)
> +{
> +  char *p = (char*)&&L;
> +  *p = 0; /* { dg-warning "write to label 'L'" } */
> +L:
> +  return p;
> +}
> +
> +struct A { const int i; };
> +
> +extern /* not const */ struct A a;
> +
> +void write_const_member (void)
> +{
> +  char *p = (char*)&a.i;
> +  *p = 0;   // missing warning
> +}
> diff --git a/gcc/testsuite/gcc.dg/analyzer/strchr-1.c b/gcc/testsuite/gcc.dg/analyzer/strchr-1.c
> new file mode 100644
> index 00000000000..dfe1bc9ea1a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/analyzer/strchr-1.c
> @@ -0,0 +1,26 @@
> +#include <string.h>
> +#include "analyzer-decls.h"
> +
> +const char* test_literal (int x)
> +{
> +  char *p = __builtin_strchr ("123", x);
> +  if (p)
> +    {
> +      __analyzer_eval (*p == x); /* { dg-message "UNKNOWN" } */
> +      /* TODO: this ought to be TRUE, but it's unclear that it's
> +	 worth stashing this constraint.  */
> +    }
> +  return p;
> +}
> +
> +void test_2 (const char *s, int c)
> +{
> +  char *p = __builtin_strchr (s, c); /* { dg-message "when '__builtin_strchr' returns NULL"} */
> +  *p = 'A'; /* { dg-warning "dereference of NULL 'p'" "null deref" } */
> +}
> +
> +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" } */
> +}
>