Re: [PATCH RFA] ubsan: do return check with -fsanitize=unreachable

[Jakub Jelinek <jakub@redhat.com>]

On Fri, Jun 17, 2022 at 05:20:02PM -0400, Jason Merrill wrote:
> Related to PR104642, the current situation where we get less return checking
> with just -fsanitize=unreachable than no sanitize flags seems undesirable; I
> propose that we do return checking when -fsanitize=unreachable.

__builtin_unreachable itself (unless turned into trap or
__ubsan_handle_builtin_unreachable) is not any kind of return checking, it
is just an optimization.

> Looks like clang just traps on missing return if not -fsanitize=return, but
> the approach in this patch seems more helpful to me if we're already
> sanitizing other should-be-unreachable code.
> 
> I'm assuming that the difference in treatment of SANITIZE_UNREACHABLE and
> SANITIZE_RETURN with regard to loop optimization is deliberate.

return and unreachable are separate sanitizers and such silent one way
implication can have quite unexpected consequences, especially with
-fsanitize-trap=.
Say with -fsanitize=unreachable -fsanitize-trap=unreachable, both current
trunk and clang will link without -lubsan, because the only enabled UBSan
sanitizers use __builtin_trap () which doesn't need library.
With -fsanitize=unreachable silently meaning -fsanitize=unreachable,return
the above would link in -lubsan, because while SANITIZE_UNREACHABLE uses
__builtin_trap, SANITIZE_RETURN doesn't.
Similarly, one has no_sanitize attribute, one could in certain function
__attribute__((no_sanitize ("unreachable"))) and because on the command
line using -fsanitize=unreachable assume other sanitizers aren't enabled,
but the silent addition of return sanitizer would break that.

> --- a/gcc/cp/cp-gimplify.cc
> +++ b/gcc/cp/cp-gimplify.cc
> @@ -1806,18 +1806,6 @@ cp_maybe_instrument_return (tree fndecl)
>        || !targetm.warn_func_return (fndecl))
>      return;
>  
> -  if (!sanitize_flags_p (SANITIZE_RETURN, fndecl)
> -      /* Don't add __builtin_unreachable () if not optimizing, it will not
> -	 improve any optimizations in that case, just break UB code.
> -	 Don't add it if -fsanitize=unreachable -fno-sanitize=return either,
> -	 UBSan covers this with ubsan_instrument_return above where sufficient
> -	 information is provided, while the __builtin_unreachable () below
> -	 if return sanitization is disabled will just result in hard to
> -	 understand runtime error without location.  */
> -      && (!optimize
> -	  || sanitize_flags_p (SANITIZE_UNREACHABLE, fndecl)))
> -    return;
> -
>    tree t = DECL_SAVED_TREE (fndecl);
>    while (t)
>      {

I think the above is correct, if -fsanitize=return, we want to fall through
and use __ubsan_handle_missing_return (or __builtin_trap if
-fsanitize-trap=return).
Otherwise, for -O0, __builtin_unreachable most likely doesn't offer any
important optimization benefits and just makes debugging bad code harder.
Similarly for -fsanitize=unreachable, the __builtin_unreachable there would
be an optimization which we shouldn't turn into
__ubsan_handle_builtin_unreachable / __builtin_trap.

Now, -funreachable-traps can of course change the condition a little bit,
and so can implementation of builtin_decl_unreachable and stopping of
folding of __builtin_unreachable to __builtin_trap if -fsanitize=unreachable
-fsanitize-trap=unreachable.

The -fsanitize=return case remains the same no matter what.

Otherwise, if -funreachable-traps, we are emitting __builtin_trap rather
than __builtin_unreachable, so it is perfectly fine to fall through
regardless of !optimize or SANITIZE_UNREACHABLE being on, it isn't an
optimization in that case, but checking.

Otherwise, if !optimize, we should return, __builtin_unreachable in there
wouldn't bring many advantages and just punish users of bad code.

Otherwise, if builtin_decl_unreachable is implemented and we never fold
__builtin_unreachable to __builtin_trap, for SANITIZE_UNREACHABLE
enabled and (flag_sanitize_trap & SANITIZE_UNREACHABLE) != 0 we could
emit __builtin_unreachable (but in that case directly, not through
builtin_decl_unreachable).

Otherwise, if SANITIZE_UNREACHABLE is on and
(flag_sanitize_trap & SANITIZE_UNREACHABLE) == 0, I assume we'll still
want to fold __builtin_unreachable to __ubsan_handle_builtin_unreachable
during sanopt etc., we can live without the optimization and not instrument.

Otherwise emit __builtin_unreachable (directly).

	Jakub