[committed] analyzer: detect malloc, free, calloc within "std" [PR93959]

Message ID 20200302214826.9274-1-dmalcolm@redhat.com
State New
Headers show
Series
  • [committed] analyzer: detect malloc, free, calloc within "std" [PR93959]
Related show

Commit Message

David Malcolm March 2, 2020, 9:48 p.m.
PR analyzer/93959 reported that g++.dg/analyzer/malloc.C was failing
with no output on Solaris.

The issue is that <stdlib.h> there has "using std::free;", converting
all the "free" calls to std::free, which fails the name-matching via
is_named_call_p.

This patch implements an is_std_named_call_p variant of is_named_call_p
to check for the name within "std", and uses it in sm-malloc.c to check
for std::malloc, std::calloc, and std::free.

Verified the fix on sparc-sun-solaris2.11 (gcc211.fsffrance.org).
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Pushed to master as r10-6981-g9f00b22f98ec0688fcd9816a03aa3f7eea58bcf7.

gcc/analyzer/ChangeLog:
	PR analyzer/93959
	* analyzer.cc (is_std_function_p): New function.
	(is_std_named_call_p): New functions.
	* analyzer.h (is_std_named_call_p): New decl.
	* sm-malloc.cc (malloc_state_machine::on_stmt): Check for "std::"
	variants when checking for malloc, calloc and free.

gcc/testsuite/ChangeLog:
	PR analyzer/93959
	* g++.dg/analyzer/cstdlib-2.C: New test.
	* g++.dg/analyzer/cstdlib.C: New test.
---
 gcc/analyzer/analyzer.cc                  | 61 +++++++++++++++++++++++
 gcc/analyzer/analyzer.h                   |  2 +
 gcc/analyzer/sm-malloc.cc                 |  3 ++
 gcc/testsuite/g++.dg/analyzer/cstdlib-2.C | 25 ++++++++++
 gcc/testsuite/g++.dg/analyzer/cstdlib.C   | 17 +++++++
 5 files changed, 108 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/analyzer/cstdlib-2.C
 create mode 100644 gcc/testsuite/g++.dg/analyzer/cstdlib.C

-- 
2.21.0

Comments

Bernhard Reutner-Fischer March 4, 2020, 3:54 p.m. | #1
On Mon,  2 Mar 2020 16:48:26 -0500
David Malcolm <dmalcolm@redhat.com> wrote:

> +static inline bool

> +is_std_function_p (const_tree fndecl)

> +{

> +  tree name_decl = DECL_NAME (fndecl);

> +  if (!name_decl)

> +    return false;

> +  if (!DECL_CONTEXT (fndecl))

> +    return false;

> +  if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)

> +    return false;

> +  tree ns = DECL_CONTEXT (fndecl);

> +  if (!(DECL_CONTEXT (ns) == NULL_TREE

> +	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))

> +    return false;

> +  if (!DECL_NAME (ns))

> +    return false;

> +  return id_equal ("std", DECL_NAME (ns));

> +}


Sounds a bit elaborate, doesn't?
I hope this is optimized to

static inline bool
is_std_function_p (const_tree fndecl)
{
  tree name_decl = DECL_NAME (fndecl);
  if (!name_decl)
    return false;
  tree ns = DECL_CONTEXT (fndecl);
  if (!ns)
    return false;
  if (TREE_CODE (ns) != NAMESPACE_DECL)
    return false;
  if (!(DECL_CONTEXT (ns) == NULL_TREE
	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
    return false;
  if (!DECL_NAME (ns))
    return false;
  return id_equal ("std", DECL_NAME (ns));
}

isn't "std" spelled out std_identifier() and is an identifier, i.e.
return DECL_NAME (ns) == std_identifier; ?

thanks,
Marek Polacek March 4, 2020, 4:05 p.m. | #2
On Wed, Mar 04, 2020 at 04:54:54PM +0100, Bernhard Reutner-Fischer wrote:
> On Mon,  2 Mar 2020 16:48:26 -0500

> David Malcolm <dmalcolm@redhat.com> wrote:

> 

> > +static inline bool

> > +is_std_function_p (const_tree fndecl)

> > +{

> > +  tree name_decl = DECL_NAME (fndecl);

> > +  if (!name_decl)

> > +    return false;

> > +  if (!DECL_CONTEXT (fndecl))

> > +    return false;

> > +  if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)

> > +    return false;

> > +  tree ns = DECL_CONTEXT (fndecl);

> > +  if (!(DECL_CONTEXT (ns) == NULL_TREE

> > +	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))

> > +    return false;

> > +  if (!DECL_NAME (ns))

> > +    return false;

> > +  return id_equal ("std", DECL_NAME (ns));

> > +}

> 

> Sounds a bit elaborate, doesn't?

> I hope this is optimized to

> 

> static inline bool

> is_std_function_p (const_tree fndecl)

> {

>   tree name_decl = DECL_NAME (fndecl);

>   if (!name_decl)

>     return false;

>   tree ns = DECL_CONTEXT (fndecl);

>   if (!ns)

>     return false;

>   if (TREE_CODE (ns) != NAMESPACE_DECL)

>     return false;

>   if (!(DECL_CONTEXT (ns) == NULL_TREE

> 	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))

>     return false;

>   if (!DECL_NAME (ns))

>     return false;

>   return id_equal ("std", DECL_NAME (ns));

> }

> 

> isn't "std" spelled out std_identifier() and is an identifier, i.e.

> return DECL_NAME (ns) == std_identifier; ?


We have decl_in_std_namespace_p for that.

Marek
David Malcolm March 4, 2020, 4:13 p.m. | #3
On Wed, 2020-03-04 at 16:54 +0100, Bernhard Reutner-Fischer wrote:
> On Mon,  2 Mar 2020 16:48:26 -0500

> David Malcolm <dmalcolm@redhat.com> wrote:

> 

> > +static inline bool

> > +is_std_function_p (const_tree fndecl)

> > +{

> > +  tree name_decl = DECL_NAME (fndecl);

> > +  if (!name_decl)

> > +    return false;

> > +  if (!DECL_CONTEXT (fndecl))

> > +    return false;

> > +  if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)

> > +    return false;

> > +  tree ns = DECL_CONTEXT (fndecl);

> > +  if (!(DECL_CONTEXT (ns) == NULL_TREE

> > +	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))

> > +    return false;

> > +  if (!DECL_NAME (ns))

> > +    return false;

> > +  return id_equal ("std", DECL_NAME (ns));

> > +}

> 

> Sounds a bit elaborate, doesn't?

> I hope this is optimized to

> 

> static inline bool

> is_std_function_p (const_tree fndecl)

> {

>   tree name_decl = DECL_NAME (fndecl);

>   if (!name_decl)

>     return false;

>   tree ns = DECL_CONTEXT (fndecl);

>   if (!ns)

>     return false;

>   if (TREE_CODE (ns) != NAMESPACE_DECL)

>     return false;

>   if (!(DECL_CONTEXT (ns) == NULL_TREE

> 	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))

>     return false;

>   if (!DECL_NAME (ns))

>     return false;

>   return id_equal ("std", DECL_NAME (ns));

> }

> 

> isn't "std" spelled out std_identifier() and is an identifier, i.e.

> return DECL_NAME (ns) == std_identifier; ?


gcc/cp/ChangeLog-2019 has:

2019-10-23  Nathan Sidwell  <nathan@acm.org>

	* cp-tree.c (CPTI_STD_IDENTIFIER): Delete.
	(std_identifier): Delete.
	(DECL_NAME_SPACE_STD_P): Compare against std_node.
        [...snippped...]


The current ideal way to do it in gcc/cp/cp-tree.h seems to be:

  /* Nonzero if NODE is the std namespace.  */
  #define DECL_NAMESPACE_STD_P(NODE)			\
    ((NODE) == std_node)

where:

  #define std_node			cp_global_trees[CPTI_STD]

and:

  extern GTY(()) tree cp_global_trees[CPTI_MAX];


However all of the above is specific to the C++ frontend, whereas the
analyzer is part of the middle-end (and can be run from within lto1,
where any concept of "the frontend" becomes rather abstract).

I don't like the above analyzer code, but I don't see a better way to
do it.

Dave
David Malcolm March 4, 2020, 4:16 p.m. | #4
On Wed, 2020-03-04 at 11:05 -0500, Marek Polacek wrote:
> On Wed, Mar 04, 2020 at 04:54:54PM +0100, Bernhard Reutner-Fischer

> wrote:

> > On Mon,  2 Mar 2020 16:48:26 -0500

> > David Malcolm <dmalcolm@redhat.com> wrote:

> > 

> > > +static inline bool

> > > +is_std_function_p (const_tree fndecl)

> > > +{

> > > +  tree name_decl = DECL_NAME (fndecl);

> > > +  if (!name_decl)

> > > +    return false;

> > > +  if (!DECL_CONTEXT (fndecl))

> > > +    return false;

> > > +  if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)

> > > +    return false;

> > > +  tree ns = DECL_CONTEXT (fndecl);

> > > +  if (!(DECL_CONTEXT (ns) == NULL_TREE

> > > +	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))

> > > +    return false;

> > > +  if (!DECL_NAME (ns))

> > > +    return false;

> > > +  return id_equal ("std", DECL_NAME (ns));

> > > +}

> > 

> > Sounds a bit elaborate, doesn't?

> > I hope this is optimized to

> > 

> > static inline bool

> > is_std_function_p (const_tree fndecl)

> > {

> >   tree name_decl = DECL_NAME (fndecl);

> >   if (!name_decl)

> >     return false;

> >   tree ns = DECL_CONTEXT (fndecl);

> >   if (!ns)

> >     return false;

> >   if (TREE_CODE (ns) != NAMESPACE_DECL)

> >     return false;

> >   if (!(DECL_CONTEXT (ns) == NULL_TREE

> > 	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))

> >     return false;

> >   if (!DECL_NAME (ns))

> >     return false;

> >   return id_equal ("std", DECL_NAME (ns));

> > }

> > 

> > isn't "std" spelled out std_identifier() and is an identifier, i.e.

> > return DECL_NAME (ns) == std_identifier; ?

> 

> We have decl_in_std_namespace_p for that.


Thanks.  Indeed the comment to the function quoted above which wasn't
quoted reads:

/* Return true if FNDECL is within the namespace "std".
   Compare with cp/typeck.c: decl_in_std_namespace_p, but this doesn't
   rely on being the C++ FE (or handle inline namespaces inside of std).  */

since as noted in my reply to Bernhard the analyzer runs in the middle-
end and thus can't rely on features of a specific frontend.

Is there a better way to do this?

Dave
Bernhard Reutner-Fischer March 4, 2020, 7:49 p.m. | #5
On Wed, 04 Mar 2020 11:16:35 -0500
David Malcolm <dmalcolm@redhat.com> wrote:

> On Wed, 2020-03-04 at 11:05 -0500, Marek Polacek wrote:

> > On Wed, Mar 04, 2020 at 04:54:54PM +0100, Bernhard Reutner-Fischer

> > wrote:  

> > > On Mon,  2 Mar 2020 16:48:26 -0500

> > > David Malcolm <dmalcolm@redhat.com> wrote:

> > >   

> > > > +static inline bool

> > > > +is_std_function_p (const_tree fndecl)

> > > > +{

> > > > +  tree name_decl = DECL_NAME (fndecl);

> > > > +  if (!name_decl)

> > > > +    return false;

> > > > +  if (!DECL_CONTEXT (fndecl))

> > > > +    return false;

> > > > +  if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)

> > > > +    return false;

> > > > +  tree ns = DECL_CONTEXT (fndecl);

> > > > +  if (!(DECL_CONTEXT (ns) == NULL_TREE

> > > > +	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))

> > > > +    return false;

> > > > +  if (!DECL_NAME (ns))

> > > > +    return false;

> > > > +  return id_equal ("std", DECL_NAME (ns));

> > > > +}  

> > > 

> > > Sounds a bit elaborate, doesn't?

> > > I hope this is optimized to

> > > 

> > > static inline bool

> > > is_std_function_p (const_tree fndecl)

> > > {

> > >   tree name_decl = DECL_NAME (fndecl);

> > >   if (!name_decl)

> > >     return false;


Make the above
  if (!DECL_NAME (fndecl))
    return false;

> > >   tree ns = DECL_CONTEXT (fndecl);

> > >   if (!ns)

> > >     return false;

> > >   if (TREE_CODE (ns) != NAMESPACE_DECL)

> > >     return false;

> > >   if (!(DECL_CONTEXT (ns) == NULL_TREE

> > > 	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))

> > >     return false;

> > >   if (!DECL_NAME (ns))

> > >     return false;

> > >   return id_equal ("std", DECL_NAME (ns));

> > > }

> > > 

> > > isn't "std" spelled out std_identifier() and is an identifier, i.e.

> > > return DECL_NAME (ns) == std_identifier; ?  

> > 

> > We have decl_in_std_namespace_p for that.  

> 

> Thanks.  Indeed the comment to the function quoted above which wasn't

> quoted reads:

> 

> /* Return true if FNDECL is within the namespace "std".

>    Compare with cp/typeck.c: decl_in_std_namespace_p, but this doesn't

>    rely on being the C++ FE (or handle inline namespaces inside of std).  */

> 

> since as noted in my reply to Bernhard the analyzer runs in the middle-

> end and thus can't rely on features of a specific frontend.

> 

> Is there a better way to do this?


yea probably not (my tree is obviously outdated, i still had
std_identifier lying around here, sorry for that!)

Still, to the bikeshedding question above, does CSE or whatever arrive
at the same or wouldn't we want to phrase it more concisely as i tried
to imply?

thanks,

Patch

diff --git a/gcc/analyzer/analyzer.cc b/gcc/analyzer/analyzer.cc
index 5cf745ea632..8bc3ce49f07 100644
--- a/gcc/analyzer/analyzer.cc
+++ b/gcc/analyzer/analyzer.cc
@@ -86,6 +86,49 @@  is_named_call_p (tree fndecl, const char *funcname)
   return 0 == strcmp (tname, funcname);
 }
 
+/* Return true if FNDECL is within the namespace "std".
+   Compare with cp/typeck.c: decl_in_std_namespace_p, but this doesn't
+   rely on being the C++ FE (or handle inline namespaces inside of std).  */
+
+static inline bool
+is_std_function_p (const_tree fndecl)
+{
+  tree name_decl = DECL_NAME (fndecl);
+  if (!name_decl)
+    return false;
+  if (!DECL_CONTEXT (fndecl))
+    return false;
+  if (TREE_CODE (DECL_CONTEXT (fndecl)) != NAMESPACE_DECL)
+    return false;
+  tree ns = DECL_CONTEXT (fndecl);
+  if (!(DECL_CONTEXT (ns) == NULL_TREE
+	|| TREE_CODE (DECL_CONTEXT (ns)) == TRANSLATION_UNIT_DECL))
+    return false;
+  if (!DECL_NAME (ns))
+    return false;
+  return id_equal ("std", DECL_NAME (ns));
+}
+
+/* Like is_named_call_p, but look for std::FUNCNAME.  */
+
+bool
+is_std_named_call_p (tree fndecl, const char *funcname)
+{
+  gcc_assert (fndecl);
+  gcc_assert (funcname);
+
+  if (!is_std_function_p (fndecl))
+    return false;
+
+  tree identifier = DECL_NAME (fndecl);
+  const char *name = IDENTIFIER_POINTER (identifier);
+  const char *tname = name;
+
+  /* Don't disregard prefix _ or __ in FNDECL's name.  */
+
+  return 0 == strcmp (tname, funcname);
+}
+
 /* Helper function for checkers.  Is FNDECL an extern fndecl at file scope
    that has the given FUNCNAME, and does CALL have the given number of
    arguments?  */
@@ -106,6 +149,24 @@  is_named_call_p (tree fndecl, const char *funcname,
   return true;
 }
 
+/* Like is_named_call_p, but check for std::FUNCNAME.  */
+
+bool
+is_std_named_call_p (tree fndecl, const char *funcname,
+		     const gcall *call, unsigned int num_args)
+{
+  gcc_assert (fndecl);
+  gcc_assert (funcname);
+
+  if (!is_std_named_call_p (fndecl, funcname))
+    return false;
+
+  if (gimple_call_num_args (call) != num_args)
+    return false;
+
+  return true;
+}
+
 /* Return true if stmt is a setjmp or sigsetjmp call.  */
 
 bool
diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h
index 1ae76cc4ea0..5364edb3d96 100644
--- a/gcc/analyzer/analyzer.h
+++ b/gcc/analyzer/analyzer.h
@@ -78,6 +78,8 @@  extern bool is_special_named_call_p (const gcall *call, const char *funcname,
 extern bool is_named_call_p (tree fndecl, const char *funcname);
 extern bool is_named_call_p (tree fndecl, const char *funcname,
 			     const gcall *call, unsigned int num_args);
+extern bool is_std_named_call_p (tree fndecl, const char *funcname,
+				 const gcall *call, unsigned int num_args);
 extern bool is_setjmp_call_p (const gcall *call);
 extern bool is_longjmp_call_p (const gcall *call);
 
diff --git a/gcc/analyzer/sm-malloc.cc b/gcc/analyzer/sm-malloc.cc
index 46225b6f700..aaef6959362 100644
--- a/gcc/analyzer/sm-malloc.cc
+++ b/gcc/analyzer/sm-malloc.cc
@@ -611,6 +611,8 @@  malloc_state_machine::on_stmt (sm_context *sm_ctxt,
       {
 	if (is_named_call_p (callee_fndecl, "malloc", call, 1)
 	    || is_named_call_p (callee_fndecl, "calloc", call, 2)
+	    || is_std_named_call_p (callee_fndecl, "malloc", call, 1)
+	    || is_std_named_call_p (callee_fndecl, "calloc", call, 2)
 	    || is_named_call_p (callee_fndecl, "__builtin_malloc", call, 1)
 	    || is_named_call_p (callee_fndecl, "__builtin_calloc", call, 2))
 	  {
@@ -640,6 +642,7 @@  malloc_state_machine::on_stmt (sm_context *sm_ctxt,
 	  }
 
 	if (is_named_call_p (callee_fndecl, "free", call, 1)
+	    || is_std_named_call_p (callee_fndecl, "free", call, 1)
 	    || is_named_call_p (callee_fndecl, "__builtin_free", call, 1))
 	  {
 	    tree arg = gimple_call_arg (call, 0);
diff --git a/gcc/testsuite/g++.dg/analyzer/cstdlib-2.C b/gcc/testsuite/g++.dg/analyzer/cstdlib-2.C
new file mode 100644
index 00000000000..0dedf8aef5c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/analyzer/cstdlib-2.C
@@ -0,0 +1,25 @@ 
+/* Manual reimplemenation of <cstdlib>, to test name-matching within std.  */
+
+namespace std
+{
+  typedef __SIZE_TYPE__ size_t;
+  void *malloc (std::size_t size);
+  void *calloc (std::size_t num, std::size_t size);
+  void free (void *ptr);
+}
+
+void test_1 (void *ptr)
+{
+  std::free (ptr); /* { dg-message "first 'free' here" } */
+  std::free (ptr); /* { dg-warning "double-'free' of 'ptr'" } */
+}
+
+void test_2 (void)
+{
+  void *p = std::malloc (1024); /* { dg-message "allocated here" } */
+} /* { dg-warning "leak of 'p'" } */
+
+void test_3 (void)
+{
+  void *p = std::calloc (42, 1024); /* { dg-message "allocated here" } */
+} /* { dg-warning "leak of 'p'" } */
diff --git a/gcc/testsuite/g++.dg/analyzer/cstdlib.C b/gcc/testsuite/g++.dg/analyzer/cstdlib.C
new file mode 100644
index 00000000000..ec6327bf884
--- /dev/null
+++ b/gcc/testsuite/g++.dg/analyzer/cstdlib.C
@@ -0,0 +1,17 @@ 
+#include <cstdlib>
+
+void test_1 (void *ptr)
+{
+  std::free (ptr); /* { dg-message "first 'free' here" } */
+  std::free (ptr); /* { dg-warning "double-'free' of 'ptr'" } */
+}
+
+void test_2 (void)
+{
+  void *p = std::malloc (1024); /* { dg-message "allocated here" } */
+} /* { dg-warning "leak of 'p'" } */
+
+void test_3 (void)
+{
+  void *p = std::calloc (42, 1024); /* { dg-message "allocated here" } */
+} /* { dg-warning "leak of 'p'" } */