-fsanitize=function: support C

[MaskRay]

Summary

With D148785, -fsanitize=function no longer uses C++ RTTI objects and therefore
can support C. The rationale for reporting errors is C11 6.5.2.2p9:

If the function is defined with a type that is not compatible with the type (of the expression) pointed to by the expression that denotes the called function, the behavior is undefined.

The mangled types approach we use does not exactly match the C type
compatibility (see f(callee1) below).
This is probably fine as the rules are unlikely leveraged in practice. In
addition, the call is warned by -Wincompatible-function-pointer-types-strict.

void callee0(int (*a)[]) {}
void callee1(int (*a)[1]) {}
void f(void (*fp)(int (*)[])) { fp(0); }
int main() {
  int a[1];
  f(callee0);
  f(callee1); // compatible but flagged by -fsanitize=function, -fsanitize=kcfi, and -Wincompatible-function-pointer-types-strict
}

Skip indirect call sites of a function type without a prototype to avoid deal
with C11 6.5.2.2p6. -fsanitize=kcfi skips such calls as well.

[MaskRay]

This patch is also available at https://github.com/MaskRay/llvm-project/tree/function-c , which may make inspecting stacked patches easier.

[peter.smith]

I agree with the reasoning. Can you update the documentation in clang/docs/UndefinedBehaviorSanitizer.rst to include C and state the known limitation. After D148573 it says:

-  ``-fsanitize=function``: Indirect call of a function through a
     function pointer of the wrong type (C++ only).

[MaskRay]

rebase the final patch in the series

[MaskRay]

rebase.

unsupport powerpc64-* as ppc64be ELFv1 may have an existing
error: Cannot represent a difference across sections issue
unrelated to the -fsanitize=function.

[glandium]

This breaks sqlite:

/builds/worker/checkouts/gecko/third_party/sqlite3/src/sqlite3.c:137791:5: runtime error: call to function sqlite3DeleteTable through pointer to incorrect function type 'void (*)(struct sqlite3 *, void *)'
/builds/worker/checkouts/gecko/third_party/sqlite3/src/sqlite3.c:118360: note: sqlite3DeleteTable defined here

The function is declared as void sqlite3DeleteTable(sqlite3 *db, Table *pTable) so technically, it's not wrong, but is it really a problem that needs to be caught? (when the difference is between type* and void*)

This also breaks NSS:

/builds/worker/checkouts/gecko/security/nss/lib/freebl/loader.c:95:12: runtime error: call to function BL_Init through pointer to incorrect function type 'enum _SECStatus (*)(void)'
/builds/worker/checkouts/gecko/security/nss/lib/freebl/blinit.c:557: note: BL_Init defined here

Pointer type is:

SECStatus (*p_BL_Init)(void);

and BL_Init is declared as:

SECStatus BL_Init()

I guess it's tripping on the missing void... should that be an error?

[MaskRay]

In D148827#4379560, @glandium wrote:

This breaks sqlite:

/builds/worker/checkouts/gecko/third_party/sqlite3/src/sqlite3.c:137791:5: runtime error: call to function sqlite3DeleteTable through pointer to incorrect function type 'void (*)(struct sqlite3 *, void *)'
/builds/worker/checkouts/gecko/third_party/sqlite3/src/sqlite3.c:118360: note: sqlite3DeleteTable defined here

The function is declared as void sqlite3DeleteTable(sqlite3 *db, Table *pTable) so technically, it's not wrong, but is it really a problem that needs to be caught? (when the difference is between type* and void*)

This also breaks NSS:

/builds/worker/checkouts/gecko/security/nss/lib/freebl/loader.c:95:12: runtime error: call to function BL_Init through pointer to incorrect function type 'enum _SECStatus (*)(void)'
/builds/worker/checkouts/gecko/security/nss/lib/freebl/blinit.c:557: note: BL_Init defined here

Pointer type is:

SECStatus (*p_BL_Init)(void);

and BL_Init is declared as:

SECStatus BL_Init()

I guess it's tripping on the missing void... should that be an error?

These cases are UB and should be caught. It's not an excuse that they use C.

[glandium]

In D148827#4379764, @MaskRay wrote:

These cases are UB and should be caught. It's not an excuse that they use C.

Are they really though?

struct A {
  int a;
};

int foo(struct A *a) { return 42; }

int bar(void *a) { return foo(a); }

int main(void) {
  struct A a;
  bar(&a);
  int (*qux)(void *) = (int (*)(void *))foo;
  qux(&a); // If this is UB, why isn't the call to foo from bar?
  return 0;
}

Likewise for int foo() and int bar(void)/int (*qux)(void).
Likewise for struct A* foo(void) and void *bar(void)/void *(*qux)(void) (surprisingly, clang doesn't emit this error for struct A* foo() and void* bar()/void*(*qux)())

[MaskRay]

In D148827#4422498, @glandium wrote:

In D148827#4379764, @MaskRay wrote:

These cases are UB and should be caught. It's not an excuse that they use C.

Are they really though?

They are.

struct A {
  int a;
};

int foo(struct A *a) { return 42; }

int bar(void *a) { return foo(a); }

int main(void) {
  struct A a;
  bar(&a);
  int (*qux)(void *) = (int (*)(void *))foo;
  qux(&a); // If this is UB, why isn't the call to foo from bar?
  return 0;
}

Likewise for int foo() and int bar(void)/int (*qux)(void).
Likewise for struct A* foo(void) and void *bar(void)/void *(*qux)(void) (surprisingly, clang doesn't emit this error for struct A* foo() and void* bar()/void*(*qux)())

C11 6.5.2.2p9 (as mentioned in the patch summary)

If the function is defined with a type that is not compatible with the type (of the expression) pointed to by the expression that denotes the called function, the behavior is undefined.

void * and struct A* are not compatible per 6.7..6.1p2

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

For old-style (parameter-less) definitions (warned by -Wstrict-prototypes unless -std=c2x), we have the following behaviors:

int foo() { return 42; } // old-style (parameter-less) definition, warned by -Wstrict-prototypes unless -std=c2x
int bar(int a) { return 42; }

int main(void) {
  //int (*qux)(int) = (int (*)(int))foo;
  //qux(42);  // rejected by -fsanitize=function
  int (*qux)() = (int (*)())foo;
  qux(); // not rejected by -fsanitize=function
  return 0;
}

If you enable -fsanitize=undefined for some C proijects with the UB, you can consider suppressing them with ubsan_ignorelist.txt or enabling -fno-sanitize=function for the C files.

[glandium]

OTOH. 6.2.7.3:

A composite type can be constructed from two types that are compatible; it is a type that is compatible with both of the two types (...)

6.2.7.5:

EXAMPLE Given the following two file scope declarations:
int f(int (*)(), double (*)[3]);
int f(int (*)(char *), double (*)[]);
The resulting composite type for the function is:
int f(int (*)(char *), double (*)[3]);

This suggests that int(*)() and int(*)(char*) are compatible.

6.2.7.2:

All declarations that refer to the same object or function shall have compatible type; otherwise, the behavior is undefined.

6.3.2.3.1:

A pointer to void may be converted to or from a pointer to any object type. A pointer to any object type may be converted to a pointer to void and back again; the result shall compare equal to the original pointer.

These are either contradicting, making 6.3.2.3.1 UB, or void* is compatible with other pointer types.

[MaskRay]

In D148827#4422709, @glandium wrote:

OTOH. 6.2.7.3:

A composite type can be constructed from two types that are compatible; it is a type that is compatible with both of the two types (...)

6.2.7.5:

EXAMPLE Given the following two file scope declarations:
int f(int (*)(), double (*)[3]);
int f(int (*)(char *), double (*)[]);
The resulting composite type for the function is:
int f(int (*)(char *), double (*)[3]);

This suggests that int(*)() and int(*)(char*) are compatible.

Yes, I mentioned this in my previous comment. This fragment is overly strict but I think it should be fine in practice. This mismatching is uncommon and old-style (parameter-less) definitions will be unsupported in C2x anyway.
The other direction is worked around by this patch (see !isa<FunctionNoProtoType>(PointeeType)).

int foo() { return 42; } // old-style (parameter-less) definition, warned by -Wstrict-prototypes unless -std=c2x

int main(void) {
  //int (*qux)(int) = (int (*)(int))foo;
  //qux(42);  // rejected by -fsanitize=function
...

6.2.7.2:

All declarations that refer to the same object or function shall have compatible type; otherwise, the behavior is undefined.

6.3.2.3.1:

A pointer to void may be converted to or from a pointer to any object type. A pointer to any object type may be converted to a pointer to void and back again; the result shall compare equal to the original pointer.

These are either contradicting, making 6.3.2.3.1 UB, or void* is compatible with other pointer types.

6.3.2.3.1 is about a pointer casting rule, which is different from type compatibility.
In C++, there is no C type compatibility rule, but we can still cast pointers.

To elaborate my previous comment about working around C projects, you can use the following to suppress instrumentation for all .c files (but not .cc or .cpp)

# -fsanitize=undefined -fsanitize-ignorelist=a.ignorelist
% cat a.ignorelist
[function]
src:*\.c

[glandium]

In D148827#4422794, @MaskRay wrote:

6.3.2.3.1 is about a pointer casting rule, which is different from type compatibility.

Pointer _conversion_ rule. Specifically, these are implicit conversions that require no casting. That doesn't invalidate that the rule from 6.2.7.2 still applies, and I don't see how the conversions can be non-UB without the types being compatible.

[uabelho]

Hi,

A question: it seems like this messes with alignment on functions?
So with input program al.c:

__attribute__((aligned(64)))
void alignedfn(void) {
  __asm("nop");
}
void alignedfn2(void) {
  __asm("nop");
}
int main(void){}

if we compile with -fsanitize=function we get:

-> clang al.c -o al.o -c -fsanitize=function
-> nm al.o
0000000000000008 T alignedfn
0000000000000018 T alignedfn2
0000000000000028 T main

So alignedfn and alignedfn2 doesn't seem to be aligned as we said.
Without -fsanitize=function:

-> clang al.c -o al.o -c 
-> nm al.o
0000000000000000 T alignedfn
0000000000000010 T alignedfn2
0000000000000020 T main

I guess the data put before the functions get aligned but not the functions themselves. Should I write a ticket about this?

[MaskRay]

In D148827#4424237, @uabelho wrote:

Hi,

A question: it seems like this messes with alignment on functions?
So with input program al.c:

__attribute__((aligned(64)))
void alignedfn(void) {
  __asm("nop");
}
void alignedfn2(void) {
  __asm("nop");
}
int main(void){}

if we compile with -fsanitize=function we get:

-> clang al.c -o al.o -c -fsanitize=function
-> nm al.o
0000000000000008 T alignedfn
0000000000000018 T alignedfn2
0000000000000028 T main

So alignedfn and alignedfn2 doesn't seem to be aligned as we said.
Without -fsanitize=function:

-> clang al.c -o al.o -c 
-> nm al.o
0000000000000000 T alignedfn
0000000000000010 T alignedfn2
0000000000000020 T main

I guess the data put before the functions get aligned but not the functions themselves. Should I write a ticket about this?

When __attribute__((aligned(...))) is used with an instrumentation that places extra bytes before the function label, I think it is unclear what the desired behavior should be.
This applies to many instructions including -fsanitize=kcfi, -fsanitize=function, and -fpatchable-function-entry=N,M (M>0).
It's unclear whether this attribute applies to the first byte of the function or the first intended instruction.
For example, with -fpatchable-function-entry=2,2, we can make st_value(foo)%64=2.

gcc -c -fpatchable-function-entry=2,2 -xc =(printf '__attribute__((aligned(64))) void foo() {}') -o a.o
clang -c -fpatchable-function-entry=2,2 -xc =(printf '__attribute__((aligned(64))) void foo() {}') -o a.o

I acknowledge that https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html says "The aligned attribute specifies a minimum alignment for the first instruction of the function, measured in bytes.", but I think the GCC documentation doesn't consider these instructions.
I consider Clang's behavior working as intended.

(Other angles: with PPC64 ELFv2, the local entry, the target destination of a function call, is not aligned by __attribute__((aligned(...))). __attribute__((aligned(...))) is usually for performance reasons and has different tradeoff with -fsanitize=function.)

clang/docs/ControlFlowIntegrity.rst

 
 This tool is similar to ``-fsanitize=function`` in that both tools check
 the types of function calls. However, the two tools occupy different points
 on the design space; ``-fsanitize=function`` is a developer tool designed
 to find bugs in local development builds, whereas ``-fsanitize=cfi-icall``
 is a security hardening mechanism designed to be deployed in release builds.
 
 ``-fsanitize=function`` has a higher space and time overhead due to a more
-complex type check at indirect call sites, as well as a need for run-time
-type information (RTTI), which may make it unsuitable for deployment. Because
-of the need for RTTI, ``-fsanitize=function`` can only be used with C++
-programs, whereas ``-fsanitize=cfi-icall`` can protect both C and C++ programs.
+complex type check at indirect call sites, which may make it unsuitable for
+deployment.
 
 On the other hand, ``-fsanitize=function`` conforms more closely with the C++
 standard and user expectations around interaction with shared libraries;
 the identity of function pointers is maintained, and calls across shared
 library boundaries are no different from calls within a single program or
 shared library.
 
 .. _kcfi:

clang/docs/UndefinedBehaviorSanitizer.rst

-  ``-fsanitize=float-cast-overflow``: Conversion to, from, or
      destination. Because the range of representable values for all
      floating-point types supported by Clang is [-inf, +inf], the only
      cases detected are conversions from floating point to integer types.
   -  ``-fsanitize=float-divide-by-zero``: Floating point division by
      zero. This is undefined per the C and C++ standards, but is defined
      by Clang (and by ISO/IEC/IEEE 60559 / IEEE 754) as producing either an
      infinity or NaN value, so is not included in ``-fsanitize=undefined``.
   -  ``-fsanitize=function``: Indirect call of a function through a
-     function pointer of the wrong type (C++ only).
+     function pointer of the wrong type.
   -  ``-fsanitize=implicit-unsigned-integer-truncation``,
      ``-fsanitize=implicit-signed-integer-truncation``: Implicit conversion from
      integer of larger bit width to smaller bit width, if that results in data
      loss. That is, if the demoted value, after casting back to the original
      width, is not equal to the original value before the downcast.
      The ``-fsanitize=implicit-unsigned-integer-truncation`` handles conversions
      between two ``unsigned`` types, while
      ``-fsanitize=implicit-signed-integer-truncation`` handles the rest of the

clang/lib/CodeGen/CGExpr.cpp

const Decl *TargetDecl =
       OrigCallee.getAbstractInfo().getCalleeDecl().getDecl();
 
   CalleeType = getContext().getCanonicalType(CalleeType);
 
   auto PointeeType = cast<PointerType>(CalleeType)->getPointeeType();
 
   CGCallee Callee = OrigCallee;
 
-  if (getLangOpts().CPlusPlus && SanOpts.has(SanitizerKind::Function) &&
-      (!TargetDecl || !isa<FunctionDecl>(TargetDecl))) {
+  if (SanOpts.has(SanitizerKind::Function) &&
+      (!TargetDecl || !isa<FunctionDecl>(TargetDecl)) &&
+      !isa<FunctionNoProtoType>(PointeeType)) {
     if (llvm::Constant *PrefixSig =
             CGM.getTargetCodeGenInfo().getUBSanFunctionSignature(CGM)) {
       SanitizerScope SanScope(this);
       auto *TypeHash = getUBSanFunctionTypeHash(PointeeType);
 
       llvm::Type *PrefixSigType = PrefixSig->getType();
       llvm::StructType *PrefixStructTy = llvm::StructType::get(
           CGM.getLLVMContext(), {PrefixSigType, Int32Ty}, /*isPacked=*/true);

clang/lib/CodeGen/CodeGenFunction.cpp

return CGM.getCodeGenOpts().XRayInstrumentFunctions &&
           CGM.getCodeGenOpts().XRayInstrumentationBundle.Mask ==
               XRayInstrKind::Typed);
 }
 
 llvm::ConstantInt *
 CodeGenFunction::getUBSanFunctionTypeHash(QualType Ty) const {
   // Remove any (C++17) exception specifications, to allow calling e.g. a
   // noexcept function through a non-noexcept pointer.
-  auto ProtoTy = getContext().getFunctionTypeWithExceptionSpec(Ty, EST_None);
+  if (!isa<FunctionNoProtoType>(Ty))
+    Ty = getContext().getFunctionTypeWithExceptionSpec(Ty, EST_None);
   std::string Mangled;
   llvm::raw_string_ostream Out(Mangled);
-  CGM.getCXXABI().getMangleContext().mangleTypeName(ProtoTy, Out, false);
+  CGM.getCXXABI().getMangleContext().mangleTypeName(Ty, Out, false);
   return llvm::ConstantInt::get(CGM.Int32Ty,
                                 static_cast<uint32_t>(llvm::xxHash64(Mangled)));
 }
 
 void CodeGenFunction::EmitKernelMetadata(const FunctionDecl *FD,
                                          llvm::Function *Fn) {
   if (!FD->hasAttr<OpenCLKernelAttr>() && !FD->hasAttr<CUDAGlobalAttr>())
     return;
#undef SANITIZER
   if (FD && (getLangOpts().OpenCL ||
              (getLangOpts().HIP && getLangOpts().CUDAIsDevice))) {
     // Add metadata for a kernel function.
     EmitKernelMetadata(FD, Fn);
   }
 
   // If we are checking function types, emit a function type signature as
   // prologue data.
-  if (FD && getLangOpts().CPlusPlus && SanOpts.has(SanitizerKind::Function)) {
+  if (FD && SanOpts.has(SanitizerKind::Function)) {
     if (llvm::Constant *PrologueSig = getPrologueSignature(CGM, FD)) {
       llvm::LLVMContext &Ctx = Fn->getContext();
       llvm::MDBuilder MDB(Ctx);
       Fn->setMetadata(
           llvm::LLVMContext::MD_func_sanitize,
           MDB.createRTTIPointerPrologue(
               PrologueSig, getUBSanFunctionTypeHash(FD->getType())));
     }

clang/test/CodeGen/ubsan-function.c

+// RUN: %clang_cc1 -emit-llvm -triple x86_64 -std=c17 -fsanitize=function %s -o - | FileCheck %s
+
+// CHECK-LABEL: define{{.*}} @call_no_prototype(
+// CHECK-NOT:     __ubsan_handle_function_type_mismatch
+void call_no_prototype(void (*f)()) { f(); }
+
+// CHECK-LABEL: define{{.*}} @call_prototype(
+// CHECK:         __ubsan_handle_function_type_mismatch
+void call_prototype(void (*f)(void)) { f(); }

compiler-rt/test/ubsan/TestCases/TypeCheck/Function/c.c

+// RUN: %clang -g -fsanitize=function %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=CHECK --implicit-check-not='runtime error:'
+
+void f(void (*fp)(int (*)[])) { fp(0); }
+
+void callee0(int (*a)[]) {}
+void callee1(int (*a)[1]) {}
+
+int main() {
+  int a[1];
+  f(callee0);
+  // CHECK: runtime error: call to function callee1 through pointer to incorrect function type 'void (*)(int (*)[])'
+  f(callee1); // compatible type in C, but flagged
+}

compiler-rt/test/ubsan/TestCases/TypeCheck/Function/function.cpp

-// Work around "Cannot represent a difference across sections"
-// UNSUPPORTED: target=powerpc64-{{.*}}
-
 // RUN: %clangxx -DDETERMINE_UNIQUE %s -o %t-unique
 // RUN: %clangxx -std=c++17 -fsanitize=function %s -O3 -g -DSHARED_LIB -fPIC -shared -o %t-so.so
 // RUN: %clangxx -std=c++17 -fsanitize=function %s -O3 -g -o %t %t-so.so
 // RUN: %run %t 2>&1 | FileCheck %s --check-prefix=CHECK $(%run %t-unique UNIQUE)
 // Verify that we can disable symbolization if needed:
 // RUN: %env_ubsan_opts=symbolize=0 %run %t 2>&1 | FileCheck %s --check-prefix=NOSYM $(%run %t-unique NOSYM-UNIQUE)
 
 #ifdef DETERMINE_UNIQUE

compiler-rt/test/ubsan/TestCases/TypeCheck/Function/lit.local.cfg.py

 if config.host_os not in ['Darwin', 'FreeBSD', 'Linux', 'NetBSD']:
   config.unsupported = True
+# Work around "Cannot represent a difference across sections"
+if config.target_arch == 'powerpc64':
+  config.unsupported = True
 # Work around "library ... not found: needed by main executable" in qemu.
 if config.android and config.target_arch not in ['x86', 'x86_64']:
   config.unsupported = True