analyzer: fix leak false positives on "*UNKNOWN = PTR;" [PR108252]

PR analyzer/108252 reports a false positive from -Wanalyzer-malloc-leak on
code like this:

  *ptr_ptr = strdup(EXPR);

where ptr_ptr is an UNKNOWN_VALUE.

When we handle:
  *UNKNOWN = PTR;
store::set_value normally marks *PTR as having escaped, and this means
we don't report PTR as leaking when the last usage of PTR is lost.

However this only works for cases where PTR is a region_svalue.
In the example in the bug, it's a conjured_svalue, rather than a
region_svalue.  A similar problem can arise for FDs, which aren't
pointers.

This patch fixes the bug by updating store::set_value to mark any
values stored via *UNKNOWN = VAL as not leaking.

Additionally, sm-malloc.cc's known_allocator_p hardcodes strdup and
strndup as allocators (and thus transitioning their result to
"unchecked"), but we don't implement known_functions for these, leading
to the LHS being a CONJURED_SVALUE, rather than a region_svalue to a
heap-allocated region.  A similar issue happens with functions marked
with __attribute__((malloc)).  As part of a "belt and braces" fix, the
patch also updates the handling of these functions, so that they use
heap-allocated regions.

gcc/analyzer/ChangeLog:
	PR analyzer/108252
	* kf.cc (class kf_strdup): New.
	(class kf_strndup): New.
	(register_known_functions): Register them.
	* region-model.cc (region_model::on_call_pre): Use
	&HEAP_ALLOCATED_REGION for the default result of an external
	function with the "malloc" attribute, rather than CONJURED_SVALUE.
	(region_model::get_or_create_region_for_heap_alloc): Allow
	"size_in_bytes" to be NULL.
	* store.cc (store::set_value): When handling *UNKNOWN = VAL,
	mark VAL as "maybe bound".

gcc/testsuite/ChangeLog:
	PR analyzer/108252
	* gcc.dg/analyzer/attr-malloc-pr108252.c: New test.
	* gcc.dg/analyzer/fd-leak-pr108252.c: New test.
	* gcc.dg/analyzer/flex-with-call-summaries.c: Remove xfail from
	warning false +ve directives.
	* gcc.dg/analyzer/pr103217-2.c: Add -Wno-analyzer-too-complex.
	* gcc.dg/analyzer/pr103217-3.c: Likewise.
	* gcc.dg/analyzer/strdup-pr108252.c: New test.
	* gcc.dg/analyzer/strndup-pr108252.c: New test.

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

diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
index 6088bfc..53190c5 100644 (file)
--- a/gcc/analyzer/kf.cc
+++ b/gcc/analyzer/kf.cc
@@ -851,6 +851,32 @@ kf_strcpy::impl_call_pre (const call_details &cd) const
   model->set_value (sized_dest_reg, src_contents_sval, cd.get_ctxt ());
 }
 
+/* Handler for "strdup" and "__builtin_strdup".  */
+
+class kf_strdup : 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
+  {
+    region_model *model = cd.get_model ();
+    region_model_manager *mgr = cd.get_manager ();
+    /* 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 ());
+    model->mark_region_as_unknown (new_reg, NULL);
+    if (cd.get_lhs_type ())
+      {
+       const svalue *ptr_sval
+         = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+       cd.maybe_set_lhs (ptr_sval);
+      }
+  }
+};
+
 /* Handle the on_call_pre part of "strlen".  */
 
 class kf_strlen : public known_function
@@ -892,6 +918,32 @@ kf_strlen::impl_call_pre (const call_details &cd) const
   /* Otherwise a conjured value.  */
 }
 
+/* Handler for "strndup" and "__builtin_strndup".  */
+
+class kf_strndup : public known_function
+{
+public:
+  bool matches_call_types_p (const call_details &cd) const final override
+  {
+    return (cd.num_args () == 2 && cd.arg_is_pointer_p (0));
+  }
+  void impl_call_pre (const call_details &cd) const final override
+  {
+    region_model *model = cd.get_model ();
+    region_model_manager *mgr = cd.get_manager ();
+    /* 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 ());
+    model->mark_region_as_unknown (new_reg, NULL);
+    if (cd.get_lhs_type ())
+      {
+       const svalue *ptr_sval
+         = mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+       cd.maybe_set_lhs (ptr_sval);
+      }
+  }
+};
+
 class kf_ubsan_bounds : public internal_known_function
 {
   /* Empty.  */
@@ -943,6 +995,8 @@ register_known_functions (known_function_manager &kfm)
     kfm.add (BUILT_IN_STRCHR, make_unique<kf_strchr> ());
     kfm.add (BUILT_IN_STRCPY, make_unique<kf_strcpy> (2));
     kfm.add (BUILT_IN_STRCPY_CHK, make_unique<kf_strcpy> (3));
+    kfm.add (BUILT_IN_STRDUP, make_unique<kf_strdup> ());
+    kfm.add (BUILT_IN_STRNDUP, make_unique<kf_strndup> ());
     kfm.add (BUILT_IN_STRLEN, make_unique<kf_strlen> ());
 
     register_varargs_builtins (kfm);
@@ -951,6 +1005,8 @@ register_known_functions (known_function_manager &kfm)
   /* Known builtins and C standard library functions.  */
   {
     kfm.add ("memset", make_unique<kf_memset> ());
+    kfm.add ("strdup", make_unique<kf_strdup> ());
+    kfm.add ("strndup", make_unique<kf_strndup> ());
   }
 
   /* Known POSIX functions, and some non-standard extensions.  */

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index 5506440..2e59fba 100644 (file)
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -1441,6 +1441,8 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt)
   if (ctxt)
     check_call_args (cd);
 
+  tree callee_fndecl = get_fndecl_for_call (call, ctxt);
+
   /* Some of the cases below update the lhs of the call based on the
      return value, but not all.  Provide a default value, which may
      get overwritten below.  */
@@ -1450,13 +1452,22 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt)
       const svalue *sval = maybe_get_const_fn_result (cd);
       if (!sval)
        {
-         /* For the common case of functions without __attribute__((const)),
-            use a conjured value, and purge any prior state involving that
-            value (in case this is in a loop).  */
-         sval = m_mgr->get_or_create_conjured_svalue (TREE_TYPE (lhs), call,
-                                                      lhs_region,
-                                                      conjured_purge (this,
-                                                                      ctxt));
+         if (callee_fndecl
+             && lookup_attribute ("malloc", DECL_ATTRIBUTES (callee_fndecl)))
+           {
+             const region *new_reg
+               = get_or_create_region_for_heap_alloc (NULL, ctxt);
+             mark_region_as_unknown (new_reg, NULL);
+             sval = m_mgr->get_ptr_svalue (cd.get_lhs_type (), new_reg);
+           }
+         else
+           /* For the common case of functions without __attribute__((const)),
+              use a conjured value, and purge any prior state involving that
+              value (in case this is in a loop).  */
+           sval = m_mgr->get_or_create_conjured_svalue (TREE_TYPE (lhs), call,
+                                                        lhs_region,
+                                                        conjured_purge (this,
+                                                                        ctxt));
        }
       set_value (lhs_region, sval, ctxt);
     }
@@ -1469,7 +1480,7 @@ region_model::on_call_pre (const gcall *call, region_model_context *ctxt)
        return false;
       }
 
-  if (tree callee_fndecl = get_fndecl_for_call (call, ctxt))
+  if (callee_fndecl)
     {
       int callee_fndecl_flags = flags_from_decl_or_type (callee_fndecl);
 
@@ -4909,8 +4920,9 @@ region_model::get_or_create_region_for_heap_alloc (const svalue *size_in_bytes,
   get_referenced_base_regions (base_regs_in_use);
   const region *reg
     = m_mgr->get_or_create_region_for_heap_alloc (base_regs_in_use);
-  if (compat_types_p (size_in_bytes->get_type (), size_type_node))
-    set_dynamic_extents (reg, size_in_bytes, ctxt);
+  if (size_in_bytes)
+    if (compat_types_p (size_in_bytes->get_type (), size_type_node))
+      set_dynamic_extents (reg, size_in_bytes, ctxt);
   return reg;
 }
 

diff --git a/gcc/analyzer/store.cc b/gcc/analyzer/store.cc
index f3b500c..6ad7110 100644 (file)
--- a/gcc/analyzer/store.cc
+++ b/gcc/analyzer/store.cc
@@ -2578,6 +2578,8 @@ store::set_value (store_manager *mgr, const region *lhs_reg,
          const region *ptr_base_reg = ptr_dst->get_base_region ();
          mark_as_escaped (ptr_base_reg);
        }
+      if (uncertainty)
+       uncertainty->on_maybe_bound_sval (rhs_sval);
     }
   else if (lhs_base_reg->tracked_p ())
     {

diff --git a/gcc/testsuite/gcc.dg/analyzer/attr-malloc-pr108252.c b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-pr108252.c
new file mode 100644 (file)
index 0000000..5e34379
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/attr-malloc-pr108252.c
@@ -0,0 +1,25 @@
+struct foo
+{
+  int m_int;
+};
+
+extern void foo_release (struct foo *);
+extern struct foo *foo_acquire (void)
+  __attribute__ ((malloc (foo_release)));
+
+struct {
+  /* [...snip...] */
+  struct foo *listen_default_ciphers;
+  struct foo *connect_default_ciphers;
+  /* [...snip...] */
+} g;
+
+int parse_global_ciphers(char **args)
+{
+  struct foo **target;
+  target = ((args[0][12] == 'b')
+           ? &g.listen_default_ciphers
+           : &g.connect_default_ciphers);
+  *target = foo_acquire ();
+  return 0; /* { dg-bogus "leak" } */
+}

diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-leak-pr108252.c b/gcc/testsuite/gcc.dg/analyzer/fd-leak-pr108252.c
new file mode 100644 (file)
index 0000000..29f58d3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/fd-leak-pr108252.c
@@ -0,0 +1,15 @@
+extern int open(const char *, int mode);
+#define O_RDONLY 0
+
+struct {
+  int fd_a;
+  int fd_b;
+} g;
+
+int test (const char *path, int flag)
+{
+  int *target;
+  target = flag ? &g.fd_a : &g.fd_b;
+  *target = open (path, O_RDONLY);  
+  return 0; /* { dg-bogus "leak of file descriptor" } */
+}

diff --git a/gcc/testsuite/gcc.dg/analyzer/flex-with-call-summaries.c b/gcc/testsuite/gcc.dg/analyzer/flex-with-call-summaries.c
index 00566d5..0ff652b 100644 (file)
--- a/gcc/testsuite/gcc.dg/analyzer/flex-with-call-summaries.c
+++ b/gcc/testsuite/gcc.dg/analyzer/flex-with-call-summaries.c
@@ -913,8 +913,7 @@ static int yy_get_next_buffer (void)
                if ( number_to_move == YY_MORE_ADJ )
                        {
                        ret_val = EOB_ACT_END_OF_FILE;
-                       yyrestart( yyin  );/* { dg-bogus "leak" "" { xfail *-*-* } } */
-                       /* TODO: leak false positive: PR analyzer/103546.  */
+                       yyrestart( yyin  );/* { dg-bogus "leak" } */
                        }
 
                else
@@ -1101,8 +1100,7 @@ static int yy_get_next_buffer (void)
 #ifdef __cplusplus
                                        return yyinput();
 #else
-                                       return input();  /* { dg-bogus "infinite recursion" "" { xfail *-*-* } } */
-                                       /* TODO: infinite recursion positive: PR analyzer/103546.  */
+                                       return input();  /* { dg-bogus "infinite recursion" } */
 #endif
                                        }
 

diff --git a/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c b/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c
index 3a9c414..aa8bca7 100644 (file)
--- a/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c
+++ b/gcc/testsuite/gcc.dg/analyzer/pr103217-2.c
@@ -1,3 +1,5 @@
+/* { dg-additional-options "-Wno-analyzer-too-complex" } */
+
 typedef __SIZE_TYPE__ size_t;
 
 extern void *calloc (size_t __nmemb, size_t __size)

diff --git a/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c b/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c
index b103a12..e5f1e4b 100644 (file)
--- a/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c
+++ b/gcc/testsuite/gcc.dg/analyzer/pr103217-3.c
@@ -1,3 +1,5 @@
+/* { dg-additional-options "-Wno-analyzer-too-complex" } */
+
 extern char *strdup (const char *__s)
   __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1)));
 

diff --git a/gcc/testsuite/gcc.dg/analyzer/strdup-pr108252.c b/gcc/testsuite/gcc.dg/analyzer/strdup-pr108252.c
new file mode 100644 (file)
index 0000000..d79d11f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/strdup-pr108252.c
@@ -0,0 +1,19 @@
+extern char *strdup (const char *__s)
+  __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1)));
+
+struct {
+  /* [...snip...] */
+  char *listen_default_ciphers;
+  char *connect_default_ciphers;
+  /* [...snip...] */
+} g;
+
+int parse_global_ciphers(char **args)
+{
+  char **target;
+  target = ((args[0][12] == 'b')
+           ? &g.listen_default_ciphers
+           : &g.connect_default_ciphers);
+  *target = strdup(args[1]);
+  return 0; /* { dg-bogus "leak" } */
+}

diff --git a/gcc/testsuite/gcc.dg/analyzer/strndup-pr108252.c b/gcc/testsuite/gcc.dg/analyzer/strndup-pr108252.c
new file mode 100644 (file)
index 0000000..3e64f27
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/strndup-pr108252.c
@@ -0,0 +1,21 @@
+typedef __SIZE_TYPE__ size_t;
+
+extern char *strndup (const char *__s, size_t sz)
+  __attribute__ ((__nothrow__ , __leaf__, __malloc__, __nonnull__ (1)));
+
+struct {
+  /* [...snip...] */
+  char *listen_default_ciphers;
+  char *connect_default_ciphers;
+  /* [...snip...] */
+} g;
+
+int parse_global_ciphers(char **args)
+{
+  char **target;
+  target = ((args[0][12] == 'b')
+           ? &g.listen_default_ciphers
+           : &g.connect_default_ciphers);
+  *target = strndup(args[1], 42);
+  return 0; /* { dg-bogus "leak" } */
+}