llvm / llvm/llvm-project

[clang][UBSan] array-bounds misses off-by-one accesses: Accessed is not threaded through lvalue emission

Open
#215,699 10 comments 0 reactions 1 assignee Claimed by @usama54321 View on GitHub
clang:codegen
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

With `-fsanitize=array-bounds`, whether an index equal to the array bound is an error comes down to the `Accessed` argument of `CodeGenFunction::EmitArraySubscriptExpr` which tells whether the LValue will be used to access the object. `EmitBoundsCheckImpl` emits `icmp ult` when Accessed is set and `icmp ule` when it is not, because `&a[N]` has to stay legal while dereferencing `a[N]` does not. There are a couple of issues with the way this argument is passed around currently.

Firstly, the only general place that sets it is `EmitCheckedLValue`, and it does so by testing `isa(E)` on the outermost node it was handed. That test fails whenever anything sits on top of the subscript. In short, Accessed is currently not threaded through correctly and the information is lost. Some examples are:

- a `ParenExpr`, including the ones macros add
- an `ImplicitCastExpr` with `CK_NoOp`, which Sema inserts around the object argument of a `const` member function in `PerformImplicitObjectArgumentInitialization`, so `a[N].size()` is affected (where a is an aggregate)
- `ConditionalOperator`
- a comma `BinaryOperator` etc.

Second, many callers never set the flag at all, because they call `EmitLValue` rather than `EmitCheckedLValue`. `CGExprComplex.cpp` does this at all 6 of its sites, including `EmitLValue(E->getLHS())` for complex assignment, so a store to `carr[N]` gets the permissive form no matter what the AST looks like. Note that this also skips the null, alignment and vptr checks that `EmitCheckedLValue` emits through `EmitTypeCheck`, which is a separate question per call site.

```c
// Does -fsanitize=array-bounds catch an access one element past the end?
// clang -fsanitize=array-bounds -Wno-array-bounds -O0 bounds-c.c -o bounds-c && ./bounds-c
// Every function below reads or writes arr[4] of a 4-element array, which is
// undefined behaviour, except address_only() which is legal and must stay quiet.
struct S { int f, g; };

int arr[4]; int pad1[8];
struct S sarr[4]; int pad2[8];
_Complex double carr[4]; int pad3[8];

#define ELEM(a, i) ((a)[i])

void plain_store(void) { arr[4] = 1; }
void paren_store(void) { (arr[4]) = 1; }
void macro_store(void) { ELEM(arr, 4) = 1; }
void struct_store(struct S s) { (sarr[4]) = s; }
void field_store(void) { (sarr[4]).f = 1; }
void complex_store(void) { carr[4] = 1.0; }
double complex_read(void) { return __real__ carr[4]; }
void complex_compound(void) { carr[4] += 1.0; }
int *address_only(void) { return &arr[4]; } /* legal: must stay quiet */

int main(void) {
struct S s = {0, 0};
plain_store();
paren_store();
macro_store();
struct_store(s);
field_store();
complex_store();
(void)complex_read();
complex_compound();
(void)address_only();
return 0;
}
```

```c++
// Does -fsanitize=array-bounds catch an access one element past the end?
// clang++ -std=c++17 -fsanitize=array-bounds -Wno-array-bounds -O0 bounds-cxx.cpp -o bounds-cxx && ./bounds-cxx
// Every function below accesses tarr[4]/narr[4] of a 4-element array, which is
// undefined behaviour, except address_only() which is legal and must stay quiet.
struct Triv {
int x;
int get() const { return 0; }
};
struct NonTriv {
int x;
NonTriv &operator=(const NonTriv &) { return *this; }
~NonTriv() {}
int get() const { return 0; }
};

Triv tarr[4]; int pad1[8];
NonTriv narr[4]; int pad2[8];

using PMF = int (Triv::*)() const;

int const_member_call(void) { return tarr[4].get(); }
int nontrivial_member_call(void) { return narr[4].get(); }
void trivial_assign(Triv t) { tarr[4] = t; }
void nontrivial_assign(const NonTriv &n) { narr[4] = n; }
void destructor_call(void) { narr[4].~NonTriv(); }
int via_member_pointer(PMF pmf) { return (tarr[4].*pmf)(); }
int copy_init(void) { Triv c = tarr[4]; return c.x; }
int conditional(bool b) { return (b ? tarr[4] : tarr[0]).get(); }
int comma(void) { return ((void)0, tarr[4]).get(); }
Triv *address_only(void) { return &tarr[4]; } // legal: must stay quiet

int main(int argc, char **) {
Triv t; NonTriv n;
(void)const_member_call();
(void)nontrivial_member_call();
trivial_assign(t);
nontrivial_assign(n);
destructor_call();
(void)via_member_pointer(&Triv::get);
(void)copy_init();
(void)conditional(argc != 0);
(void)comma();
(void)address_only();
return 0;
}
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.