Re: [PATCH] -fsanitize-recover=list

[Alexey Samsonov <samsonov at google dot com>]

Hi Jakub,

I've just prepared a patch implementing -fsanitize-recover=<list> in
Clang (http://reviews.llvm.org/D6302), writing here to make sure we're
on
the same page w.r.t. flag semantics:

* -fsanitize-recover: Enable recovery for all checks enabled by
-fsanitize= which support it.
* -fno-sanitize-recover: Disable recovery for all checks.
* -fsanitize-recover=<list>: Enable recovery for all selected checks
or group of checks. It is forbidden to list unrecoverable sanitizers
here (e.g., "-fsanitize-recover=address" will produce an error).
* -fno-sanitize-recover=<list>: Disable recovery for selected checks
or group of checks.

We maintain the set of "recoverable" sanitizers. Initially it contains
all UBSan checks (except for "unreachable" and "return"). We then
update this set as we
parse flags listed above, from left to right. "-fsanitize-recover" are
"-fsanitize" flags are independent. If a sanitizer is listed as
recoverable, but is not enabled via -fsanitize=,
we just ignore corresponding -fsanitize-recover flag.

Examples:
  * $(CC) -fsanitize=undefined,thread -fsanitize-recover=thread  //
Use UBSan and TSan, both of them are recoverable (Note: in reality,
"-fsanitize-recover=thread" is not implemented in Clang, this example
just illustrates intended semantics).
  * $(CC) -fsanitize=undefined -fno-sanitize-recover
-fsanitize-recover=signed-integer-overflow // Recover only from
signed-integer-overflow check, all other UBSan checks are fatal.

Sounds good?

As for the generated code, I'm at the stage where I can implement the
following: if a single UBSan hander is used to report multiple error
kinds (__ubsan_handle_type_mismatch is used for
-fsanitize=null,alignment,object-size), and these kinds have different
recoverability, then we emit two handler calls like this:

$ (CC) -fsanitize=undefined -fno-sanitize-recover=alignment   //
"null" and "object-size" are recoverable by default.

bool IsNonNull = ...;
bool IsAligned = ...;
bool HasCorrectSize = ...;
bool OK = IsAligned && IsNonNull && HasCorrectSize;
if (UNLIKELY(!OK)) {
  // setup arguments for UBSan handler call
  if (!IsAligned) {
    __ubsan_handle_type_mismatch_abort(<args>);
    __builtin_unreachable();
  }
  __ubsan_handle_type_mismatch(<args>);
}
// user code



On Fri, Oct 17, 2014 at 9:13 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Oct 13, 2014 at 02:47:07PM +0400, Yury Gribov wrote:
>> On 09/30/2014 09:39 PM, Jakub Jelinek wrote:
>> >LGTM, will hack it up soon in GCC then.
>>
>> Do you plan to work on this in near future?
>
> Here is only very lightly tested patch, didn't get to updating
> documentation though, plus there is no testsuite coverage for it.
> Supposedly, most of the tests that use -fno-sanitize-recover
> or -fsanitize-recover in dg-options should be changed to use
> -fno-sanitize-recover= or -fsanitize-recover (in some cases for
> the kind that is enabled with -fsanitize= only, in other cases
> perhaps for something covering that and some other options),
> plus perhaps some new smallish tests that test that if you e.g.
> do -fsanitize=undefined -fno-sanitize-recover=divide , that
> you can recover from several say out of bound shifts, but that
> the first divide will terminate, etc.
> Yuri or Marek, would you have spare time for that?
>
> There is one not so nice thing, if one requests e.g.
> -fsanitize=null,alignment -fno-sanitize-recover=null -fsanitize-recover=alignment
> (or vice versa), as a single call is used for both alignment and null
> checks, if both are tested, it needs to pick something, the patch
> right now picks recovery rather than abort if the two bits
> in flag_sanitize_recover disagree.  In theory, we could in that case
> just use two separate calls rather than sharing one call, doing the
> check and conditional branch to *_abort () first and if that wasn't true,
> do the other check.
>
> Also, the patch enables -fsanitize-recover by default for
> -fsanitize-recover=undefined,float-divide-by-zero,float-cast-overflow
> but not for kernel-address and doesn't check
> (flag_sanitize_recover & SANITIZE_KERNEL_ADDRESS) in asan.c, you'd
> need to add that afterwards.
>
> 2014-10-17  Jakub Jelinek  <jakub@redhat.com>
>
>         * common.opt (flag_sanitize_recover): New variable.
>         (fsanitize-recover): Remove Var/Init, deprecate.
>         (fsanitize-recover=): New option.
>         * opts.c (finish_options): Use opts->x_flag_sanitize
>         instead of flag_sanitize.  Formatting.
>         (common_handle_option): Handle OPT_fsanitize_recover_
>         and OPT_fsanitize_recover.  Use opts->x_flag_sanitize
>         instead of flag_sanitize.
>         * asan.c (pass_sanopt::execute): Fix up formatting.
>         * ubsan.c (ubsan_expand_bounds_ifn, ubsan_expand_null_ifn,
>         ubsan_expand_objsize_ifn, ubsan_build_overflow_builtin,
>         instrument_bool_enum_load, ubsan_instrument_float_cast,
>         instrument_nonnull_arg, instrument_nonnull_return): Check
>         bits in flag_sanitize_recover bitmask instead of
>         flag_sanitize_recover as bool flag.
> c-family/
>         * c-ubsan.c (ubsan_instrument_division, ubsan_instrument_shift,
>         ubsan_instrument_vla): Check bits in flag_sanitize_recover bitmask
>         instead of flag_sanitize_recover as bool flag.
>
> --- gcc/common.opt.jj   2014-10-17 08:47:21.000000000 +0200
> +++ gcc/common.opt      2014-10-17 17:45:41.816337133 +0200
> @@ -211,6 +211,10 @@ bool flag_opts_finished
>  Variable
>  unsigned int flag_sanitize
>
> +; What sanitizers should recover from errors
> +Variable
> +unsigned int flag_sanitize_recover = SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT
> +
>  ; Flag whether a prefix has been added to dump_base_name
>  Variable
>  bool dump_base_name_prefixed = false
> @@ -879,10 +883,14 @@ fsanitize=
>  Common Driver Report Joined
>  Select what to sanitize
>
> -fsanitize-recover
> -Common Report Var(flag_sanitize_recover) Init(1)
> +fsanitize-recover=
> +Common Report Joined
>  After diagnosing undefined behavior attempt to continue execution
>
> +fsanitize-recover
> +Common Report
> +This switch is deprecated; use -fsanitize-recover= instead
> +
>  fsanitize-undefined-trap-on-error
>  Common Report Var(flag_sanitize_undefined_trap_on_error) Init(0)
>  Use trap instead of a library function for undefined behavior sanitization
> --- gcc/opts.c.jj       2014-10-17 12:01:19.000000000 +0200
> +++ gcc/opts.c  2014-10-17 17:17:38.620375035 +0200
> @@ -879,17 +879,17 @@ finish_options (struct gcc_options *opts
>
>    /* Userspace and kernel ASan conflict with each other and with TSan.  */
>
> -  if ((flag_sanitize & SANITIZE_USER_ADDRESS)
> -      && (flag_sanitize & SANITIZE_KERNEL_ADDRESS))
> +  if ((opts->x_flag_sanitize & SANITIZE_USER_ADDRESS)
> +      && (opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS))
>      error_at (loc,
> -              "-fsanitize=address is incompatible with "
> -              "-fsanitize=kernel-address");
> +             "-fsanitize=address is incompatible with "
> +             "-fsanitize=kernel-address");
>
> -  if ((flag_sanitize & SANITIZE_ADDRESS)
> -      && (flag_sanitize & SANITIZE_THREAD))
> +  if ((opts->x_flag_sanitize & SANITIZE_ADDRESS)
> +      && (opts->x_flag_sanitize & SANITIZE_THREAD))
>      error_at (loc,
> -              "-fsanitize=address and -fsanitize=kernel-address "
> -              "are incompatible with -fsanitize=thread");
> +             "-fsanitize=address and -fsanitize=kernel-address "
> +             "are incompatible with -fsanitize=thread");
>  }
>
>  #define LEFT_COLUMN    27
> @@ -1473,6 +1473,7 @@ common_handle_option (struct gcc_options
>        break;
>
>      case OPT_fsanitize_:
> +    case OPT_fsanitize_recover_:
>        {
>         const char *p = arg;
>         while (*p != 0)
> @@ -1539,33 +1540,48 @@ common_handle_option (struct gcc_options
>                   && memcmp (p, spec[i].name, len) == 0)
>                 {
>                   /* Handle both -fsanitize and -fno-sanitize cases.  */
> -                 if (value)
> -                   flag_sanitize |= spec[i].flag;
> +                 if (code == OPT_fsanitize_)
> +                   {
> +                     if (value)
> +                       opts->x_flag_sanitize |= spec[i].flag;
> +                     else
> +                       opts->x_flag_sanitize &= ~spec[i].flag;
> +                   }
>                   else
> -                   flag_sanitize &= ~spec[i].flag;
> +                   {
> +                     if (value)
> +                       opts->x_flag_sanitize_recover |= spec[i].flag;
> +                     else
> +                       opts->x_flag_sanitize_recover &= ~spec[i].flag;
> +                   }
>                   found = true;
>                   break;
>                 }
>
>             if (! found)
>               error_at (loc,
> -                       "unrecognized argument to -fsanitize= option: %q.*s",
> -                       (int) len, p);
> +                       code == OPT_fsanitize_
> +                       ? "unrecognized argument to -fsanitize= option: %q.*s"
> +                       : "unrecognized argument to -fsanitize-recover= "
> +                         "option: %q.*s", (int) len, p);
>
>             if (comma == NULL)
>               break;
>             p = comma + 1;
>           }
>
> +       if (code != OPT_fsanitize_)
> +         break;
> +
>         /* When instrumenting the pointers, we don't want to remove
>            the null pointer checks.  */
> -       if (flag_sanitize & (SANITIZE_NULL | SANITIZE_NONNULL_ATTRIBUTE
> -                            | SANITIZE_RETURNS_NONNULL_ATTRIBUTE))
> +       if (opts->x_flag_sanitize & (SANITIZE_NULL | SANITIZE_NONNULL_ATTRIBUTE
> +                                    | SANITIZE_RETURNS_NONNULL_ATTRIBUTE))
>           opts->x_flag_delete_null_pointer_checks = 0;
>
>         /* Kernel ASan implies normal ASan but does not yet support
>            all features.  */
> -       if (flag_sanitize & SANITIZE_KERNEL_ADDRESS)
> +       if (opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS)
>           {
>             maybe_set_param_value (PARAM_ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD, 0,
>                                    opts->x_param_values,
> @@ -1584,6 +1600,15 @@ common_handle_option (struct gcc_options
>         break;
>        }
>
> +    case OPT_fsanitize_recover:
> +      if (value)
> +       opts->x_flag_sanitize_recover
> +         |= SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT;
> +      else
> +       opts->x_flag_sanitize_recover
> +         &= ~(SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT);
> +      break;
> +
>      case OPT_O:
>      case OPT_Os:
>      case OPT_Ofast:
> --- gcc/asan.c.jj       2014-10-13 17:54:34.000000000 +0200
> +++ gcc/asan.c  2014-10-17 17:46:20.952620580 +0200
> @@ -2884,10 +2884,8 @@ pass_sanopt::execute (function *fun)
>                   no_next = ubsan_expand_objsize_ifn (&gsi);
>                   break;
>                 case IFN_ASAN_CHECK:
> -                 {
> -                   no_next = asan_expand_check_ifn (&gsi, use_calls);
> -                   break;
> -                 }
> +                 no_next = asan_expand_check_ifn (&gsi, use_calls);
> +                 break;
>                 default:
>                   break;
>                 }
> --- gcc/ubsan.c.jj      2014-10-10 19:42:22.000000000 +0200
> +++ gcc/ubsan.c 2014-10-17 17:05:57.609330882 +0200
> @@ -638,7 +638,7 @@ ubsan_expand_bounds_ifn (gimple_stmt_ite
>                              NULL_TREE, NULL_TREE);
>        data = build_fold_addr_expr_loc (loc, data);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_BOUNDS)
>           ? BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS
>           : BUILT_IN_UBSAN_HANDLE_OUT_OF_BOUNDS_ABORT;
>        tree fn = builtin_decl_explicit (bcode);
> @@ -741,7 +741,8 @@ ubsan_expand_null_ifn (gimple_stmt_itera
>    else
>      {
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & ((check_align ? SANITIZE_ALIGNMENT : 0)
> +                                   | (check_null ? SANITIZE_NULL : 0)))
>           ? BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH
>           : BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH_ABORT;
>        tree fn = builtin_decl_implicit (bcode);
> @@ -879,7 +880,7 @@ ubsan_expand_objsize_ifn (gimple_stmt_it
>                                  NULL_TREE);
>           data = build_fold_addr_expr_loc (loc, data);
>           enum built_in_function bcode
> -           = flag_sanitize_recover
> +           = (flag_sanitize_recover & SANITIZE_OBJECT_SIZE)
>               ? BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH
>               : BUILT_IN_UBSAN_HANDLE_TYPE_MISMATCH_ABORT;
>           tree p = make_ssa_name (pointer_sized_int_node, NULL);
> @@ -964,22 +965,22 @@ ubsan_build_overflow_builtin (tree_code
>    switch (code)
>      {
>      case PLUS_EXPR:
> -      fn_code = flag_sanitize_recover
> +      fn_code = (flag_sanitize_recover & SANITIZE_SI_OVERFLOW)
>                 ? BUILT_IN_UBSAN_HANDLE_ADD_OVERFLOW
>                 : BUILT_IN_UBSAN_HANDLE_ADD_OVERFLOW_ABORT;
>        break;
>      case MINUS_EXPR:
> -      fn_code = flag_sanitize_recover
> +      fn_code = (flag_sanitize_recover & SANITIZE_SI_OVERFLOW)
>                 ? BUILT_IN_UBSAN_HANDLE_SUB_OVERFLOW
>                 : BUILT_IN_UBSAN_HANDLE_SUB_OVERFLOW_ABORT;
>        break;
>      case MULT_EXPR:
> -      fn_code = flag_sanitize_recover
> +      fn_code = (flag_sanitize_recover & SANITIZE_SI_OVERFLOW)
>                 ? BUILT_IN_UBSAN_HANDLE_MUL_OVERFLOW
>                 : BUILT_IN_UBSAN_HANDLE_MUL_OVERFLOW_ABORT;
>        break;
>      case NEGATE_EXPR:
> -      fn_code = flag_sanitize_recover
> +      fn_code = (flag_sanitize_recover & SANITIZE_SI_OVERFLOW)
>                 ? BUILT_IN_UBSAN_HANDLE_NEGATE_OVERFLOW
>                 : BUILT_IN_UBSAN_HANDLE_NEGATE_OVERFLOW_ABORT;
>        break;
> @@ -1156,7 +1157,8 @@ instrument_bool_enum_load (gimple_stmt_i
>                                      NULL_TREE);
>        data = build_fold_addr_expr_loc (loc, data);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & (TREE_CODE (type) == BOOLEAN_TYPE
> +                                   ? SANITIZE_BOOL : SANITIZE_ENUM))
>           ? BUILT_IN_UBSAN_HANDLE_LOAD_INVALID_VALUE
>           : BUILT_IN_UBSAN_HANDLE_LOAD_INVALID_VALUE_ABORT;
>        tree fn = builtin_decl_explicit (bcode);
> @@ -1278,7 +1280,7 @@ ubsan_instrument_float_cast (location_t
>                                      ubsan_type_descriptor (type), NULL_TREE,
>                                      NULL_TREE);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_FLOAT_CAST)
>           ? BUILT_IN_UBSAN_HANDLE_FLOAT_CAST_OVERFLOW
>           : BUILT_IN_UBSAN_HANDLE_FLOAT_CAST_OVERFLOW_ABORT;
>        fn = builtin_decl_explicit (bcode);
> @@ -1344,7 +1346,7 @@ instrument_nonnull_arg (gimple_stmt_iter
>                                              NULL_TREE);
>               data = build_fold_addr_expr_loc (loc[0], data);
>               enum built_in_function bcode
> -               = flag_sanitize_recover
> +               = (flag_sanitize_recover & SANITIZE_NONNULL_ATTRIBUTE)
>                   ? BUILT_IN_UBSAN_HANDLE_NONNULL_ARG
>                   : BUILT_IN_UBSAN_HANDLE_NONNULL_ARG_ABORT;
>               tree fn = builtin_decl_explicit (bcode);
> @@ -1396,7 +1398,7 @@ instrument_nonnull_return (gimple_stmt_i
>                                          2, loc, NULL_TREE, NULL_TREE);
>           data = build_fold_addr_expr_loc (loc[0], data);
>           enum built_in_function bcode
> -           = flag_sanitize_recover
> +           = (flag_sanitize_recover & SANITIZE_RETURNS_NONNULL_ATTRIBUTE)
>               ? BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN
>               : BUILT_IN_UBSAN_HANDLE_NONNULL_RETURN_ABORT;
>           tree fn = builtin_decl_explicit (bcode);
> --- gcc/c-family/c-ubsan.c.jj   2014-09-10 11:20:49.000000000 +0200
> +++ gcc/c-family/c-ubsan.c      2014-10-17 17:13:14.393241619 +0200
> @@ -104,7 +104,7 @@ ubsan_instrument_division (location_t lo
>                                      NULL_TREE);
>        data = build_fold_addr_expr_loc (loc, data);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_DIVIDE)
>           ? BUILT_IN_UBSAN_HANDLE_DIVREM_OVERFLOW
>           : BUILT_IN_UBSAN_HANDLE_DIVREM_OVERFLOW_ABORT;
>        tt = builtin_decl_explicit (bcode);
> @@ -199,7 +199,7 @@ ubsan_instrument_shift (location_t loc,
>        data = build_fold_addr_expr_loc (loc, data);
>
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_SHIFT)
>           ? BUILT_IN_UBSAN_HANDLE_SHIFT_OUT_OF_BOUNDS
>           : BUILT_IN_UBSAN_HANDLE_SHIFT_OUT_OF_BOUNDS_ABORT;
>        tt = builtin_decl_explicit (bcode);
> @@ -229,7 +229,7 @@ ubsan_instrument_vla (location_t loc, tr
>                                      NULL_TREE);
>        data = build_fold_addr_expr_loc (loc, data);
>        enum built_in_function bcode
> -       = flag_sanitize_recover
> +       = (flag_sanitize_recover & SANITIZE_VLA)
>           ? BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE
>           : BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE_ABORT;
>        tt = builtin_decl_explicit (bcode);
>
>
>         Jakub



-- 
Alexey Samsonov, Mountain View, CA