Initialize sanitizer builtins (PR sanitizer/60745)

Submitted by Marek Polacek on April 3, 2014, 4:56 p.m.

Details

Message ID 20140403165630.GE24603@redhat.com
State New
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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);