Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix stack-buffer-overflow in generated code #46260

Closed
wants to merge 1 commit into from
Closed

Fix stack-buffer-overflow in generated code #46260

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/codegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4341,7 +4341,7 @@ static jl_cgval_t emit_varinfo(jl_codectx_t &ctx, jl_varinfo_t &vi, jl_sym_t *va
}
else {
// copy value to a non-mutable (non-volatile SSA) location
AllocaInst *varslot = cast<AllocaInst>(vi.value.V);
AllocaInst *varslot = cast<AllocaInst>(vi.value.V->stripPointerCasts());
Type *T = varslot->getAllocatedType();
assert(!varslot->isArrayAllocation() && "variables not expected to be VLA");
AllocaInst *ssaslot = cast<AllocaInst>(varslot->clone());
Expand Down Expand Up @@ -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) {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This seems now just to be i8, which would not be good

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so we should use julia_type_to_llvm(vi.value.typ) then?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

We might need to force this to become a boxroot

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or I guess late-gc-lowering does look at the AllocaInst type ... sigh.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess for now, we could just insert an appropriate amount of padding at the LLVM type if the allocation is not aligned?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe we could stop rounding up the datatype size?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I think we should stop rounding, since at best it may waste space that could be useful

// make sure gc pointers (including ptr_phi of union-split) are initialized to NULL
ctx.builder.CreateStore(Constant::getNullValue(T), vi.value.V, true);
Expand Down Expand Up @@ -7055,7 +7055,12 @@ static jl_llvm_functions_t
Type *vtype = julia_type_to_llvm(ctx, jt, &isboxed);
assert(!isboxed);
assert(!type_is_ghost(vtype) && "constants should already be handled");
Value *lv = new AllocaInst(vtype, M->getDataLayout().getAllocaAddrSpace(), jl_symbol_name(s), /*InsertBefore*/ctx.topalloca);
Type *alloc_type = ArrayType::get(getInt8Ty(ctx.builder.getContext()), jl_datatype_size(jt));
Value *lv = new AllocaInst(alloc_type, M->getDataLayout().getAllocaAddrSpace(), nullptr,
Align(jl_datatype_align(jt)), jl_symbol_name(s), /*InsertBefore*/ctx.topalloca);
#ifndef JL_LLVM_OPAQUE_POINTERS
lv = new BitCastInst(lv, PointerType::get(vtype, M->getDataLayout().getAllocaAddrSpace()), "", /*InsertBefore*/ctx.topalloca);
#endif
if (CountTrackedPointers(vtype).count) {
StoreInst *SI = new StoreInst(Constant::getNullValue(vtype), lv, false, Align(sizeof(void*)));
SI->insertAfter(ctx.topalloca);
Expand Down