| Message ID | 20140627071307.GW31640@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 27 Jun 2014, Jakub Jelinek wrote: > Hi! > > This patch implements sanitization for nonnull and returns_nonnull > attributes. > Similarly to -fsanitize=null this option disables > -fdelete-null-pointer-checks silently, and adds checks before calling > functions with declared nonnull arguments if we don't pass NULL there, > and before return in functions declared with returns_nonnull also inserts > check whether the return value is not NULL. > > As GCC 4.9.0+ now aggressively optimizes based on these attributes and we've > seen several issues in real world apps, I think this is really needed. > > Bootstrapped/regtested on x86_64-linux and i686-linux normally (yes,rtl > checking) and c,c++,fortran release checking bootstrap-ubsan. > > Will post bugs in gcc discovered by it later. > > The patch adds two new (trivial handlers) to libubsan, as it is maintained > in llvm's compiler-rt, will talk to them if they are interested in those > and what exact wording and form (AFAIK clang also added the gcc > {,returns_}nonnull attributes). If they wouldn't be interested, guess > we could add them in a separate, gcc owned, source file in ubsan (like we > own Makefile*). Looks good to me. Thanks for adding this! Richard. > 2014-06-27 Jakub Jelinek <jakub@redhat.com> > > * flag-types.h (enum sanitize_code): Add SANITIZE_NONNULL and > SANITIZE_RETURNS_NONNULL, or them into SANITIZE_UNDEFINED. > * opts.c (common_handle_option): Handle SANITIZE_NONNULL and > SANITIZE_RETURNS_NONNULL and disable flag_delete_null_pointer_checks > for them. > * sanitizer.def (BUILT_IN_UBSAN_HANDLE_NONNULL_ARG, > BUILT_IN_UBSAN_HANDLE_NONNULL_ARG_ABORT, > BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN, > BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN_ABORT): New. > * ubsan.c (instrument_bool_enum_load): Set *gsi back to > stmt's iterator. > (instrument_nonnull_arg, instrument_nonnull_return): New functions. > (pass_ubsan::gate): Return true even for SANITIZE_NONNULL > or SANITIZE_RETURNS_NONNULL. > (pass_ubsan::execute): Call instrument_nonnull_{arg,return}. > > * c-c++-common/ubsan/nonnull-1.c: New test. > * c-c++-common/ubsan/nonnull-2.c: New test. > * c-c++-common/ubsan/nonnull-3.c: New test. > * c-c++-common/ubsan/nonnull-4.c: New test. > * c-c++-common/ubsan/nonnull-5.c: New test. > > --- gcc/flag-types.h.jj 2014-06-20 23:26:24.000000000 +0200 > +++ gcc/flag-types.h 2014-06-26 14:39:44.133970103 +0200 > @@ -231,10 +231,13 @@ enum sanitize_code { > SANITIZE_FLOAT_DIVIDE = 1 << 12, > SANITIZE_FLOAT_CAST = 1 << 13, > SANITIZE_BOUNDS = 1 << 14, > + SANITIZE_NONNULL = 1 << 15, > + SANITIZE_RETURNS_NONNULL = 1 << 16, > SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE | SANITIZE_UNREACHABLE > | SANITIZE_VLA | SANITIZE_NULL | SANITIZE_RETURN > | SANITIZE_SI_OVERFLOW | SANITIZE_BOOL | SANITIZE_ENUM > - | SANITIZE_BOUNDS, > + | SANITIZE_BOUNDS | SANITIZE_NONNULL > + | SANITIZE_RETURNS_NONNULL, > SANITIZE_NONDEFAULT = SANITIZE_FLOAT_DIVIDE | SANITIZE_FLOAT_CAST > }; > > --- gcc/opts.c.jj 2014-06-20 23:26:23.000000000 +0200 > +++ gcc/opts.c 2014-06-26 14:43:51.620652007 +0200 > @@ -1468,6 +1468,9 @@ common_handle_option (struct gcc_options > { "float-cast-overflow", SANITIZE_FLOAT_CAST, > sizeof "float-cast-overflow" - 1 }, > { "bounds", SANITIZE_BOUNDS, sizeof "bounds" - 1 }, > + { "nonnull", SANITIZE_NONNULL, sizeof "nonnull" - 1 }, > + { "returns-nonnull", SANITIZE_RETURNS_NONNULL, > + sizeof "returns-nonnull" - 1 }, > { NULL, 0, 0 } > }; > const char *comma; > @@ -1511,7 +1514,8 @@ common_handle_option (struct gcc_options > > /* When instrumenting the pointers, we don't want to remove > the null pointer checks. */ > - if (flag_sanitize & SANITIZE_NULL) > + if (flag_sanitize & (SANITIZE_NULL | SANITIZE_NONNULL > + | SANITIZE_RETURNS_NONNULL)) > opts->x_flag_delete_null_pointer_checks = 0; > break; > } > --- gcc/sanitizer.def.jj 2014-06-20 23:26:23.000000000 +0200 > +++ gcc/sanitizer.def 2014-06-26 16:52:34.644801331 +0200 > @@ -417,3 +417,19 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HAN > "__ubsan_handle_out_of_bounds_abort", > BT_FN_VOID_PTR_PTR, > ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_NONNULL_ARG, > + "__ubsan_handle_nonnull_arg", > + BT_FN_VOID_PTR_PTRMODE, > + ATTR_COLD_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_NONNULL_ARG_ABORT, > + "__ubsan_handle_nonnull_arg_abort", > + BT_FN_VOID_PTR_PTRMODE, > + ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN, > + "__ubsan_handle_nonnull_return", > + BT_FN_VOID_PTR, > + ATTR_COLD_NOTHROW_LEAF_LIST) > +DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN_ABORT, > + "__ubsan_handle_nonnull_return_abort", > + BT_FN_VOID_PTR, > + ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST) > --- gcc/ubsan.c.jj 2014-06-20 23:26:24.000000000 +0200 > +++ gcc/ubsan.c 2014-06-26 18:23:28.190191706 +0200 > @@ -997,6 +997,7 @@ instrument_bool_enum_load (gimple_stmt_i > } > gimple_set_location (g, loc); > gsi_insert_before (&gsi2, g, GSI_SAME_STMT); > + *gsi = gsi_for_stmt (stmt); > } > > /* Instrument float point-to-integer conversion. TYPE is an integer type of > @@ -1121,6 +1122,117 @@ ubsan_instrument_float_cast (location_t > fn, integer_zero_node); > } > > +/* Instrument values passed to function arguments with nonnull attribute. */ > + > +static void > +instrument_nonnull_arg (gimple_stmt_iterator *gsi) > +{ > + gimple stmt = gsi_stmt (*gsi); > + location_t loc = gimple_location (stmt); > + /* infer_nonnull_range needs flag_delete_null_pointer_checks set, > + while for nonnull sanitization it is clear. */ > + int save_flag_delete_null_pointer_checks = flag_delete_null_pointer_checks; > + flag_delete_null_pointer_checks = 1; > + for (unsigned int i = 0; i < gimple_call_num_args (stmt); i++) > + { > + tree arg = gimple_call_arg (stmt, i); > + if (POINTER_TYPE_P (TREE_TYPE (arg)) > + && infer_nonnull_range (stmt, arg, false, true)) > + { > + gimple g; > + if (!is_gimple_val (arg)) > + { > + g = gimple_build_assign (make_ssa_name (TREE_TYPE (arg), NULL), > + arg); > + gimple_set_location (g, loc); > + gsi_insert_before (gsi, g, GSI_SAME_STMT); > + arg = gimple_assign_lhs (g); > + } > + > + basic_block then_bb, fallthru_bb; > + *gsi = create_cond_insert_point (gsi, true, false, true, > + &then_bb, &fallthru_bb); > + g = gimple_build_cond (EQ_EXPR, arg, > + build_zero_cst (TREE_TYPE (arg)), > + NULL_TREE, NULL_TREE); > + gimple_set_location (g, loc); > + gsi_insert_after (gsi, g, GSI_NEW_STMT); > + > + *gsi = gsi_after_labels (then_bb); > + if (flag_sanitize_undefined_trap_on_error) > + g = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0); > + else > + { > + tree data = ubsan_create_data ("__ubsan_nonnull_arg_data", > + &loc, NULL, NULL_TREE); > + data = build_fold_addr_expr_loc (loc, data); > + enum built_in_function bcode > + = flag_sanitize_recover > + ? BUILT_IN_UBSAN_HANDLE_NONNULL_ARG > + : BUILT_IN_UBSAN_HANDLE_NONNULL_ARG_ABORT; > + tree fn = builtin_decl_explicit (bcode); > + > + g = gimple_build_call (fn, 2, data, > + build_int_cst (pointer_sized_int_node, > + i + 1)); > + } > + gimple_set_location (g, loc); > + gsi_insert_before (gsi, g, GSI_SAME_STMT); > + } > + *gsi = gsi_for_stmt (stmt); > + } > + flag_delete_null_pointer_checks = save_flag_delete_null_pointer_checks; > +} > + > +/* Instrument returns in functions with returns_nonnull attribute. */ > + > +static void > +instrument_nonnull_return (gimple_stmt_iterator *gsi) > +{ > + gimple stmt = gsi_stmt (*gsi); > + location_t loc = gimple_location (stmt); > + tree arg = gimple_return_retval (stmt); > + /* infer_nonnull_range needs flag_delete_null_pointer_checks set, > + while for nonnull return sanitization it is clear. */ > + int save_flag_delete_null_pointer_checks = flag_delete_null_pointer_checks; > + flag_delete_null_pointer_checks = 1; > + if (arg > + && POINTER_TYPE_P (TREE_TYPE (arg)) > + && is_gimple_val (arg) > + && infer_nonnull_range (stmt, arg, false, true)) > + { > + basic_block then_bb, fallthru_bb; > + *gsi = create_cond_insert_point (gsi, true, false, true, > + &then_bb, &fallthru_bb); > + gimple g = gimple_build_cond (EQ_EXPR, arg, > + build_zero_cst (TREE_TYPE (arg)), > + NULL_TREE, NULL_TREE); > + gimple_set_location (g, loc); > + gsi_insert_after (gsi, g, GSI_NEW_STMT); > + > + *gsi = gsi_after_labels (then_bb); > + if (flag_sanitize_undefined_trap_on_error) > + g = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0); > + else > + { > + tree data = ubsan_create_data ("__ubsan_nonnull_return_data", > + &loc, NULL, NULL_TREE); > + data = build_fold_addr_expr_loc (loc, data); > + enum built_in_function bcode > + = flag_sanitize_recover > + ? BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN > + : BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN_ABORT; > + tree fn = builtin_decl_explicit (bcode); > + > + g = gimple_build_call (fn, 1, data); > + } > + gimple_set_location (g, loc); > + gsi_insert_before (gsi, g, GSI_SAME_STMT); > + *gsi = gsi_for_stmt (stmt); > + } > + flag_delete_null_pointer_checks = save_flag_delete_null_pointer_checks; > +} > + > namespace { > > const pass_data pass_data_ubsan = > @@ -1148,7 +1260,8 @@ public: > virtual bool gate (function *) > { > return flag_sanitize & (SANITIZE_NULL | SANITIZE_SI_OVERFLOW > - | SANITIZE_BOOL | SANITIZE_ENUM); > + | SANITIZE_BOOL | SANITIZE_ENUM > + | SANITIZE_NONNULL | SANITIZE_RETURNS_NONNULL); > } > > virtual unsigned int execute (function *); > @@ -1188,7 +1301,25 @@ pass_ubsan::execute (function *fun) > > if (flag_sanitize & (SANITIZE_BOOL | SANITIZE_ENUM) > && gimple_assign_load_p (stmt)) > - instrument_bool_enum_load (&gsi); > + { > + instrument_bool_enum_load (&gsi); > + bb = gimple_bb (stmt); > + } > + > + if ((flag_sanitize & SANITIZE_NONNULL) > + && is_gimple_call (stmt) > + && !gimple_call_internal_p (stmt)) > + { > + instrument_nonnull_arg (&gsi); > + bb = gimple_bb (stmt); > + } > + > + if ((flag_sanitize & SANITIZE_RETURNS_NONNULL) > + && gimple_code (stmt) == GIMPLE_RETURN) > + { > + instrument_nonnull_return (&gsi); > + bb = gimple_bb (stmt); > + } > > gsi_next (&gsi); > } > --- gcc/testsuite/c-c++-common/ubsan/nonnull-1.c.jj 2014-06-26 18:19:27.655457620 +0200 > +++ gcc/testsuite/c-c++-common/ubsan/nonnull-1.c 2014-06-26 18:27:22.038955861 +0200 > @@ -0,0 +1,38 @@ > +/* { dg-do run } */ > +/* { dg-options "-fsanitize=nonnull,returns-nonnull" } */ > + > +int q, r; > +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; > + > +__attribute__((returns_nonnull, nonnull (1, 3))) > +void * > +foo (void *p, void *q, void *r) > +{ > + a = p; > + b = r; > + return q; > +} > + > +int > +bar (const void *a, const void *b) > +{ > + int c = *(const int *) a; > + int d = *(const int *) b; > + return c - d; > +} > + > +int > +main () > +{ > + asm volatile ("" : : : "memory"); > + d = foo (c, b, c); > + e = foo (e, c, f); > + g = foo (c, f, g); > + __builtin_memset (d, '\0', q); > + return 0; > +} > + > +/* { dg-output "\.c:13:\[0-9]*:\[^\n\r]*null return value where non-null required\[^\n\r]*(\n|\r\n|\r)" } */ > +/* { dg-output "\[^\n\r]*\.c:29:\[0-9]*:\[^\n\r]*null argument where non-null required .argument 1.\[^\n\r]*(\n|\r\n|\r)" } */ > +/* { dg-output "\[^\n\r]*\.c:30:\[0-9]*:\[^\n\r]*null argument where non-null required .argument 3.\[^\n\r]*(\n|\r\n|\r)" } */ > +/* { dg-output "\[^\n\r]*\.c:31:\[0-9]*:\[^\n\r]*null argument where non-null required .argument 1." } */ > --- gcc/testsuite/c-c++-common/ubsan/nonnull-2.c.jj 2014-06-26 18:28:36.151565589 +0200 > +++ gcc/testsuite/c-c++-common/ubsan/nonnull-2.c 2014-06-26 18:29:36.392248392 +0200 > @@ -0,0 +1,36 @@ > +/* { dg-do run } */ > +/* { dg-shouldfail "ubsan" } */ > +/* { dg-options "-fsanitize=undefined -fno-sanitize-recover" } */ > + > +int q, r; > +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; > + > +__attribute__((returns_nonnull, nonnull (1, 3))) > +void * > +foo (void *p, void *q, void *r) > +{ > + a = p; > + b = r; > + return q; > +} > + > +int > +bar (const void *a, const void *b) > +{ > + int c = *(const int *) a; > + int d = *(const int *) b; > + return c - d; > +} > + > +int > +main () > +{ > + asm volatile ("" : : : "memory"); > + d = foo (c, b, c); > + e = foo (e, c, f); > + g = foo (c, f, g); > + __builtin_memset (d, '\0', q); > + return 0; > +} > + > +/* { dg-output "\.c:14:\[0-9]*:\[^\n\r]*null return value where non-null required" } */ > --- gcc/testsuite/c-c++-common/ubsan/nonnull-3.c.jj 2014-06-26 18:29:50.883172086 +0200 > +++ gcc/testsuite/c-c++-common/ubsan/nonnull-3.c 2014-06-26 18:31:39.152601753 +0200 > @@ -0,0 +1,36 @@ > +/* { dg-do run } */ > +/* { dg-shouldfail "ubsan" } */ > +/* { dg-options "-fsanitize=undefined -fno-sanitize-recover" } */ > + > +int q, r; > +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; > + > +__attribute__((returns_nonnull, nonnull (1, 3))) > +void * > +foo (void *p, void *q, void *r) > +{ > + a = p; > + b = r; > + return q; > +} > + > +int > +bar (const void *a, const void *b) > +{ > + int c = *(const int *) a; > + int d = *(const int *) b; > + return c - d; > +} > + > +int > +main () > +{ > + asm volatile ("" : : : "memory"); > + d = foo (c, (void *) &r, c); > + e = foo (e, c, f); > + g = foo (c, f, g); > + __builtin_memset (d, '\0', q); > + return 0; > +} > + > +/* { dg-output "\.c:30:\[0-9]*:\[^\n\r]*null argument where non-null required .argument 1." } */ > --- gcc/testsuite/c-c++-common/ubsan/nonnull-4.c.jj 2014-06-26 18:31:49.118552359 +0200 > +++ gcc/testsuite/c-c++-common/ubsan/nonnull-4.c 2014-06-26 18:32:13.526423944 +0200 > @@ -0,0 +1,34 @@ > +/* { dg-do run } */ > +/* { dg-shouldfail "ubsan" } */ > +/* { dg-options "-fsanitize=undefined -fsanitize-undefined-trap-on-error" } */ > + > +int q, r; > +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; > + > +__attribute__((returns_nonnull, nonnull (1, 3))) > +void * > +foo (void *p, void *q, void *r) > +{ > + a = p; > + b = r; > + return q; > +} > + > +int > +bar (const void *a, const void *b) > +{ > + int c = *(const int *) a; > + int d = *(const int *) b; > + return c - d; > +} > + > +int > +main () > +{ > + asm volatile ("" : : : "memory"); > + d = foo (c, b, c); > + e = foo (e, c, f); > + g = foo (c, f, g); > + __builtin_memset (d, '\0', q); > + return 0; > +} > --- gcc/testsuite/c-c++-common/ubsan/nonnull-5.c.jj 2014-06-26 18:32:23.284367076 +0200 > +++ gcc/testsuite/c-c++-common/ubsan/nonnull-5.c 2014-06-26 18:33:42.154968619 +0200 > @@ -0,0 +1,34 @@ > +/* { dg-do run } */ > +/* { dg-shouldfail "ubsan" } */ > +/* { dg-options "-fsanitize=undefined -fsanitize-undefined-trap-on-error" } */ > + > +int q, r; > +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; > + > +__attribute__((returns_nonnull, nonnull (1, 3))) > +void * > +foo (void *p, void *q, void *r) > +{ > + a = p; > + b = r; > + return q; > +} > + > +int > +bar (const void *a, const void *b) > +{ > + int c = *(const int *) a; > + int d = *(const int *) b; > + return c - d; > +} > + > +int > +main () > +{ > + asm volatile ("" : : : "memory"); > + d = foo (c, (void *) &r, c); > + e = foo (e, c, f); > + g = foo (c, f, g); > + __builtin_memset (d, '\0', q); > + return 0; > +} > --- libsanitizer/ubsan/ubsan_handlers.cc.jj 2013-12-05 12:04:32.000000000 +0100 > +++ libsanitizer/ubsan/ubsan_handlers.cc 2014-06-26 17:03:48.018251367 +0200 > @@ -277,3 +277,31 @@ void __ubsan::__ubsan_handle_function_ty > __ubsan_handle_function_type_mismatch(Data, Function); > Die(); > } > + > +void __ubsan::__ubsan_handle_nonnull_arg(NonNullArgData *Data, uptr ArgNo) { > + SourceLocation Loc = Data->Loc.acquire(); > + if (Loc.isDisabled()) > + return; > + > + Diag(Loc, DL_Error, "null argument where non-null required " > + "(argument %0)") << ArgNo; > +} > + > +void __ubsan::__ubsan_handle_nonnull_arg_abort(NonNullArgData *Data, > + uptr ArgNo) { > + __ubsan::__ubsan_handle_nonnull_arg(Data, ArgNo); > + Die(); > +} > + > +void __ubsan::__ubsan_handle_nonnull_return(NonNullRetData *Data) { > + SourceLocation Loc = Data->Loc.acquire(); > + if (Loc.isDisabled()) > + return; > + > + Diag(Loc, DL_Error, "null return value where non-null required"); > +} > + > +void __ubsan::__ubsan_handle_nonnull_return_abort(NonNullRetData *Data) { > + __ubsan::__ubsan_handle_nonnull_return(Data); > + Die(); > +} > --- libsanitizer/ubsan/ubsan_handlers.h.jj 2013-12-05 12:04:32.000000000 +0100 > +++ libsanitizer/ubsan/ubsan_handlers.h 2014-06-26 17:02:03.330803480 +0200 > @@ -119,6 +119,20 @@ RECOVERABLE(function_type_mismatch, > FunctionTypeMismatchData *Data, > ValueHandle Val) > > +struct NonNullArgData { > + SourceLocation Loc; > +}; > + > +/// \brief Handle passing null to function argument with nonnull attribute. > +RECOVERABLE(nonnull_arg, NonNullArgData *Data, uptr ArgNo) > + > +struct NonNullRetData { > + SourceLocation Loc; > +}; > + > +/// \brief Handle returning null from function with returns_nonnull attribute. > +RECOVERABLE(nonnull_return, NonNullRetData *Data) > + > } > > #endif // UBSAN_HANDLERS_H > > Jakub > >
On Fri, 27 Jun 2014, Jakub Jelinek wrote: > This patch implements sanitization for nonnull and returns_nonnull > attributes. No documentation patch? Also, should this be documented in gcc-4.10/changes.html? > As GCC 4.9.0+ now aggressively optimizes based on these attributes and we've > seen several issues in real world apps, I think this is really needed. Nice! Is this worth pushing into GCC 4.9.1 as well? Gerald
On Sat, Jun 28, 2014 at 01:29:47PM +0200, Gerald Pfeifer wrote: > On Fri, 27 Jun 2014, Jakub Jelinek wrote: > > This patch implements sanitization for nonnull and returns_nonnull > > attributes. > > No documentation patch? I'll add it soon. > Also, should this be documented in gcc-4.10/changes.html? We'll mention all the new undefined behavior sanitizers, there have been many added already post 4.9.0. > > As GCC 4.9.0+ now aggressively optimizes based on these attributes and we've > > seen several issues in real world apps, I think this is really needed. > > Nice! Is this worth pushing into GCC 4.9.1 as well? This one is the only one that is actually hard to push into 4.9.x, as it requires new libubsan entrypoints. Jakub
--- gcc/flag-types.h.jj 2014-06-20 23:26:24.000000000 +0200 +++ gcc/flag-types.h 2014-06-26 14:39:44.133970103 +0200 @@ -231,10 +231,13 @@ enum sanitize_code { SANITIZE_FLOAT_DIVIDE = 1 << 12, SANITIZE_FLOAT_CAST = 1 << 13, SANITIZE_BOUNDS = 1 << 14, + SANITIZE_NONNULL = 1 << 15, + SANITIZE_RETURNS_NONNULL = 1 << 16, SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE | SANITIZE_UNREACHABLE | SANITIZE_VLA | SANITIZE_NULL | SANITIZE_RETURN | SANITIZE_SI_OVERFLOW | SANITIZE_BOOL | SANITIZE_ENUM - | SANITIZE_BOUNDS, + | SANITIZE_BOUNDS | SANITIZE_NONNULL + | SANITIZE_RETURNS_NONNULL, SANITIZE_NONDEFAULT = SANITIZE_FLOAT_DIVIDE | SANITIZE_FLOAT_CAST }; --- gcc/opts.c.jj 2014-06-20 23:26:23.000000000 +0200 +++ gcc/opts.c 2014-06-26 14:43:51.620652007 +0200 @@ -1468,6 +1468,9 @@ common_handle_option (struct gcc_options { "float-cast-overflow", SANITIZE_FLOAT_CAST, sizeof "float-cast-overflow" - 1 }, { "bounds", SANITIZE_BOUNDS, sizeof "bounds" - 1 }, + { "nonnull", SANITIZE_NONNULL, sizeof "nonnull" - 1 }, + { "returns-nonnull", SANITIZE_RETURNS_NONNULL, + sizeof "returns-nonnull" - 1 }, { NULL, 0, 0 } }; const char *comma; @@ -1511,7 +1514,8 @@ common_handle_option (struct gcc_options /* When instrumenting the pointers, we don't want to remove the null pointer checks. */ - if (flag_sanitize & SANITIZE_NULL) + if (flag_sanitize & (SANITIZE_NULL | SANITIZE_NONNULL + | SANITIZE_RETURNS_NONNULL)) opts->x_flag_delete_null_pointer_checks = 0; break; } --- gcc/sanitizer.def.jj 2014-06-20 23:26:23.000000000 +0200 +++ gcc/sanitizer.def 2014-06-26 16:52:34.644801331 +0200 @@ -417,3 +417,19 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HAN "__ubsan_handle_out_of_bounds_abort", BT_FN_VOID_PTR_PTR, ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_NONNULL_ARG, + "__ubsan_handle_nonnull_arg", + BT_FN_VOID_PTR_PTRMODE, + ATTR_COLD_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_NONNULL_ARG_ABORT, + "__ubsan_handle_nonnull_arg_abort", + BT_FN_VOID_PTR_PTRMODE, + ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN, + "__ubsan_handle_nonnull_return", + BT_FN_VOID_PTR, + ATTR_COLD_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN_ABORT, + "__ubsan_handle_nonnull_return_abort", + BT_FN_VOID_PTR, + ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST) --- gcc/ubsan.c.jj 2014-06-20 23:26:24.000000000 +0200 +++ gcc/ubsan.c 2014-06-26 18:23:28.190191706 +0200 @@ -997,6 +997,7 @@ instrument_bool_enum_load (gimple_stmt_i } gimple_set_location (g, loc); gsi_insert_before (&gsi2, g, GSI_SAME_STMT); + *gsi = gsi_for_stmt (stmt); } /* Instrument float point-to-integer conversion. TYPE is an integer type of @@ -1121,6 +1122,117 @@ ubsan_instrument_float_cast (location_t fn, integer_zero_node); } +/* Instrument values passed to function arguments with nonnull attribute. */ + +static void +instrument_nonnull_arg (gimple_stmt_iterator *gsi) +{ + gimple stmt = gsi_stmt (*gsi); + location_t loc = gimple_location (stmt); + /* infer_nonnull_range needs flag_delete_null_pointer_checks set, + while for nonnull sanitization it is clear. */ + int save_flag_delete_null_pointer_checks = flag_delete_null_pointer_checks; + flag_delete_null_pointer_checks = 1; + for (unsigned int i = 0; i < gimple_call_num_args (stmt); i++) + { + tree arg = gimple_call_arg (stmt, i); + if (POINTER_TYPE_P (TREE_TYPE (arg)) + && infer_nonnull_range (stmt, arg, false, true)) + { + gimple g; + if (!is_gimple_val (arg)) + { + g = gimple_build_assign (make_ssa_name (TREE_TYPE (arg), NULL), + arg); + gimple_set_location (g, loc); + gsi_insert_before (gsi, g, GSI_SAME_STMT); + arg = gimple_assign_lhs (g); + } + + basic_block then_bb, fallthru_bb; + *gsi = create_cond_insert_point (gsi, true, false, true, + &then_bb, &fallthru_bb); + g = gimple_build_cond (EQ_EXPR, arg, + build_zero_cst (TREE_TYPE (arg)), + NULL_TREE, NULL_TREE); + gimple_set_location (g, loc); + gsi_insert_after (gsi, g, GSI_NEW_STMT); + + *gsi = gsi_after_labels (then_bb); + if (flag_sanitize_undefined_trap_on_error) + g = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0); + else + { + tree data = ubsan_create_data ("__ubsan_nonnull_arg_data", + &loc, NULL, NULL_TREE); + data = build_fold_addr_expr_loc (loc, data); + enum built_in_function bcode + = flag_sanitize_recover + ? BUILT_IN_UBSAN_HANDLE_NONNULL_ARG + : BUILT_IN_UBSAN_HANDLE_NONNULL_ARG_ABORT; + tree fn = builtin_decl_explicit (bcode); + + g = gimple_build_call (fn, 2, data, + build_int_cst (pointer_sized_int_node, + i + 1)); + } + gimple_set_location (g, loc); + gsi_insert_before (gsi, g, GSI_SAME_STMT); + } + *gsi = gsi_for_stmt (stmt); + } + flag_delete_null_pointer_checks = save_flag_delete_null_pointer_checks; +} + +/* Instrument returns in functions with returns_nonnull attribute. */ + +static void +instrument_nonnull_return (gimple_stmt_iterator *gsi) +{ + gimple stmt = gsi_stmt (*gsi); + location_t loc = gimple_location (stmt); + tree arg = gimple_return_retval (stmt); + /* infer_nonnull_range needs flag_delete_null_pointer_checks set, + while for nonnull return sanitization it is clear. */ + int save_flag_delete_null_pointer_checks = flag_delete_null_pointer_checks; + flag_delete_null_pointer_checks = 1; + if (arg + && POINTER_TYPE_P (TREE_TYPE (arg)) + && is_gimple_val (arg) + && infer_nonnull_range (stmt, arg, false, true)) + { + basic_block then_bb, fallthru_bb; + *gsi = create_cond_insert_point (gsi, true, false, true, + &then_bb, &fallthru_bb); + gimple g = gimple_build_cond (EQ_EXPR, arg, + build_zero_cst (TREE_TYPE (arg)), + NULL_TREE, NULL_TREE); + gimple_set_location (g, loc); + gsi_insert_after (gsi, g, GSI_NEW_STMT); + + *gsi = gsi_after_labels (then_bb); + if (flag_sanitize_undefined_trap_on_error) + g = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0); + else + { + tree data = ubsan_create_data ("__ubsan_nonnull_return_data", + &loc, NULL, NULL_TREE); + data = build_fold_addr_expr_loc (loc, data); + enum built_in_function bcode + = flag_sanitize_recover + ? BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN + : BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN_ABORT; + tree fn = builtin_decl_explicit (bcode); + + g = gimple_build_call (fn, 1, data); + } + gimple_set_location (g, loc); + gsi_insert_before (gsi, g, GSI_SAME_STMT); + *gsi = gsi_for_stmt (stmt); + } + flag_delete_null_pointer_checks = save_flag_delete_null_pointer_checks; +} + namespace { const pass_data pass_data_ubsan = @@ -1148,7 +1260,8 @@ public: virtual bool gate (function *) { return flag_sanitize & (SANITIZE_NULL | SANITIZE_SI_OVERFLOW - | SANITIZE_BOOL | SANITIZE_ENUM); + | SANITIZE_BOOL | SANITIZE_ENUM + | SANITIZE_NONNULL | SANITIZE_RETURNS_NONNULL); } virtual unsigned int execute (function *); @@ -1188,7 +1301,25 @@ pass_ubsan::execute (function *fun) if (flag_sanitize & (SANITIZE_BOOL | SANITIZE_ENUM) && gimple_assign_load_p (stmt)) - instrument_bool_enum_load (&gsi); + { + instrument_bool_enum_load (&gsi); + bb = gimple_bb (stmt); + } + + if ((flag_sanitize & SANITIZE_NONNULL) + && is_gimple_call (stmt) + && !gimple_call_internal_p (stmt)) + { + instrument_nonnull_arg (&gsi); + bb = gimple_bb (stmt); + } + + if ((flag_sanitize & SANITIZE_RETURNS_NONNULL) + && gimple_code (stmt) == GIMPLE_RETURN) + { + instrument_nonnull_return (&gsi); + bb = gimple_bb (stmt); + } gsi_next (&gsi); } --- gcc/testsuite/c-c++-common/ubsan/nonnull-1.c.jj 2014-06-26 18:19:27.655457620 +0200 +++ gcc/testsuite/c-c++-common/ubsan/nonnull-1.c 2014-06-26 18:27:22.038955861 +0200 @@ -0,0 +1,38 @@ +/* { dg-do run } */ +/* { dg-options "-fsanitize=nonnull,returns-nonnull" } */ + +int q, r; +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; + +__attribute__((returns_nonnull, nonnull (1, 3))) +void * +foo (void *p, void *q, void *r) +{ + a = p; + b = r; + return q; +} + +int +bar (const void *a, const void *b) +{ + int c = *(const int *) a; + int d = *(const int *) b; + return c - d; +} + +int +main () +{ + asm volatile ("" : : : "memory"); + d = foo (c, b, c); + e = foo (e, c, f); + g = foo (c, f, g); + __builtin_memset (d, '\0', q); + return 0; +} + +/* { dg-output "\.c:13:\[0-9]*:\[^\n\r]*null return value where non-null required\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*\.c:29:\[0-9]*:\[^\n\r]*null argument where non-null required .argument 1.\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*\.c:30:\[0-9]*:\[^\n\r]*null argument where non-null required .argument 3.\[^\n\r]*(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*\.c:31:\[0-9]*:\[^\n\r]*null argument where non-null required .argument 1." } */ --- gcc/testsuite/c-c++-common/ubsan/nonnull-2.c.jj 2014-06-26 18:28:36.151565589 +0200 +++ gcc/testsuite/c-c++-common/ubsan/nonnull-2.c 2014-06-26 18:29:36.392248392 +0200 @@ -0,0 +1,36 @@ +/* { dg-do run } */ +/* { dg-shouldfail "ubsan" } */ +/* { dg-options "-fsanitize=undefined -fno-sanitize-recover" } */ + +int q, r; +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; + +__attribute__((returns_nonnull, nonnull (1, 3))) +void * +foo (void *p, void *q, void *r) +{ + a = p; + b = r; + return q; +} + +int +bar (const void *a, const void *b) +{ + int c = *(const int *) a; + int d = *(const int *) b; + return c - d; +} + +int +main () +{ + asm volatile ("" : : : "memory"); + d = foo (c, b, c); + e = foo (e, c, f); + g = foo (c, f, g); + __builtin_memset (d, '\0', q); + return 0; +} + +/* { dg-output "\.c:14:\[0-9]*:\[^\n\r]*null return value where non-null required" } */ --- gcc/testsuite/c-c++-common/ubsan/nonnull-3.c.jj 2014-06-26 18:29:50.883172086 +0200 +++ gcc/testsuite/c-c++-common/ubsan/nonnull-3.c 2014-06-26 18:31:39.152601753 +0200 @@ -0,0 +1,36 @@ +/* { dg-do run } */ +/* { dg-shouldfail "ubsan" } */ +/* { dg-options "-fsanitize=undefined -fno-sanitize-recover" } */ + +int q, r; +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; + +__attribute__((returns_nonnull, nonnull (1, 3))) +void * +foo (void *p, void *q, void *r) +{ + a = p; + b = r; + return q; +} + +int +bar (const void *a, const void *b) +{ + int c = *(const int *) a; + int d = *(const int *) b; + return c - d; +} + +int +main () +{ + asm volatile ("" : : : "memory"); + d = foo (c, (void *) &r, c); + e = foo (e, c, f); + g = foo (c, f, g); + __builtin_memset (d, '\0', q); + return 0; +} + +/* { dg-output "\.c:30:\[0-9]*:\[^\n\r]*null argument where non-null required .argument 1." } */ --- gcc/testsuite/c-c++-common/ubsan/nonnull-4.c.jj 2014-06-26 18:31:49.118552359 +0200 +++ gcc/testsuite/c-c++-common/ubsan/nonnull-4.c 2014-06-26 18:32:13.526423944 +0200 @@ -0,0 +1,34 @@ +/* { dg-do run } */ +/* { dg-shouldfail "ubsan" } */ +/* { dg-options "-fsanitize=undefined -fsanitize-undefined-trap-on-error" } */ + +int q, r; +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; + +__attribute__((returns_nonnull, nonnull (1, 3))) +void * +foo (void *p, void *q, void *r) +{ + a = p; + b = r; + return q; +} + +int +bar (const void *a, const void *b) +{ + int c = *(const int *) a; + int d = *(const int *) b; + return c - d; +} + +int +main () +{ + asm volatile ("" : : : "memory"); + d = foo (c, b, c); + e = foo (e, c, f); + g = foo (c, f, g); + __builtin_memset (d, '\0', q); + return 0; +} --- gcc/testsuite/c-c++-common/ubsan/nonnull-5.c.jj 2014-06-26 18:32:23.284367076 +0200 +++ gcc/testsuite/c-c++-common/ubsan/nonnull-5.c 2014-06-26 18:33:42.154968619 +0200 @@ -0,0 +1,34 @@ +/* { dg-do run } */ +/* { dg-shouldfail "ubsan" } */ +/* { dg-options "-fsanitize=undefined -fsanitize-undefined-trap-on-error" } */ + +int q, r; +void *a, *b, *c = (void *) &q, *d, *e, *f = (void *) &q, *g, *h; + +__attribute__((returns_nonnull, nonnull (1, 3))) +void * +foo (void *p, void *q, void *r) +{ + a = p; + b = r; + return q; +} + +int +bar (const void *a, const void *b) +{ + int c = *(const int *) a; + int d = *(const int *) b; + return c - d; +} + +int +main () +{ + asm volatile ("" : : : "memory"); + d = foo (c, (void *) &r, c); + e = foo (e, c, f); + g = foo (c, f, g); + __builtin_memset (d, '\0', q); + return 0; +} --- libsanitizer/ubsan/ubsan_handlers.cc.jj 2013-12-05 12:04:32.000000000 +0100 +++ libsanitizer/ubsan/ubsan_handlers.cc 2014-06-26 17:03:48.018251367 +0200 @@ -277,3 +277,31 @@ void __ubsan::__ubsan_handle_function_ty __ubsan_handle_function_type_mismatch(Data, Function); Die(); } + +void __ubsan::__ubsan_handle_nonnull_arg(NonNullArgData *Data, uptr ArgNo) { + SourceLocation Loc = Data->Loc.acquire(); + if (Loc.isDisabled()) + return; + + Diag(Loc, DL_Error, "null argument where non-null required " + "(argument %0)") << ArgNo; +} + +void __ubsan::__ubsan_handle_nonnull_arg_abort(NonNullArgData *Data, + uptr ArgNo) { + __ubsan::__ubsan_handle_nonnull_arg(Data, ArgNo); + Die(); +} + +void __ubsan::__ubsan_handle_nonnull_return(NonNullRetData *Data) { + SourceLocation Loc = Data->Loc.acquire(); + if (Loc.isDisabled()) + return; + + Diag(Loc, DL_Error, "null return value where non-null required"); +} + +void __ubsan::__ubsan_handle_nonnull_return_abort(NonNullRetData *Data) { + __ubsan::__ubsan_handle_nonnull_return(Data); + Die(); +} --- libsanitizer/ubsan/ubsan_handlers.h.jj 2013-12-05 12:04:32.000000000 +0100 +++ libsanitizer/ubsan/ubsan_handlers.h 2014-06-26 17:02:03.330803480 +0200 @@ -119,6 +119,20 @@ RECOVERABLE(function_type_mismatch, FunctionTypeMismatchData *Data, ValueHandle Val) +struct NonNullArgData { + SourceLocation Loc; +}; + +/// \brief Handle passing null to function argument with nonnull attribute. +RECOVERABLE(nonnull_arg, NonNullArgData *Data, uptr ArgNo) + +struct NonNullRetData { + SourceLocation Loc; +}; + +/// \brief Handle returning null from function with returns_nonnull attribute. +RECOVERABLE(nonnull_return, NonNullRetData *Data) + } #endif // UBSAN_HANDLERS_H
Hi! This patch implements sanitization for nonnull and returns_nonnull attributes. Similarly to -fsanitize=null this option disables -fdelete-null-pointer-checks silently, and adds checks before calling functions with declared nonnull arguments if we don't pass NULL there, and before return in functions declared with returns_nonnull also inserts check whether the return value is not NULL. As GCC 4.9.0+ now aggressively optimizes based on these attributes and we've seen several issues in real world apps, I think this is really needed. Bootstrapped/regtested on x86_64-linux and i686-linux normally (yes,rtl checking) and c,c++,fortran release checking bootstrap-ubsan. Will post bugs in gcc discovered by it later. The patch adds two new (trivial handlers) to libubsan, as it is maintained in llvm's compiler-rt, will talk to them if they are interested in those and what exact wording and form (AFAIK clang also added the gcc {,returns_}nonnull attributes). If they wouldn't be interested, guess we could add them in a separate, gcc owned, source file in ubsan (like we own Makefile*). 2014-06-27 Jakub Jelinek <jakub@redhat.com> * flag-types.h (enum sanitize_code): Add SANITIZE_NONNULL and SANITIZE_RETURNS_NONNULL, or them into SANITIZE_UNDEFINED. * opts.c (common_handle_option): Handle SANITIZE_NONNULL and SANITIZE_RETURNS_NONNULL and disable flag_delete_null_pointer_checks for them. * sanitizer.def (BUILT_IN_UBSAN_HANDLE_NONNULL_ARG, BUILT_IN_UBSAN_HANDLE_NONNULL_ARG_ABORT, BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN, BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN_ABORT): New. * ubsan.c (instrument_bool_enum_load): Set *gsi back to stmt's iterator. (instrument_nonnull_arg, instrument_nonnull_return): New functions. (pass_ubsan::gate): Return true even for SANITIZE_NONNULL or SANITIZE_RETURNS_NONNULL. (pass_ubsan::execute): Call instrument_nonnull_{arg,return}. * c-c++-common/ubsan/nonnull-1.c: New test. * c-c++-common/ubsan/nonnull-2.c: New test. * c-c++-common/ubsan/nonnull-3.c: New test. * c-c++-common/ubsan/nonnull-4.c: New test. * c-c++-common/ubsan/nonnull-5.c: New test. Jakub