This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Sema] Add -Wcast-function-type-strict
ClosedPublic

Authored by samitolvanen on Sep 28 2022, 2:32 PM.

Details

Summary

Clang supports indirect call Control-Flow Integrity (CFI) sanitizers
(e.g. -fsanitize=cfi-icall), which enforce an exact type match between
a function pointer and the target function. Unfortunately, Clang
doesn't provide diagnostics that would help developers avoid function
type casts that lead to runtime CFI failures. -Wcast-function-type,
while helpful, only warns about ABI incompatibility, which isn't
sufficient with CFI.

Add -Wcast-function-type-strict, which checks for a strict type
compatibility in function type casts and helps warn about casts that
can potentially lead to CFI failures.

Diff Detail

Event Timeline

samitolvanen created this revision.Sep 28 2022, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 2:32 PM
samitolvanen requested review of this revision.Sep 28 2022, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 2:32 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Any thoughts about adding a stricter version of -Wcast-function-type to make it easier to catch potential CFI issues? I also considered also gating this behind -fsanitize=cfi-icall/kcfi, but having a separate warning flag would be more consistent.

nickdesaulniers requested changes to this revision.Sep 28 2022, 2:49 PM

SGTM; please make a note of this new diagnostic flag in clang/docs/ReleaseNotes.rst under Improvements to Clang's diagnostics.

clang/include/clang/Basic/DiagnosticSemaKinds.td
8686

I don't think we need a new group for a warning that only contains one diagnostic kind?

This revision now requires changes to proceed.Sep 28 2022, 2:49 PM

Added a release note.

clang/include/clang/Basic/DiagnosticSemaKinds.td
8686

I don't think we need a new group for a warning that only contains one diagnostic kind?

I might have misunderstood something, but we seem to have plenty of groups with only one diagnostic kind. Is there a way to add a new warning flag without adding a diagnostic group? Most users of -Wcast-function-type wouldn't want to enable the stricter version, so I would prefer to keep this separate.

I did notice that some warnings don't define the group in DiagnosticGroups.td, but specify it directly here. For example, InGroup<DiagGroup<"argument-outside-range">>. I'm not sure if there are any benefits in doing so.

aaron.ballman added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8686

Typically we only define a group in DiagnosticGroups.td when the group is going to be used by more than one diagnostic, otherwise we prefer using InGroup<DiagGroup<"whatever">> to form one-off diagnostic groups.

However, in this case, I am wondering if we want to add CastFunctionTypeStrict to be a subgroup of CastFunctionType so that users who enable -Wcast-function-type get the stricter checking by default, but still have a way to disable the stricter checking if it's too noisy for them?

clang/lib/Sema/SemaCast.cpp
1095–1097

Coding style nit.

1184

Same elsewhere (the type isn't spelled out in the initialization, so we prefer using an explicit type).

clang/test/Sema/warn-cast-function-type-strict.c
1

Do we need to use the triple here, or can we make this test target agnostic?

clang/test/SemaCXX/warn-cast-function-type-strict.cpp
1

Same question about triples here.

32

You should use // style comments here, this is a C++ test file anyway.

kees added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8686

However, in this case, I am wondering if we want to add CastFunctionTypeStrict to be a subgroup of CastFunctionType so that users who enable -Wcast-function-type get the stricter checking by default, but still have a way to disable the stricter checking if it's too noisy for them?

I'd be for that. It'll be very noisy for the Linux kernel, but they are all instances we need to fix.

cc @nathanchance

nathanchance added inline comments.Sep 29 2022, 9:48 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8686

However, in this case, I am wondering if we want to add CastFunctionTypeStrict to be a subgroup of CastFunctionType so that users who enable -Wcast-function-type get the stricter checking by default, but still have a way to disable the stricter checking if it's too noisy for them?

I'd be for that. It'll be very noisy for the Linux kernel, but they are all instances we need to fix.

cc @nathanchance

Right, it would be quite noisy but we have already built an escape hatch for this type of scenario. We can just do what we have done for other warnings and disable it for "normal" builds to avoid disrupting the configurations with -Werror enabled (especially since we are not the only ones testing with tip of tree LLVM) then turn it on for W=1 so that the build bots will catch new instances of the warnings while we clean up the other ones. I think such a diff would just look like:

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 6ae482158bc4..52bd7df84fd6 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -64,6 +64,7 @@ KBUILD_CFLAGS += -Wno-sign-compare
 KBUILD_CFLAGS += $(call cc-disable-warning, pointer-to-enum-cast)
 KBUILD_CFLAGS += -Wno-tautological-constant-out-of-range-compare
 KBUILD_CFLAGS += $(call cc-disable-warning, unaligned-access)
+KBUILD_CFLAGS += $(call cc-disable-warning, cast-function-type-strict)
 endif

 endif

then that can just be reverted when we have all the instances cleaned up. It would be nice to get a game plan together for tackling all these because they appear to be quite numerous.

aaron.ballman added inline comments.Sep 29 2022, 11:15 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8686

Do we think this will be onerously chatty for C code bases in general? My intuition is that it will be reasonable -- folks who have enabled this warning want to know when they're type casting function pointers in odd ways. Looking at some test cases, I actually think this will behave more like users expect. e.g., https://godbolt.org/z/cWGecK1KG

clang/test/Sema/warn-cast-function-type-strict.c
30

Some other test cases I think we should try out:

typedef int (f8)(int *);
typedef int (f9)(const int);
typedef int (f10)(int);

int foo(int array[static 12]);
int bar(int i);
const int baz(int i);

f8 *h = (f8 *)foo; // Should be okay, types are the same after adjustment
f9 *i = (f9 *)bar; // Should be okay, types are the same after adjustment
f10 *j = (f10 *)baz; // Should be okay, types are the same after adjustment
samitolvanen marked 8 inline comments as done.

Moved CastFunctionTypeStrict to a subgroup of CastFunctionType, addressed other feedback, and updated tests.

clang/include/clang/Basic/DiagnosticSemaKinds.td
8686

Do we think this will be onerously chatty for C code bases in general? My intuition is that it will be reasonable -- folks who have enabled this warning want to know when they're type casting function pointers in odd ways.

This produces 1500+ new warnings in the kernel, but yes, I believe these are warnings we expected to see already with -Wcast-function-type. Nathan's suggestion to disable the strict warning without W=1 sounds reasonable to me.

clang/test/Sema/warn-cast-function-type-strict.c
1

I don't think it's needed. This was copied from the -Wcast-function-type test.

30

The first two seem to be OK, the last one does produce a warning here:

cast from 'const int (*)(int)' to 'f10 *' (aka 'int (*)(int)') converts to incompatible function type
aaron.ballman added inline comments.Sep 30 2022, 7:18 AM
clang/docs/ReleaseNotes.rst
220–222
clang/test/Sema/warn-cast-function-type-strict.c
30

Oh yeah, that's right, the C standard is pretty weird here. The return type is required to be compatible (aka same type in this case) per C2x 6.7.6.3p14, and int and const int are not compatible types (C2x 6.7.3p11). However, the qualifier on the return type is useless because it's stripped when the function is called (C2x 6.5.2.2p5, 6.8.6.4p3, 6.5.16p3, 6.3.2.1p2).

Compilers are wildly inconsistent about this: https://godbolt.org/z/hc6ordGeM

clang/test/SemaCXX/warn-cast-function-type-strict.cpp
1

You should remove the -x c++ from the RUN line still.

clang/test/SemaCXX/warn-cast-function-type.cpp
1 ↗(On Diff #464067)
samitolvanen marked 3 inline comments as done.

Addressed feedback.

aaron.ballman accepted this revision.Sep 30 2022, 10:58 AM

LGTM assuming precommit CI doesn't find any surprises, thank you for this!

nickdesaulniers accepted this revision.Sep 30 2022, 11:00 AM

Please get the patch disabling this warning for the kernel in flight before landing this. It's going to make a lot of CI red due to kernel builds enabling -Werror.

This revision is now accepted and ready to land.Sep 30 2022, 11:00 AM

Please get the patch disabling this warning for the kernel in flight before landing this.

Agreed. @nathanchance do you want to send the patch above to LKML?

Please get the patch disabling this warning for the kernel in flight before landing this.

Agreed. @nathanchance do you want to send the patch above to LKML?

I can send it on Monday, as I am offline today (or at least not at a computer lol). If you want to move on it quicker, feel free to draft a commit message and slap my Suggested-by on it.

If you want to move on it quicker, feel free to draft a commit message and slap my Suggested-by on it.

https://lore.kernel.org/all/20220930203310.4010564-1-samitolvanen@google.com/

Please consider waiting until the warning flag has started propagating to branches of stable, at least to whatever branch -Werror is first enabled in.

Please consider waiting until the warning flag has started propagating to branches of stable, at least to whatever branch -Werror is first enabled in.

Yes, I'm planning to wait until the kernel patch lands in stable branches. Greg picked up the patch this morning, so it should be in the next batch of stable releases.

The patch to disable this warning in Linux without W=1 is now in all supported stable kernels (https://lwn.net/Articles/912498/), so I'll commit this later today.

This revision was landed with ongoing or failed builds.Oct 26 2022, 1:50 PM
This revision was automatically updated to reflect the committed changes.

Is this intended to warn on code that casts a function taking a pointer to some non-void type to a function that takes a void*?

void set(void (*g)(int*)) {
  f = (void(*)(void*)) g;
}

gives me

warning: cast from 'void (*)(int *)' to 'void (*)(void *)' converts to incompatible function type [-Wcast-function-type-strict]

I didn't see this mentioned in the diff description, comments, or test. Is the behavior intentional? Are these types actually incompatible?

Is this intended to warn on code that casts a function taking a pointer to some non-void type to a function that takes a void*?

Yes, this is intended to warn if the function types don't match.

I didn't see this mentioned in the diff description, comments, or test. Is the behavior intentional? Are these types actually incompatible?

As far as the language is concerned, the types are not incompatible, but calling the function pointer would trip -fsanitize=cfi checking, which this warning is meant to catch. If your program is not using CFI, you may want to consider disabling the -strict warning.

Thanks for the reply!

FWIW, this warning is triggered by code which uses glib, which contains a number of function(some_type*) to function(void*) casts, for example here: https://github.com/GNOME/glib/blob/339aaa3719757614af61f427b66a46893e6dc760/glib/glib-autocleanups.h#L47, which expands to function pointer casts here: https://github.com/GNOME/glib/blob/339aaa3719757614af61f427b66a46893e6dc760/glib/gmacros.h#L1360

I imagine glib is reasonably commonly used, so this might cause some build failures (we're seeing some in ChromiumOS). Not sure if that's enough to affect the choice of having this on or off by default when -Wcast-function-type is in effect, but I thought I would provide the data point.

Is this intended to warn on code that casts a function taking a pointer to some non-void type to a function that takes a void*?

Yes, this is intended to warn if the function types don't match.

I didn't see this mentioned in the diff description, comments, or test. Is the behavior intentional? Are these types actually incompatible?

As far as the language is concerned, the types are not incompatible, <snip>

As far as C and C++ are concerned, those types *are* incompatible.

C2x 6.7.6.3p14: For two function types to be compatible, both shall specify compatible return types. Moreover, the parameter type lists shall agree in the number of parameters and in use of the final ellipsis; corresponding parameters shall have compatible types. In the determination of type compatibility and of a composite type, each parameter declared with function or array type is taken as having the adjusted type and each parameter declared with qualified type is taken as having the unqualified version of its declared type.

6.7.6.1p2: For two pointer types to be compatible, both shall be identically qualified and both shall be pointers to compatible types.

6.2.7p1: Two types are compatible types if they are the same. Additional rules for determining whether two types are compatible are described in <snip>

void * and int * are not compatible types, so the function parameter lists are not compatible, so the function types are not compatible.

(Similar is in C++, though the specification mechanics are different.)