Fix stack-buffer-overflow in generated code #46260
Conversation
When generating code for a local, we were using the LLVM type for the allocation. However, we were assuming that the allocation was sized according to the julia datatype size. These two sizes do not match, as the julia size is rounded up to alignment, causing a stack buffer overflow.
| @@ -4721,7 +4721,7 @@ static void emit_upsilonnode(jl_codectx_t &ctx, ssize_t phic, jl_value_t *val) | |||
| } | |||
| else if (vi.value.V && !vi.value.constant && vi.value.typ != jl_bottom_type) { | |||
| assert(vi.value.ispointer()); | |||
| Type *T = cast<AllocaInst>(vi.value.V)->getAllocatedType(); | |||
| Type *T = cast<AllocaInst>(vi.value.V->stripPointerCasts())->getAllocatedType(); | |||
| if (CountTrackedPointers(T).count) { | |||
There was a problem hiding this comment.
This seems now just to be i8, which would not be good
There was a problem hiding this comment.
Ok, so we should use julia_type_to_llvm(vi.value.typ) then?
There was a problem hiding this comment.
I am not entirely sure, since it seems like we might be losing the root here anyways. There was a thought that we might use the alloca type in the future to ensure we got all of the roots (even in the presence of stack-spills), but it was mostly never implemented.
There was a problem hiding this comment.
We might need to force this to become a boxroot
There was a problem hiding this comment.
It would be similarly good to know how the other branch of this change is reached, since typically we may lose gc-roots if they get shoved onto the stack, so we should not do that
There was a problem hiding this comment.
Let me change this to julia_type_to_llvm(vi.value.typ) for now, which will preserve current behavior and then we can separately track your concerns.
There was a problem hiding this comment.
Or I guess late-gc-lowering does look at the AllocaInst type ... sigh.
There was a problem hiding this comment.
I guess for now, we could just insert an appropriate amount of padding at the LLVM type if the allocation is not aligned?
There was a problem hiding this comment.
Or maybe we could stop rounding up the datatype size?
There was a problem hiding this comment.
I think we should stop rounding, since at best it may waste space that could be useful
Padding is then added when creating the struct layout, but otherwise the memory layout should be unchanged. This is an alternative to the proposal in #46260. LLVM size and julia size should now be aligned.
Padding is then added when creating the struct layout, but otherwise the memory layout should be unchanged. This is an alternative to the proposal in #46260. LLVM size and julia size should now be aligned.
Padding is then added when creating the struct layout, but otherwise the memory layout should be unchanged. This is an alternative to the proposal in #46260. LLVM size and julia size should now be aligned.
Padding is then added when creating the struct layout, but otherwise the memory layout should be unchanged. This is an alternative to the proposal in #46260. LLVM size and julia size should now be aligned.
Padding is then added when creating the struct layout, but otherwise the memory layout should be unchanged. This is an alternative to the proposal in #46260. LLVM size and julia size should now be aligned.
When generating code for a local, we were using the LLVM type
for the allocation. However, we were assuming that the allocation
was sized according to the julia datatype size. These two sizes
do not match, as the julia size is rounded up to alignment,
causing a stack buffer overflow.