Clang ssp-buffer-size misinterprets some IR-level padding as arrays

[davidben]

-fstack-protector inserts stack canaries into functions that have sufficiently large character buffers on the stack. I guess the idea is we don't want to pay for a stack protector on every function. So we say that, if the programmer (not the compiler) puts a character array somewhere, they are at risk of overflowing it, so it is worth adding a stack protector to just those functions.

However, Clang sometimes lowers structs to IR types that contain explicit padding in the form of arrays. The SSP implementation seems to look at IR types, not C types. It then misreads this padding as a C character array. The default threshold of 8 is large enough that this rarely happens, but if one passes --param=ssp-buffer-size=4 to the compiler, it becomes more common.

See godbolt links below: https://godbolt.org/z/6nM453Kdh https://godbolt.org/z/cvo1sh87s

Instead, it should only be looking at the C types. In so far as we believe large character stack buffers are more at risk of overflow, that heuristic should only apply to arrays from the programmer and not from the compiler.

(This seems to be the cause of at least some of the binary size regression that Chrome sees when switching from Abseil's absl::optional to libc++'s std::optional. Abseil uses bool; T while libc++ uses T; bool. The ordering change tickles this bug and so we burn binary size by adding more stack protectors. Not what I expected to find when starting down this journey!)

[zmodem]

My shallow understanding of how this works: Clang codegens these by putting "stack-protector-buffer-size"="4" attributes on functions in the LLVM IR. The LLVM StackProtector pass then looks at that attribute when deciding whether or not to insert the check: https://github.com/llvm/llvm-project/blob/llvmorg-17.0.0/llvm/lib/CodeGen/StackProtector.cpp#L275 At that level, it might have trouble differentiating between user- and compiler-generated stack allocations.

[zmodem]

(For reference, the chromium bug is https://crbug.com/1484342)

[llvmbot]

@llvm/issue-subscribers-clang-codegen

`-fstack-protector` inserts stack canaries into functions that have sufficiently large character buffers on the stack. I guess the idea is we don't want to pay for a stack protector on every function. So we say that, if the programmer (not the compiler) puts a character array somewhere, they are at risk of overflowing it, so it is worth adding a stack protector to just those functions. However, Clang sometimes lowers structs to IR types that contain explicit padding in the form of arrays. The SSP implementation seems to look at IR types, not C types. It then misreads this padding as a C character array. The default threshold of 8 is large enough that this rarely happens, but if one passes `--param=ssp-buffer-size=4` to the compiler, it becomes more common. See godbolt links below: https://godbolt.org/z/6nM453Kdh https://godbolt.org/z/cvo1sh87s Instead, it should only be looking at the C types. In so far as we believe large character stack buffers are more at risk of overflow, that heuristic should only apply to arrays from the programmer and not from the compiler. (This seems to be the cause of at least some of the binary size regression that Chrome sees when switching from Abseil's `absl::optional` to libc++'s `std::optional`. Abseil uses `bool; T` while libc++ uses `T; bool`. The ordering change tickles this bug and so we burn binary size by adding more stack protectors. Not what I expected to find when starting down this journey!)

[aeubanks]

The current implementation of the stack protector seems iffy. As Hans said, we currently can't differentiate between user and compiler added allocas.

Also, the current implementation looks for allocas of an array type, but we'd like to move away from specifying types on allocas and just the size since LLVM treats memory as an untyped bundle of bytes.

I think the proper fix is to have the frontend do the analysis for which functions should get the stack protector, especially since the frontend knows best if something is an array on the stack. Or if you want to make fewer changes to the existing implementation, have the frontend mark potentially interesting allocas with some metadata and have stack protector only look at those.

[rnk]

We discussed this in our sync, and to slightly modify Arthur's suggestion, I would say that the frontend should analyze the AST types of the allocas it makes, and it should then mark allocas that contain arrays. After optimizations, LLVM should put a stack guard in any function still containing a marked alloca. The idea is that this handles optimizations like SROA and inlining better than if the frontend is marking the functions which should get stack guards.

This does, however, require frontends to do work, and is something that has to be replicated in all non-clang frontends that care about stack guards.