Patchwork Initialize sanitizer builtins (PR sanitizer/60745)

login
register
mail settings
Submitter Marek Polacek
Date April 3, 2014, 4:56 p.m.
Message ID <20140403165630.GE24603@redhat.com>
Download mbox | patch
Permalink /patch/336677/
State New
Headers show

Comments

Marek Polacek - April 3, 2014, 4:56 p.m.
Under certain circumstances the sanitizer builtins are not initialized
properly and ubsan_instrument_return must make sure they are
initialized.  Otherwise builtin_decl_explicit returns NULL and
we'll ICE in build_call_expr_loc_array.  I'm not sure which other
ubsan routines need similar fix.

No testcase attached since it's not trivial to reproduce this.

Bootstrapped/ran ubsan testsuite on x86_64-linux, ok for trunk?

2014-04-03  Marek Polacek  <polacek@redhat.com>

	PR sanitizer/60745
	* c-ubsan.c: Include asan.h.
	(ubsan_instrument_return): Call initialize_sanitizer_builtins.


	Marek
Jeff Law - April 4, 2014, 3:17 p.m.
On 04/03/14 10:56, Marek Polacek wrote:
> Under certain circumstances the sanitizer builtins are not initialized
> properly and ubsan_instrument_return must make sure they are
> initialized.  Otherwise builtin_decl_explicit returns NULL and
> we'll ICE in build_call_expr_loc_array.  I'm not sure which other
> ubsan routines need similar fix.
>
> No testcase attached since it's not trivial to reproduce this.
>
> Bootstrapped/ran ubsan testsuite on x86_64-linux, ok for trunk?
>
> 2014-04-03  Marek Polacek  <polacek@redhat.com>
>
> 	PR sanitizer/60745
> 	* c-ubsan.c: Include asan.h.
> 	(ubsan_instrument_return): Call initialize_sanitizer_builtins.
So what are those circumstances?  ISTM this deserves some kind of 
comment at the least.

jeff
Jonathan Wakely - April 4, 2014, 3:48 p.m.
On 04/04/14 09:17 -0600, Jeff Law wrote:
>So what are those circumstances?  ISTM this deserves some kind of 
>comment at the least.

I found the ICE using ubsan on the libstdc++ testsuite. Including the
bits/stdc++.h PCH'd header (i.e. the entire library) seemed to trigger
it. So either related to PCH, or just huge translation units ... not
sure of the exact cause.
Marek Polacek - April 4, 2014, 5:14 p.m.
On Fri, Apr 04, 2014 at 04:48:48PM +0100, Jonathan Wakely wrote:
> On 04/04/14 09:17 -0600, Jeff Law wrote:
> >So what are those circumstances?  ISTM this deserves some kind of
> >comment at the least.
> 
> I found the ICE using ubsan on the libstdc++ testsuite. Including the
> bits/stdc++.h PCH'd header (i.e. the entire library) seemed to trigger
> it. So either related to PCH, or just huge translation units ... not
> sure of the exact cause.

So what happens here is that normally we initialize the builtins via
c_common_nodes_and_builtins, which has:

 5711   if (!flag_preprocess_only)
 5712     c_define_builtins (va_list_ref_type_node, va_list_arg_type_node);

c_define_builtins defines builtins in builtins.def, that includes even
sanitizer.def.  I guess that flag_preprocess_only was in effect due to PCH
and we didn't define the builtins.  I still haven't managed to create
some sweet & small testcase, but I've found another ICE with PCH:

$ touch y.h
$ cat y.c
#include "y.h"
int main () {}
$ gcc y.h
$ gcc y.c -fsanitize=undefined -S
y.c: In function ‘main’:
y.c:2:1: internal compiler error: Segmentation fault
 int main () {}
 ^
0x9a20df crash_signal
	/home/marek/src/gcc/gcc/toplev.c:337
0x52dfde bind
	/home/marek/src/gcc/gcc/c/c-decl.c:646
0x53767f c_builtin_function(tree_node*)
	/home/marek/src/gcc/gcc/c/c-decl.c:3664
0x875340 add_builtin_function_common
	/home/marek/src/gcc/gcc/langhooks.c:573
0x875ff3 add_builtin_function(char const*, tree_node*, int, built_in_class, char const*, tree_node*)
	/home/marek/src/gcc/gcc/langhooks.c:589
0x9b46ba initialize_sanitizer_builtins()
	/home/marek/src/gcc/gcc/sanitizer.def:30
0x9bfca5 ubsan_pass
	/home/marek/src/gcc/gcc/ubsan.c:870
0x9bfca5 execute
	/home/marek/src/gcc/gcc/ubsan.c:938
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <http://gcc.gnu.org/bugs.html> for instructions.

	Marek
Jeff Law - April 4, 2014, 5:50 p.m.
On 04/04/14 11:14, Marek Polacek wrote:
>
> So what happens here is that normally we initialize the builtins via
> c_common_nodes_and_builtins, which has:
>
>   5711   if (!flag_preprocess_only)
>   5712     c_define_builtins (va_list_ref_type_node, va_list_arg_type_node);
>
> c_define_builtins defines builtins in builtins.def, that includes even
> sanitizer.def.  I guess that flag_preprocess_only was in effect due to PCH
> and we didn't define the builtins.  I still haven't managed to create
> some sweet & small testcase, but I've found another ICE with PCH:
What doesn't make sense to me is I don't see where PCH sets 
flag_preprocess_only.  If you can point me to where that happens, then I 
think we'd just need a comment that PCH turns on flag_preprocess_only, 
which guards the creation of some builtins.

jeff
Marek Polacek - April 4, 2014, 7:45 p.m.
On Fri, Apr 04, 2014 at 11:50:04AM -0600, Jeff Law wrote:
> On 04/04/14 11:14, Marek Polacek wrote:
> >
> >So what happens here is that normally we initialize the builtins via
> >c_common_nodes_and_builtins, which has:
> >
> >  5711   if (!flag_preprocess_only)
> >  5712     c_define_builtins (va_list_ref_type_node, va_list_arg_type_node);
> >
> >c_define_builtins defines builtins in builtins.def, that includes even
> >sanitizer.def.  I guess that flag_preprocess_only was in effect due to PCH
> >and we didn't define the builtins.  I still haven't managed to create
> >some sweet & small testcase, but I've found another ICE with PCH:
> What doesn't make sense to me is I don't see where PCH sets
> flag_preprocess_only.  If you can point me to where that happens,
> then I think we'd just need a comment that PCH turns on
> flag_preprocess_only, which guards the creation of some builtins.

I'm sorry, my guess was wrong, it's not about flag_preprocess_only at
all.  But still it's about PCH ;).  In short: we define the builtins,
but PCH then removes the definitions.

The thing is, we define the
builtins just fine (via c_common_nodes_and_builtins -> c_define_builtins),
but the problem is that later on we call c_common_read_pch that calls
gt_pch_restore which "reads the state of the compiler back in from
file" -- and as a part of this, it overwrites the table with defined
builtins (builtin_info.decl).  And so we end up with zapped table
and thus it's needed to re-initialize it.  I don't see an easy way
how to tell PCH to not to do the removal.

	Marek
Jeff Law - April 4, 2014, 8:53 p.m.
On 04/04/14 13:45, Marek Polacek wrote:
>
> I'm sorry, my guess was wrong, it's not about flag_preprocess_only at
> all.  But still it's about PCH ;).  In short: we define the builtins,
> but PCH then removes the definitions.
Ah.

> The thing is, we define the
> builtins just fine (via c_common_nodes_and_builtins -> c_define_builtins),
> but the problem is that later on we call c_common_read_pch that calls
> gt_pch_restore which "reads the state of the compiler back in from
> file" -- and as a part of this, it overwrites the table with defined
> builtins (builtin_info.decl).  And so we end up with zapped table
> and thus it's needed to re-initialize it.  I don't see an easy way
> how to tell PCH to not to do the removal.
Presumably when we wrote out the PCH -fsanitize wasn't being used and 
thus those builtins are not initialized.  Right?

Assuming that's correct, your patch is fine with a comment to that effect.

Jeff

Patch

diff --git gcc/c-family/c-ubsan.c gcc/c-family/c-ubsan.c
index dc4d981..9d2403c 100644
--- gcc/c-family/c-ubsan.c
+++ gcc/c-family/c-ubsan.c
@@ -29,6 +29,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "ubsan.h"
 #include "c-family/c-common.h"
 #include "c-family/c-ubsan.h"
+#include "asan.h"
 
 /* Instrument division by zero and INT_MIN / -1.  If not instrumenting,
    return NULL_TREE.  */
@@ -185,6 +186,8 @@  ubsan_instrument_vla (location_t loc, tree size)
 tree
 ubsan_instrument_return (location_t loc)
 {
+  initialize_sanitizer_builtins ();
+
   tree data = ubsan_create_data ("__ubsan_missing_return_data", &loc,
 				 NULL, NULL_TREE);
   tree t = builtin_decl_explicit (BUILT_IN_UBSAN_HANDLE_MISSING_RETURN);