clang mislowers C function parameters that are arrays with `static` length into LLVM's `dereferenceable`

[ais523]

When compiling a function that takes a function argument that's a static-length array (e.g. int x[static 1048576]), Clang lowers this into LLVM's dereferenceable intrinsic. However, as far as I can tell from the C standards, static on an array length doesn't guarantee that the array will be vaild for the entire function – only at the point of the function call. The lowering could therefore lead to a miscompile if the array is freed within the function.

Here's an example of a (contrived) end-to-end miscompile, using two files foo.c and bar.c, which are separately compiled at -O3 -g and then linked with -g but no optimisation options (I tested this on x86-64 but suspect it affects other platforms too):

foo.c

extern int bar(int x[static 1048576]);
int foo(int x[static 1048576])
{
    *x = 1;
    int y = bar(x);
    int z = 0;
    while (y--) z += *x;
    return z;
}

bar.c

#include <stdlib.h>

extern int foo(int x[static 1048576]);
int bar(int x[static 1048586])
{
    free(x);
    return 0;
}

int main(void)
{
    int *x = calloc(1048576, sizeof (int));
    return foo(x);
}

foo incorrectly gets a dereferenceable on the argument – I think it should instead be an llvm.assume … "dereferenceable" at the start of the function body itself (i.e. to guarantee that all 1048576 elements of x are dereferenceable at the start of the function, without necessarily guaranteeing that they aren't freed since). With some memory allocators, e.g. glibc calloc, this causes a segmentation fault at runtime, because a memory allocation that large is returned to the OS by free, but (relying on the dereferenceable) LLVM attempts to unconditionally read from it anyway (without checking to see whether y is 0 first).

It is possible that I'm mistaken about what the C standard requires here (although I checked with someone more knowledgeable in the C standards than me, and they agreed with my interpretation).

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: None (ais523)

When compiling a function that takes a function argument that's a `static`-length array (e.g. `int x[static 1048576]`), Clang lowers this into LLVM's `dereferenceable` intrinsic. However, as far as I can tell from the C standards, `static` on an array length doesn't guarantee that the array will be vaild for the entire function – only at the point of the function call. The lowering could therefore lead to a miscompile if the array is freed within the function.

Here's an example of a (contrived) end-to-end miscompile, using two files foo.c and bar.c, which are separately compiled at -O3 -g and then linked with -g but no optimisation options (I tested this on x86-64 but suspect it affects other platforms too):

<blockquote>

foo.c

extern int bar(int x[static 1048576]);
int foo(int x[static 1048576])
{
    *x = 1;
    int y = bar(x);
    int z = 0;
    while (y--) z += *x;
    return z;
}

bar.c

#include <stdlib.h>

extern int foo(int x[static 1048576]);
int bar(int x[static 1048586])
{
    free(x);
    return 0;
}

int main(void)
{
    int *x = calloc(1048576, sizeof (int));
    return foo(x);
}

</blockquote>

foo incorrectly gets a dereferenceable on the argument – I think it should instead be an llvm.assume … "dereferenceable" at the start of the function body itself (i.e. to guarantee that all 1048576 elements of x are dereferenceable at the start of the function, without necessarily guaranteeing that they aren't freed since). With some memory allocators, e.g. glibc calloc, this causes a segmentation fault at runtime, because a memory allocation that large is returned to the OS by free, but (relying on the dereferenceable) LLVM attempts to unconditionally read from it anyway (without checking to see whether y is 0 first).

It is possible that I'm mistaken about what the C standard requires here (although I checked with someone more knowledgeable in the C standards than me, and they agreed with my interpretation).

[AaronBallman]

C23 6.7.7.4p6: ... If the keyword static also appears within the [ and ] of the array type derivation, then for each call to the function, the value of the corresponding actual argument shall provide access to the first element of an array with at least as many elements as specified by the size expression.

Based on that, I agree with the assessment here; the array has to be valid on entry to the function, but the body of the function can still alter the array by freeing it. The LLVM documentation for dereferenceable is:

dereferenceable(<n>)

This indicates that the parameter or return pointer is dereferenceable. This attribute may only be applied to pointer-typed parameters. A pointer that is dereferenceable can be loaded from speculatively without a risk of trapping. The number of bytes known to be dereferenceable must be provided in parentheses. It is legal for the number of bytes to be less than the size of the pointee type. The nonnull attribute does not imply dereferenceability (consider a pointer to one element past the end of an array), however dereferenceable() does imply nonnull in addrspace(0) (which is the default address space), except if the null_pointer_is_valid function attribute is present. n should be a positive number. The pointer should be well defined, otherwise it is undefined behavior. This means dereferenceable() implies noundef. When used in an assume operand bundle, more restricted semantics apply. See assume operand bundles for more details.

which has this important bit:

The pointer should be well defined, otherwise it is undefined behavior.

Once you free the pointer, the pointer is no longer well-defined, so I think this is a misuse of dereferenceable on the Clang CodeGen side. CC @nikic @efriedma-quic for more opinions.

[efriedma-quic]

Maybe worth poking the committee to clarify the spec.

We have __builtin_assume_dereferenceable now; we can use the same lowering here, if we want. But "static" is so infrequently used that maybe it's not worth it.

[AaronBallman]

Maybe worth poking the committee to clarify the spec.

I can do that if there's confusion still, but I'm not certain there's confusion on the spec side.

We have __builtin_assume_dereferenceable now; we can use the same lowering here, if we want. But "static" is so infrequently used that maybe it's not worth it.

It is reasonably well-used in the wild, so I think it's worth investing in it (we also have diagnostic improvements we could make): https://sourcegraph.com/search?q=context:global+lang:C+%5C%5Bstatic+%5B0-9%2B%5D%5C%5D+count:10000000&patternType=regexp&sm=0

[efriedma-quic]

I'm not sure "for each call to the function" is sufficiently precise to indicate that it only applies at the point where the function is called, and not for the duration of the call. Particularly considering the notion of the "point where the function is called" is a little ambiguous: what happens if free() is called on another thread?

But if everyone agrees it's a point-in-time check, fine.