Patchwork [middle-end] : Fix PR53136, Use after free in cgraph dumps

login
register
mail settings
Submitter Uros Bizjak
Date April 30, 2012, 6:03 p.m.
Message ID <CAFULd4YcgjiLa3jhBziMdPXEgAoQQ79nBR7CizsRiaVt2XEnjw@mail.gmail.com>
Download mbox | patch
Permalink /patch/155944/
State New
Headers show

Comments

Uros Bizjak - April 30, 2012, 6:03 p.m.
Hello!

There is a problem with multiple calls of cgraph_node_name in fprintf
dumps. Please note that C++ uses caching in
cxx_printable_name_internal (aka LANG_HOOKS_DECL_PRINTABLE_NAME), so
when cxx_printable_name_internal is called multiple times from printf
(i.e. fprintf "%s/%i -> %s/%i"), it can happen that the first string
gets evicted by the second call, before fprintf is fully evaluated.

Attached patch audits all uses of cgraph_node_name, and in case of
multiple calls in dump fprintf, wraps every call in xstrdup. This
fixes valgrind report in the PR, as well as original dump failure on
alpha [1].

I think that small memory leak is tolerable here (the changes are
exclusively in the dump code), and follows the same approach as in
java frontend.

2012-04-30  Uros Bizjak  <ubizjak@gmail.com>

	PR middle-end/53136
	* ipa-prop.c (ipa_print_node_jump_functions): Wrap multiple
	calls to cgraph_node_name in xstrdup.
	(ipa_make_edge_direct_to_target): Ditto.
	* cgraph.c (dump_cgraph_node): Ditto.
	* tree-sra.c (convert_callers_for_node): Ditto.
	* lto-symtab.c (lto_cgraph_replace_node): Ditto.
	* ipa-cp.c (perhaps_add_new_callers): Ditto.
	* cgraphunit.c (cgraph_redirect_edge_call_stmt_to_callee): Ditto.
	(cgraph_materialize_all_clones): Ditto.
	* ipa-inline.c (report_inline_failed_reason): Ditto.
	(want_early_inline_function_p): Ditto.
	(edge_badness): Ditto.
	(update_edge_key): Ditto.
	(flatten_function): Ditto.
	(ipa_inline): Ditto.
	(inlinw_always_inline_functions): Ditto.
	(early_inline_small_functions): Ditto.

Patch was tested on x86_64-pc-linux-gnu {,-m32} and alphaev68-pc-linux-gnu.

OK for mainline?

[1] http://gcc.gnu.org/ml/gcc-testresults/2012-04/msg02722.html

Uros.
Jan Hubicka - April 30, 2012, 7:16 p.m.
> Hello!
> 
> There is a problem with multiple calls of cgraph_node_name in fprintf
> dumps. Please note that C++ uses caching in
> cxx_printable_name_internal (aka LANG_HOOKS_DECL_PRINTABLE_NAME), so
> when cxx_printable_name_internal is called multiple times from printf
> (i.e. fprintf "%s/%i -> %s/%i"), it can happen that the first string
> gets evicted by the second call, before fprintf is fully evaluated.
> 
> Attached patch audits all uses of cgraph_node_name, and in case of
> multiple calls in dump fprintf, wraps every call in xstrdup. This
> fixes valgrind report in the PR, as well as original dump failure on
> alpha [1].
> 
> I think that small memory leak is tolerable here (the changes are
> exclusively in the dump code), and follows the same approach as in
> java frontend.

Hmm, it is somewhat ugly, but I guess it is OK.
I was not aware that the printable names are getting released.
Thanks!

Honza

Patch

Index: tree-sra.c
===================================================================
--- tree-sra.c	(revision 186992)
+++ tree-sra.c	(working copy)
@@ -4612,8 +4612,8 @@  convert_callers_for_node (struct cgraph_node *node
       if (dump_file)
 	fprintf (dump_file, "Adjusting call (%i -> %i) %s -> %s\n",
 		 cs->caller->uid, cs->callee->uid,
-		 cgraph_node_name (cs->caller),
-		 cgraph_node_name (cs->callee));
+		 xstrdup (cgraph_node_name (cs->caller)),
+		 xstrdup (cgraph_node_name (cs->callee)));
 
       ipa_modify_call_arguments (cs, cs->call_stmt, adjustments);
 
Index: ipa-prop.c
===================================================================
--- ipa-prop.c	(revision 186992)
+++ ipa-prop.c	(working copy)
@@ -230,8 +230,8 @@  ipa_print_node_jump_functions (FILE *f, struct cgr
 	continue;
 
       fprintf (f, "    callsite  %s/%i -> %s/%i : \n",
-	       cgraph_node_name (node), node->uid,
-	       cgraph_node_name (cs->callee), cs->callee->uid);
+	       xstrdup (cgraph_node_name (node)), node->uid,
+	       xstrdup (cgraph_node_name (cs->callee)), cs->callee->uid);
       ipa_print_node_jump_functions_for_edge (f, cs);
     }
 
@@ -1780,8 +1780,8 @@  ipa_make_edge_direct_to_target (struct cgraph_edge
       fprintf (dump_file, "ipa-prop: Discovered %s call to a known target "
 	       "(%s/%i -> %s/%i), for stmt ",
 	       ie->indirect_info->polymorphic ? "a virtual" : "an indirect",
-	       cgraph_node_name (ie->caller), ie->caller->uid,
-	       cgraph_node_name (ie->callee), ie->callee->uid);
+	       xstrdup (cgraph_node_name (ie->caller)), ie->caller->uid,
+	       xstrdup (cgraph_node_name (ie->callee)), ie->callee->uid);
       if (ie->call_stmt)
 	print_gimple_stmt (dump_file, ie->call_stmt, 2, TDF_SLIM);
       else
Index: cgraph.c
===================================================================
--- cgraph.c	(revision 186992)
+++ cgraph.c	(working copy)
@@ -1521,9 +1521,9 @@  dump_cgraph_node (FILE *f, struct cgraph_node *nod
 
   if (node->global.inlined_to)
     fprintf (f, "  Function %s/%i is inline copy in %s/%i\n",
-	     cgraph_node_name (node),
+	     xstrdup (cgraph_node_name (node)),
 	     node->symbol.order,
-	     cgraph_node_name (node->global.inlined_to),
+	     xstrdup (cgraph_node_name (node->global.inlined_to)),
 	     node->global.inlined_to->symbol.order);
   if (node->clone_of)
     fprintf (f, "  Clone of %s/%i\n",
Index: lto-symtab.c
===================================================================
--- lto-symtab.c	(revision 186992)
+++ lto-symtab.c	(working copy)
@@ -215,8 +215,8 @@  lto_cgraph_replace_node (struct cgraph_node *node,
     {
       fprintf (cgraph_dump_file, "Replacing cgraph node %s/%i by %s/%i"
  	       " for symbol %s\n",
-	       cgraph_node_name (node), node->uid,
-	       cgraph_node_name (prevailing_node),
+	       xstrdup (cgraph_node_name (node)), node->uid,
+	       xstrdup (cgraph_node_name (prevailing_node)),
 	       prevailing_node->uid,
 	       IDENTIFIER_POINTER ((*targetm.asm_out.mangle_assembler_name)
 		 (IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (node->symbol.decl)))));
Index: ipa-cp.c
===================================================================
--- ipa-cp.c	(revision 186992)
+++ ipa-cp.c	(working copy)
@@ -2180,8 +2180,9 @@  perhaps_add_new_callers (struct cgraph_node *node,
 		  if (dump_file)
 		    fprintf (dump_file, " - adding an extra caller %s/%i"
 			     " of %s/%i\n",
-			     cgraph_node_name (cs->caller), cs->caller->uid,
-			     cgraph_node_name (val->spec_node),
+			     xstrdup (cgraph_node_name (cs->caller)),
+			     cs->caller->uid,
+			     xstrdup (cgraph_node_name (val->spec_node)),
 			     val->spec_node->uid);
 
 		  cgraph_redirect_edge_callee (cs, val->spec_node);
Index: cgraphunit.c
===================================================================
--- cgraphunit.c	(revision 186992)
+++ cgraphunit.c	(working copy)
@@ -2133,8 +2133,8 @@  cgraph_redirect_edge_call_stmt_to_callee (struct c
   if (cgraph_dump_file)
     {
       fprintf (cgraph_dump_file, "updating call of %s/%i -> %s/%i: ",
-	       cgraph_node_name (e->caller), e->caller->uid,
-	       cgraph_node_name (e->callee), e->callee->uid);
+	       xstrdup (cgraph_node_name (e->caller)), e->caller->uid,
+	       xstrdup (cgraph_node_name (e->callee)), e->callee->uid);
       print_gimple_stmt (cgraph_dump_file, e->call_stmt, 0, dump_flags);
       if (e->callee->clone.combined_args_to_skip)
 	{
@@ -2220,8 +2220,8 @@  cgraph_materialize_all_clones (void)
 		  if (cgraph_dump_file)
 		    {
 		      fprintf (cgraph_dump_file, "cloning %s to %s\n",
-			       cgraph_node_name (node->clone_of),
-			       cgraph_node_name (node));
+			       xstrdup (cgraph_node_name (node->clone_of)),
+			       xstrdup (cgraph_node_name (node)));
 		      if (node->clone.tree_map)
 		        {
 			  unsigned int i;
Index: ipa-inline.c
===================================================================
--- ipa-inline.c	(revision 186992)
+++ ipa-inline.c	(working copy)
@@ -220,8 +220,8 @@  report_inline_failed_reason (struct cgraph_edge *e
   if (dump_file)
     {
       fprintf (dump_file, "  not inlinable: %s/%i -> %s/%i, %s\n",
-	       cgraph_node_name (e->caller), e->caller->uid,
-	       cgraph_node_name (e->callee), e->callee->uid,
+	       xstrdup (cgraph_node_name (e->caller)), e->caller->uid,
+	       xstrdup (cgraph_node_name (e->callee)), e->callee->uid,
 	       cgraph_inline_failed_string (e->inline_failed));
     }
 }
@@ -423,8 +423,8 @@  want_early_inline_function_p (struct cgraph_edge *
 	  if (dump_file)
 	    fprintf (dump_file, "  will not early inline: %s/%i->%s/%i, "
 		     "call is cold and code would grow by %i\n",
-		     cgraph_node_name (e->caller), e->caller->uid,
-		     cgraph_node_name (callee), callee->uid,
+		     xstrdup (cgraph_node_name (e->caller)), e->caller->uid,
+		     xstrdup (cgraph_node_name (callee)), callee->uid,
 		     growth);
 	  want_inline = false;
 	}
@@ -434,8 +434,8 @@  want_early_inline_function_p (struct cgraph_edge *
 	  if (dump_file)
 	    fprintf (dump_file, "  will not early inline: %s/%i->%s/%i, "
 		     "callee is not leaf and code would grow by %i\n",
-		     cgraph_node_name (e->caller), e->caller->uid,
-		     cgraph_node_name (callee), callee->uid,
+		     xstrdup (cgraph_node_name (e->caller)), e->caller->uid,
+		     xstrdup (cgraph_node_name (callee)), callee->uid,
 		     growth);
 	  want_inline = false;
 	}
@@ -444,8 +444,8 @@  want_early_inline_function_p (struct cgraph_edge *
 	  if (dump_file)
 	    fprintf (dump_file, "  will not early inline: %s/%i->%s/%i, "
 		     "growth %i exceeds --param early-inlining-insns\n",
-		     cgraph_node_name (e->caller), e->caller->uid,
-		     cgraph_node_name (callee), callee->uid,
+		     xstrdup (cgraph_node_name (e->caller)), e->caller->uid,
+		     xstrdup (cgraph_node_name (callee)), callee->uid,
 		     growth);
 	  want_inline = false;
 	}
@@ -754,8 +754,8 @@  edge_badness (struct cgraph_edge *edge, bool dump)
   if (dump)
     {
       fprintf (dump_file, "    Badness calculation for %s -> %s\n",
-	       cgraph_node_name (edge->caller),
-	       cgraph_node_name (callee));
+	       xstrdup (cgraph_node_name (edge->caller)),
+	       xstrdup (cgraph_node_name (callee)));
       fprintf (dump_file, "      size growth %i, time growth %i\n",
 	       growth,
 	       time_growth);
@@ -910,8 +910,10 @@  update_edge_key (fibheap_t heap, struct cgraph_edg
 	    {
 	      fprintf (dump_file,
 		       "  decreasing badness %s/%i -> %s/%i, %i to %i\n",
-		       cgraph_node_name (edge->caller), edge->caller->uid,
-		       cgraph_node_name (edge->callee), edge->callee->uid,
+		       xstrdup (cgraph_node_name (edge->caller)),
+		       edge->caller->uid,
+		       xstrdup (cgraph_node_name (edge->callee)),
+		       edge->callee->uid,
 		       (int)n->key,
 		       badness);
 	    }
@@ -925,8 +927,10 @@  update_edge_key (fibheap_t heap, struct cgraph_edg
 	 {
 	   fprintf (dump_file,
 		    "  enqueuing call %s/%i -> %s/%i, badness %i\n",
-		    cgraph_node_name (edge->caller), edge->caller->uid,
-		    cgraph_node_name (edge->callee), edge->callee->uid,
+		    xstrdup (cgraph_node_name (edge->caller)),
+		    edge->caller->uid,
+		    xstrdup (cgraph_node_name (edge->callee)),
+		    edge->callee->uid,
 		    badness);
 	 }
       edge->aux = fibheap_insert (heap, badness, edge);
@@ -1610,8 +1614,8 @@  flatten_function (struct cgraph_node *node, bool e
 	  if (dump_file)
 	    fprintf (dump_file,
 		     "Not inlining %s into %s to avoid cycle.\n",
-		     cgraph_node_name (callee),
-		     cgraph_node_name (e->caller));
+		     xstrdup (cgraph_node_name (callee)),
+		     xstrdup (cgraph_node_name (e->caller)));
 	  e->inline_failed = CIF_RECURSIVE_INLINING;
 	  continue;
 	}
@@ -1651,8 +1655,8 @@  flatten_function (struct cgraph_node *node, bool e
          recursing through the original node if the node was cloned.  */
       if (dump_file)
 	fprintf (dump_file, " Inlining %s into %s.\n",
-		 cgraph_node_name (callee),
-		 cgraph_node_name (e->caller));
+		 xstrdup (cgraph_node_name (callee)),
+		 xstrdup (cgraph_node_name (e->caller)));
       orig_callee = callee;
       inline_call (e, true, NULL, NULL);
       if (e->callee != orig_callee)
@@ -1754,7 +1758,8 @@  ipa_inline (void)
 		    {
 		      fprintf (dump_file,
 			       "\nInlining %s size %i.\n",
-			       cgraph_node_name (node), inline_summary (node)->size);
+			       cgraph_node_name (node),
+			       inline_summary (node)->size);
 		      fprintf (dump_file,
 			       " Called once from %s %i insns.\n",
 			       cgraph_node_name (node->callers->caller),
@@ -1817,8 +1822,8 @@  inline_always_inline_functions (struct cgraph_node
 
       if (dump_file)
 	fprintf (dump_file, "  Inlining %s into %s (always_inline).\n",
-		 cgraph_node_name (e->callee),
-		 cgraph_node_name (e->caller));
+		 xstrdup (cgraph_node_name (e->callee)),
+		 xstrdup (cgraph_node_name (e->caller)));
       inline_call (e, true, NULL, NULL);
       inlined = true;
     }
@@ -1867,8 +1872,8 @@  early_inline_small_functions (struct cgraph_node *
 
       if (dump_file)
 	fprintf (dump_file, " Inlining %s into %s.\n",
-		 cgraph_node_name (callee),
-		 cgraph_node_name (e->caller));
+		 xstrdup (cgraph_node_name (callee)),
+		 xstrdup (cgraph_node_name (e->caller)));
       inline_call (e, true, NULL, NULL);
       inlined = true;
     }