Skip to content

Commit

Permalink
-fsanitize=alignment: check memcpy/memmove arguments (#67766)
Browse files Browse the repository at this point in the history 
The -fsanitize=alignment implementation follows the model that we allow
forming unaligned pointers but disallow accessing unaligned pointers.
See [RFC: Enforcing pointer type alignment in Clang](https://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html)
for detail.

memcpy is a memory access and we require an `int *` argument to be aligned.
Similar to https://reviews.llvm.org/D9673 , emit -fsanitize=alignment check for
arguments of builtin memcpy and memmove functions to catch misaligned load like:
```
// Check the alignment of a but ignore the alignment of b
void unaligned_load(int *a, void *b) { memcpy(a, b, sizeof(*a)); }
```

For a reference parameter, we emit a -fsanitize=alignment check as well, which
can be optimized out by InstCombinePass. We rely on the call site
`TCK_ReferenceBinding` check instead.
```
// The alignment check of a will be optimized out.
void unaligned_load(int &a, void *b) { memcpy(&a, b, sizeof(a)); }
```

The diagnostic message looks like
```
runtime error: store to misaligned address [[PTR:0x[0-9a-f]*]] for type 'int *'
```

We could use a better message for memcpy, but we don't do it for now as it would
require a new check name like misaligned-pointer-use, which is probably not
necessary. *RFC: Enforcing pointer type alignment in Clang* is not well documented,
but this patch does not intend to change the that.

Technically builtin memset functions can be checked for -fsanitize=alignment as
well, but it does not seem too useful.
  • Loading branch information
@MaskRay
MaskRay committed Oct 10, 2023
1 parent 4790578 commit 7926744
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 14 deletions.
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/Sanitizers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,8 @@ struct SanitizerSet {
Mask = Value ? (Mask | K) : (Mask & ~K);
}

void set(SanitizerMask K) { Mask = K; }

/// Disable the sanitizers specified in \p K.
void clear(SanitizerMask K = SanitizerKind::All) { Mask &= ~K; }

Expand Down
39 changes: 27 additions & 12 deletions clang/lib/CodeGen/CGBuiltin.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2730,6 +2730,27 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
}
}

// Check NonnullAttribute/NullabilityArg and Alignment.
auto EmitArgCheck = [&](TypeCheckKind Kind, Address A, const Expr *Arg,
unsigned ParmNum) {
Value *Val = A.getPointer();
EmitNonNullArgCheck(RValue::get(Val), Arg->getType(), Arg->getExprLoc(), FD,
ParmNum);

if (SanOpts.has(SanitizerKind::Alignment)) {
SanitizerSet SkippedChecks;
SkippedChecks.set(SanitizerKind::All);
SkippedChecks.clear(SanitizerKind::Alignment);
SourceLocation Loc = Arg->getExprLoc();
// Strip an implicit cast.
if (auto *CE = dyn_cast<ImplicitCastExpr>(Arg))
if (CE->getCastKind() == CK_BitCast)
Arg = CE->getSubExpr();
EmitTypeCheck(Kind, Loc, Val, Arg->getType(), A.getAlignment(),
SkippedChecks);
}
};

switch (BuiltinIDIfNoAsmLabel) {
default: break;
case Builtin::BI__builtin___CFStringMakeConstantString:
Expand Down Expand Up @@ -3720,10 +3741,8 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
Address Dest = EmitPointerWithAlignment(E->getArg(0));
Address Src = EmitPointerWithAlignment(E->getArg(1));
Value *SizeVal = EmitScalarExpr(E->getArg(2));
EmitNonNullArgCheck(RValue::get(Dest.getPointer()), E->getArg(0)->getType(),
E->getArg(0)->getExprLoc(), FD, 0);
EmitNonNullArgCheck(RValue::get(Src.getPointer()), E->getArg(1)->getType(),
E->getArg(1)->getExprLoc(), FD, 1);
EmitArgCheck(TCK_Store, Dest, E->getArg(0), 0);
EmitArgCheck(TCK_Load, Src, E->getArg(1), 1);
Builder.CreateMemCpy(Dest, Src, SizeVal, false);
if (BuiltinID == Builtin::BImempcpy ||
BuiltinID == Builtin::BI__builtin_mempcpy)
Expand All @@ -3738,10 +3757,8 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
Address Src = EmitPointerWithAlignment(E->getArg(1));
uint64_t Size =
E->getArg(2)->EvaluateKnownConstInt(getContext()).getZExtValue();
EmitNonNullArgCheck(RValue::get(Dest.getPointer()), E->getArg(0)->getType(),
E->getArg(0)->getExprLoc(), FD, 0);
EmitNonNullArgCheck(RValue::get(Src.getPointer()), E->getArg(1)->getType(),
E->getArg(1)->getExprLoc(), FD, 1);
EmitArgCheck(TCK_Store, Dest, E->getArg(0), 0);
EmitArgCheck(TCK_Load, Src, E->getArg(1), 1);
Builder.CreateMemCpyInline(Dest, Src, Size);
return RValue::get(nullptr);
}
Expand Down Expand Up @@ -3798,10 +3815,8 @@ RValue CodeGenFunction::EmitBuiltinExpr(const GlobalDecl GD, unsigned BuiltinID,
Address Dest = EmitPointerWithAlignment(E->getArg(0));
Address Src = EmitPointerWithAlignment(E->getArg(1));
Value *SizeVal = EmitScalarExpr(E->getArg(2));
EmitNonNullArgCheck(RValue::get(Dest.getPointer()), E->getArg(0)->getType(),
E->getArg(0)->getExprLoc(), FD, 0);
EmitNonNullArgCheck(RValue::get(Src.getPointer()), E->getArg(1)->getType(),
E->getArg(1)->getExprLoc(), FD, 1);
EmitArgCheck(TCK_Store, Dest, E->getArg(0), 0);
EmitArgCheck(TCK_Load, Src, E->getArg(1), 1);
Builder.CreateMemMove(Dest, Src, SizeVal, false);
return RValue::get(Dest.getPointer());
}
Expand Down
77 changes: 75 additions & 2 deletions clang/test/CodeGen/catch-undef-behavior.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
// CHECK-UBSAN: @[[SCHAR:.*]] = private unnamed_addr constant { i16, i16, [14 x i8] } { i16 0, i16 7, [14 x i8] c"'signed char'\00" }
// CHECK-UBSAN: @[[LINE_1500:.*]] = {{.*}}, i32 1500, i32 10 {{.*}} @[[FP16]], {{.*}} }

// CHECK-UBSAN: @[[PLONG:.*]] = private unnamed_addr constant { i16, i16, [9 x i8] } { i16 -1, i16 0, [9 x i8] c"'long *'\00" }
// CHECK-UBSAN: @[[LINE_1600:.*]] = {{.*}}, i32 1600, i32 10 {{.*}} @[[PLONG]], {{.*}} }

// PR6805
// CHECK-COMMON-LABEL: @foo
void foo(void) {
Expand Down Expand Up @@ -354,21 +357,69 @@ void call_decl_nonnull(int *a) {
decl_nonnull(a);
}

extern void *memcpy (void *, const void *, unsigned) __attribute__((nonnull(1, 2)));
extern void *memcpy(void *, const void *, unsigned long) __attribute__((nonnull(1, 2)));

// CHECK-COMMON-LABEL: @call_memcpy_nonnull
void call_memcpy_nonnull(void *p, void *q, int sz) {
// CHECK-COMMON: icmp ne ptr {{.*}}, null
// CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg
// CHECK-TRAP: call void @llvm.ubsantrap(i8 16)
// CHECK-COMMON-NOT: call

// CHECK-COMMON: icmp ne ptr {{.*}}, null
// CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg
// CHECK-TRAP: call void @llvm.ubsantrap(i8 16)
// CHECK-COMMON-NOT: call

// CHECK-COMMON: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %0, ptr align 1 %1, i64 %conv, i1 false)
memcpy(p, q, sz);
}

// CHECK-COMMON-LABEL: define{{.*}} void @call_memcpy(
void call_memcpy(long *p, short *q, int sz) {
// CHECK-COMMON: icmp ne ptr {{.*}}, null
// CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg(
// CHECK-TRAP: call void @llvm.ubsantrap(i8 16)
// CHECK-COMMON: and i64 %[[#]], 7, !nosanitize
// CHECK-COMMON: icmp eq i64 %[[#]], 0, !nosanitize
// CHECK-UBSAN: call void @__ubsan_handle_type_mismatch_v1(ptr @[[LINE_1600]]
// CHECK-TRAP: call void @llvm.ubsantrap(i8 22)

// CHECK-COMMON: icmp ne ptr {{.*}}, null
// CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg(
// CHECK-TRAP: call void @llvm.ubsantrap(i8 16)
// CHECK-COMMON: and i64 %[[#]], 1, !nosanitize
// CHECK-COMMON: icmp eq i64 %[[#]], 0, !nosanitize
// CHECK-UBSAN: call void @__ubsan_handle_type_mismatch_v1(
// CHECK-TRAP: call void @llvm.ubsantrap(i8 22)

// CHECK-COMMON: call void @llvm.memcpy.p0.p0.i64(ptr align 8 %0, ptr align 2 %1, i64 %conv, i1 false)

// CHECK-UBSAN-NOT: call void @__ubsan_handle_type_mismatch_v1(
// CHECK-COMMON: call void @llvm.memcpy.p0.p0.i64(ptr align 1 %[[#]], ptr align 1 %[[#]], i64 %{{.*}}, i1 false)
#line 1600
memcpy(p, q, sz);
/// Casting to void * or char * drops the alignment requirement.
memcpy((void *)p, (char *)q, sz);
}

extern void *memmove (void *, const void *, unsigned) __attribute__((nonnull(1, 2)));
// CHECK-COMMON-LABEL: define{{.*}} void @call_memcpy_inline(
void call_memcpy_inline(long *p, short *q) {
// CHECK-COMMON: and i64 %[[#]], 7, !nosanitize
// CHECK-COMMON: icmp eq i64 %[[#]], 0, !nosanitize
// CHECK-UBSAN: call void @__ubsan_handle_type_mismatch_v1(
// CHECK-TRAP: call void @llvm.ubsantrap(i8 22)

// CHECK-COMMON: and i64 %[[#]], 1, !nosanitize
// CHECK-COMMON: icmp eq i64 %[[#]], 0, !nosanitize
// CHECK-UBSAN: call void @__ubsan_handle_type_mismatch_v1(
// CHECK-TRAP: call void @llvm.ubsantrap(i8 22)

// CHECK-COMMON: call void @llvm.memcpy.inline.p0.p0.i64(ptr align 8 %0, ptr align 2 %1, i64 2, i1 false)
__builtin_memcpy_inline(p, q, 2);
}

extern void *memmove(void *, const void *, unsigned long) __attribute__((nonnull(1, 2)));

// CHECK-COMMON-LABEL: @call_memmove_nonnull
void call_memmove_nonnull(void *p, void *q, int sz) {
Expand All @@ -382,6 +433,28 @@ void call_memmove_nonnull(void *p, void *q, int sz) {
memmove(p, q, sz);
}

// CHECK-COMMON-LABEL: define{{.*}} void @call_memmove(
void call_memmove(long *p, short *q, int sz) {
// CHECK-COMMON: icmp ne ptr {{.*}}, null
// CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg(
// CHECK-TRAP: call void @llvm.ubsantrap(i8 16)
// CHECK-COMMON: and i64 %[[#]], 7, !nosanitize
// CHECK-COMMON: icmp eq i64 %[[#]], 0, !nosanitize
// CHECK-UBSAN: call void @__ubsan_handle_type_mismatch_v1(
// CHECK-TRAP: call void @llvm.ubsantrap(i8 22)

// CHECK-COMMON: icmp ne ptr {{.*}}, null
// CHECK-UBSAN: call void @__ubsan_handle_nonnull_arg(
// CHECK-TRAP: call void @llvm.ubsantrap(i8 16)
// CHECK-COMMON: and i64 %[[#]], 1, !nosanitize
// CHECK-COMMON: icmp eq i64 %[[#]], 0, !nosanitize
// CHECK-UBSAN: call void @__ubsan_handle_type_mismatch_v1(
// CHECK-TRAP: call void @llvm.ubsantrap(i8 22)

// CHECK-COMMON: call void @llvm.memmove.p0.p0.i64(ptr align 8 %0, ptr align 2 %1, i64 %conv, i1 false)
memmove(p, q, sz);
}

// CHECK-COMMON-LABEL: @call_nonnull_variadic
__attribute__((nonnull)) void nonnull_variadic(int a, ...);
void call_nonnull_variadic(int a, int *b) {
Expand Down
23 changes: 23 additions & 0 deletions compiler-rt/test/ubsan/TestCases/TypeCheck/misaligned.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// RUN: %clangxx %gmlt -fsanitize=alignment %s -O3 -o %t
// RUN: %run %t l0 && %run %t s0 && %run %t r0 && %run %t m0 && %run %t f0 && %run %t n0 && %run %t u0
// RUN: %run %t l1 2>&1 | FileCheck %s --check-prefix=CHECK-LOAD --strict-whitespace
// RUN: %run %t L1 2>&1 | FileCheck %s --check-prefix=CHECK-MEMCPY-LOAD
// RUN: %run %t S1 2>&1 | FileCheck %s --check-prefix=CHECK-MEMCPY-STORE
// RUN: %run %t r1 2>&1 | FileCheck %s --check-prefix=CHECK-REFERENCE
// RUN: %run %t m1 2>&1 | FileCheck %s --check-prefix=CHECK-MEMBER
// RUN: %run %t f1 2>&1 | FileCheck %s --check-prefix=CHECK-MEMFUN
Expand All @@ -15,6 +17,7 @@
// XFAIL: target={{.*openbsd.*}}

#include <new>
#include <string.h>

struct S {
S() {}
Expand Down Expand Up @@ -47,6 +50,16 @@ int main(int, char **argv) {
return *p && 0;
// CHECK-STACK-LOAD: #0 {{.*}}main{{.*}}misaligned.cpp

case 'L': {
int x;
// CHECK-MEMCPY-LOAD: misaligned.cpp:[[#@LINE+4]]{{(:16)?}}: runtime error: load of misaligned address [[PTR:0x[0-9a-f]*]] for type 'int *', which requires 4 byte alignment
// CHECK-MEMCPY-LOAD-NEXT: [[PTR]]: note: pointer points here
// CHECK-MEMCPY-LOAD-NEXT: {{^ 00 00 00 01 02 03 04 05}}
// CHECK-MEMCPY-LOAD-NEXT: {{^ \^}}
memcpy(&x, p, sizeof(x));
return x && 0;
}

case 's':
// CHECK-STORE: misaligned.cpp:[[@LINE+4]]{{(:5)?}}: runtime error: store to misaligned address [[PTR:0x[0-9a-f]*]] for type 'int', which requires 4 byte alignment
// CHECK-STORE-NEXT: [[PTR]]: note: pointer points here
Expand All @@ -55,6 +68,16 @@ int main(int, char **argv) {
*p = 1;
break;

case 'S': {
int x = 1;
// CHECK-MEMCPY-STORE: misaligned.cpp:[[#@LINE+4]]{{(:12)?}}: runtime error: store to misaligned address [[PTR:0x[0-9a-f]*]] for type 'int *', which requires 4 byte alignment
// CHECK-MEMCPY-STORE-NEXT: [[PTR]]: note: pointer points here
// CHECK-MEMCPY-STORE-NEXT: {{^ 00 00 00 01 02 03 04 05}}
// CHECK-MEMCPY-STORE-NEXT: {{^ \^}}
memcpy(p, &x, sizeof(x));
break;
}

case 'r':
// CHECK-REFERENCE: misaligned.cpp:[[@LINE+4]]{{(:(5|15))?}}: runtime error: reference binding to misaligned address [[PTR:0x[0-9a-f]*]] for type 'int', which requires 4 byte alignment
// CHECK-REFERENCE-NEXT: [[PTR]]: note: pointer points here
Expand Down

2 comments on commit 7926744

@thurstond
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is breaking a buildbot by exposing an error in an existing test:

https://lab.llvm.org/buildbot/#/builders/85/builds/19482

RUN: at line 28: /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llvm-objcopy /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/test/tools/llvm-objcopy/MachO/Output/universal-object.test.tmp.archive.containing.universal /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/test/tools/llvm-objcopy/MachO/Output/universal-object.test.tmp.archive.containing.universal.copy
+ /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llvm-objcopy /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/test/tools/llvm-objcopy/MachO/Output/universal-object.test.tmp.archive.containing.universal /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/test/tools/llvm-objcopy/MachO/Output/universal-object.test.tmp.archive.containing.universal.copy
/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/MachO/MachOReader.cpp:70:26: runtime error: load of misaligned address 0x5560ddc4326a for type 'const llvm::MachO::section *', which requires 4 byte alignment
0x5560ddc4326a: note: pointer points here
 00 00  00 00 5f 5f 74 65 78 74  00 00 00 00 00 00 00 00  00 00 5f 5f 54 45 58 54  00 00 00 00 00 00
              ^ 
    #0 0x5560dc66b584 in extractSections<llvm::MachO::section, llvm::MachO::segment_command> /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/MachO/MachOReader.cpp:70:5
    #1 0x5560dc66b584 in llvm::objcopy::macho::MachOReader::readLoadCommands(llvm::objcopy::macho::Object&) const /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/MachO/MachOReader.cpp:136:15
    #2 0x5560dc66d565 in llvm::objcopy::macho::MachOReader::create() const /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/MachO/MachOReader.cpp:368:17
    #3 0x5560dc654fbb in llvm::objcopy::macho::executeObjcopyOnBinary(llvm::objcopy::CommonConfig const&, llvm::objcopy::MachOConfig const&, llvm::object::MachOObjectFile&, llvm::raw_ostream&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp:445:48
    #4 0x5560dc65953b in llvm::objcopy::macho::executeObjcopyOnMachOUniversalBinary(llvm::objcopy::MultiFormatConfig const&, llvm::object::MachOUniversalBinary const&, llvm::raw_ostream&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/MachO/MachOObjcopy.cpp:537:19
    #5 0x5560dc5ea4e2 in llvm::objcopy::executeObjcopyOnBinary(llvm::objcopy::MultiFormatConfig const&, llvm::object::Binary&, llvm::raw_ostream&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/ObjCopy.cpp:66:12
    #6 0x5560dc5daa94 in llvm::objcopy::createNewArchiveMembers(llvm::objcopy::MultiFormatConfig const&, llvm::object::Archive const&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/Archive.cpp:40:19
    #7 0x5560dc5db87b in llvm::objcopy::executeObjcopyOnArchive(llvm::objcopy::MultiFormatConfig const&, llvm::object::Archive const&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/Archive.cpp:101:7
    #8 0x5560dc51d6fb in executeObjcopy /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llvm-objcopy/llvm-objcopy.cpp:180:21
    #9 0x5560dc51d6fb in llvm_objcopy_main(int, char**, llvm::ToolContext const&) /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/tools/llvm-objcopy/llvm-objcopy.cpp:253:19
    #10 0x5560dc51f90f in main /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/llvm-objcopy/llvm-objcopy-driver.cpp:15:10
    #11 0x7f9f68c23a8f  (/lib/x86_64-linux-gnu/libc.so.6+0x23a8f) (BuildId: ff2d8e707625b73b293961a4bc168e373d14a44a)
    #12 0x7f9f68c23b48 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x23b48) (BuildId: ff2d8e707625b73b293961a4bc168e373d14a44a)
    #13 0x5560dc4dece4 in _start (/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/bin/llvm-objcopy+0x4e2ce4)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/llvm/lib/ObjCopy/MachO/MachOReader.cpp:70:26 in

@thurstond
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please disregard the previous message; the MachO unaligned load has since been fixed in bfa7d10

Please sign in to comment.