compiler_wrapper: keep FORTIFY if sanitizer is trivial

For sanitizers like `return`, `builtin`, etc., we have no reason to also
drop FORTIFY checks.

BUG=b:231357370
TEST=`go test`

Change-Id: I1e349a4f6743e549b7bc0899a307b10683bb42e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/3651188
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Commit-Queue: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>

2 files changed, 90 insertions, 27 deletions

diff --git a/compiler_wrapper/sanitizer_flags.go b/compiler_wrapper/sanitizer_flags.go
index b9eb0558..163b0f4f 100644
--- a/compiler_wrapper/sanitizer_flags.go
+++ b/compiler_wrapper/sanitizer_flags.go
@@ -8,38 +8,79 @@ import (
 	"strings"
 )
 
+// Returns whether the flag turns on 'invasive' sanitizers. These are sanitizers incompatible with
+// things like FORTIFY, since they require meaningful runtime support, intercept libc calls, etc.
+func isInvasiveSanitizerFlag(flag string) bool {
+	// There are a few valid spellings here:
+	//   -fsanitize=${sanitizer_list}, which enables the given sanitizers
+	//   -fsanitize-trap=${sanitizer_list}, which specifies sanitizer behavior _if_ these
+	//     sanitizers are already enabled.
+	//   -fsanitize-recover=${sanitizer_list}, which also specifies sanitizer behavior _if_
+	//     these sanitizers are already enabled.
+	//   -fsanitize-ignorelist=/path/to/file, which designates a config file for sanitizers.
+	//
+	// All we care about is the first one, since that's what actually enables sanitizers. Clang
+	// does not accept a `-fsanitize ${sanitizer_list}` spelling of this flag.
+	fsanitize := "-fsanitize="
+	if !strings.HasPrefix(flag, fsanitize) {
+		return false
+	}
+
+	sanitizers := flag[len(fsanitize):]
+	if sanitizers == "" {
+		return false
+	}
+
+	for _, sanitizer := range strings.Split(sanitizers, ",") {
+		// Keep an allowlist of sanitizers known to not cause issues.
+		switch sanitizer {
+		case "alignment", "array-bounds", "bool", "bounds", "builtin", "enum",
+			"float-cast-overflow", "integer-divide-by-zero", "local-bounds",
+			"nullability", "nullability-arg", "nullability-assign",
+			"nullability-return", "null", "return", "returns-nonnull-attribute",
+			"shift-base", "shift-exponent", "shift", "unreachable", "vla-bound":
+			// These sanitizers are lightweight. Ignore them.
+		default:
+			return true
+		}
+	}
+	return false
+}
+
 func processSanitizerFlags(builder *commandBuilder) {
 	hasSanitizeFlags := false
+	// TODO: This doesn't take -fno-sanitize flags into account. This doesn't seem to be an
+	// issue in practice.
 	for _, arg := range builder.args {
-		// TODO: This should probably be -fsanitize= to not match on
-		// e.g. -fsanitize-blocklist
-		if arg.fromUser {
-			if strings.HasPrefix(arg.value, "-fsanitize") {
-				hasSanitizeFlags = true
-			}
+		if arg.fromUser && isInvasiveSanitizerFlag(arg.value) {
+			hasSanitizeFlags = true
+			break
 		}
 	}
-	if hasSanitizeFlags {
-		// Flags not supported by sanitizers (ASan etc.)
-		unsupportedSanitizerFlags := map[string]bool{
-			"-D_FORTIFY_SOURCE=1": true,
-			"-D_FORTIFY_SOURCE=2": true,
-			"-Wl,--no-undefined":  true,
-			"-Wl,-z,defs":         true,
-		}
 
-		builder.transformArgs(func(arg builderArg) string {
-			// TODO: This is a bug in the old wrapper to not filter
-			// non user args for gcc. Fix this once we don't compare to the old wrapper anymore.
-			if (builder.target.compilerType != gccType || arg.fromUser) &&
-				unsupportedSanitizerFlags[arg.value] {
-				return ""
-			}
-			return arg.value
-		})
-
-		builder.filterArgPairs(func(arg1, arg2 builderArg) bool {
-			return !(arg1.value == "-Wl,-z" && arg2.value == "-Wl,defs")
-		})
+	if !hasSanitizeFlags {
+		return
+	}
+
+	// Flags not supported by sanitizers (ASan etc.)
+	unsupportedSanitizerFlags := map[string]bool{
+		"-D_FORTIFY_SOURCE=1": true,
+		"-D_FORTIFY_SOURCE=2": true,
+		"-Wl,--no-undefined":  true,
+		"-Wl,-z,defs":         true,
 	}
+
+	builder.transformArgs(func(arg builderArg) string {
+		// TODO: This is a bug in the old wrapper to not filter
+		// non user args for gcc. Fix this once we don't compare to the old wrapper anymore.
+		if (builder.target.compilerType != gccType || arg.fromUser) &&
+			unsupportedSanitizerFlags[arg.value] {
+			return ""
+		}
+		return arg.value
+	})
+
+	builder.filterArgPairs(func(arg1, arg2 builderArg) bool {
+		return !(arg1.value == "-Wl,-z" && arg2.value == "-Wl,defs")
+	})
 }
diff --git a/compiler_wrapper/sanitizer_flags_test.go b/compiler_wrapper/sanitizer_flags_test.go
index 551691f4..796961eb 100644
--- a/compiler_wrapper/sanitizer_flags_test.go
+++ b/compiler_wrapper/sanitizer_flags_test.go
@@ -8,6 +8,28 @@ import (
 	"testing"
 )
 
+func TestFortifyIsKeptIfSanitizerIsTrivial(t *testing.T) {
+	withTestContext(t, func(ctx *testContext) {
+		cmd := ctx.must(callCompiler(ctx, ctx.cfg,
+			ctx.newCommand(gccX86_64, "-fsanitize=return", "-D_FORTIFY_SOURCE=1", mainCc)))
+		if err := verifyArgCount(cmd, 1, "-D_FORTIFY_SOURCE=1"); err != nil {
+			t.Error(err)
+		}
+
+		cmd = ctx.must(callCompiler(ctx, ctx.cfg,
+			ctx.newCommand(gccX86_64, "-fsanitize=return,address", "-D_FORTIFY_SOURCE=1", mainCc)))
+		if err := verifyArgCount(cmd, 0, "-D_FORTIFY_SOURCE=1"); err != nil {
+			t.Error(err)
+		}
+
+		cmd = ctx.must(callCompiler(ctx, ctx.cfg,
+			ctx.newCommand(gccX86_64, "-fsanitize=address,return", "-D_FORTIFY_SOURCE=1", mainCc)))
+		if err := verifyArgCount(cmd, 0, "-D_FORTIFY_SOURCE=1"); err != nil {
+			t.Error(err)
+		}
+	})
+}
+
 func TestFilterUnsupportedSanitizerFlagsIfSanitizeGiven(t *testing.T) {
 	withTestContext(t, func(ctx *testContext) {
 		cmd := ctx.must(callCompiler(ctx, ctx.cfg,