Skip to content

Commit 2c93a40

Browse files
niaowaykevl
niaow
authored and
aykevl
committed
transform (MakeGCStackSlots): do not move the stack chain pop earlier
Previously, the MakeGCStackSlots pass would attempt to pop the stack chain before a tail call. This resulted in use-after-free bugs when the tail call allocated memory and used a value allocated by its caller. Instead of trying to move the stack chain pop, remove the tail flag from the call.
1 parent 9067576 commit 2c93a40

File tree

3 files changed

+65
-34
lines changed

3 files changed

+65
-34
lines changed

‎transform/gc.go

Lines changed: 10 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -271,37 +271,17 @@ func MakeGCStackSlots(mod llvm.Module) bool {
271271
// Make sure this stack object is popped from the linked list of stack
272272
// objects at return.
273273
for _, ret := range returns {
274-
inst := ret
275-
// Try to do the popping of the stack object earlier, by inserting
276-
// it not right before the return instruction but moving the insert
277-
// position up.
278-
// This is necessary so that the GC stack slot pass doesn't
279-
// interfere with tail calls (in particular, musttail calls).
280-
for {
281-
prevInst := llvm.PrevInstruction(inst)
282-
if prevInst == parent {
283-
break
284-
}
285-
if _, ok := pointerStores[prevInst]; ok {
286-
// Pop the stack object after the last store instruction.
287-
// This can probably be made more efficient: storing to the
288-
// stack chain object and then immediately popping isn't
289-
// useful.
290-
break
291-
}
292-
if prevInst.IsNil() {
293-
// Start of basic block. Pop the stack object here.
294-
break
295-
}
296-
if !prevInst.IsAPHINode().IsNil() {
297-
// Do not insert before a PHI node. PHI nodes must be
298-
// grouped at the beginning of a basic block before any
299-
// other instruction.
300-
break
301-
}
302-
inst = prevInst
274+
// Check for any tail calls at this return.
275+
prev := llvm.PrevInstruction(ret)
276+
if !prev.IsNil() && !prev.IsABitCastInst().IsNil() {
277+
// A bitcast can appear before a tail call, so skip backwards more.
278+
prev = llvm.PrevInstruction(prev)
279+
}
280+
if !prev.IsNil() && !prev.IsACallInst().IsNil() {
281+
// This is no longer a tail call.
282+
prev.SetTailCall(false)
303283
}
304-
builder.SetInsertPointBefore(inst)
284+
builder.SetInsertPointBefore(ret)
305285
builder.CreateStore(parent, stackChainStart)
306286
}
307287
}

‎transform/testdata/gc-stackslots.ll

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ target triple = "wasm32-unknown-unknown-wasm"
55

66
@runtime.stackChainStart = external global %runtime.stackChainObject*
77
@someGlobal = global i8 3
8+
@ptrGlobal = global i8** null
89

910
declare void @runtime.trackPointer(i8* nocapture readonly)
1011

@@ -20,8 +21,6 @@ define i8* @needsStackSlots() {
2021
; so tracking it is not really necessary.
2122
%ptr = call i8* @runtime.alloc(i32 4, i8* null)
2223
call void @runtime.trackPointer(i8* %ptr)
23-
; Restoring the stack pointer can happen at this position, before the return.
24-
; This avoids issues with tail calls.
2524
call void @someArbitraryFunction()
2625
%val = load i8, i8* @someGlobal
2726
ret i8* %ptr
@@ -103,3 +102,20 @@ define void @testGEPBitcast() {
103102
define void @someArbitraryFunction() {
104103
ret void
105104
}
105+
106+
define void @earlyPopRegression() {
107+
%x.alloc = call i8* @runtime.alloc(i32 4, i8* null)
108+
call void @runtime.trackPointer(i8* %x.alloc)
109+
%x = bitcast i8* %x.alloc to i8**
110+
; At this point the pass used to pop the stack chain, resulting in a potential use-after-free during allocAndSave.
111+
musttail call void @allocAndSave(i8** %x)
112+
ret void
113+
}
114+
115+
define void @allocAndSave(i8** %x) {
116+
%y = call i8* @runtime.alloc(i32 4, i8* null)
117+
call void @runtime.trackPointer(i8* %y)
118+
store i8* %y, i8** %x
119+
store i8** %x, i8*** @ptrGlobal
120+
ret void
121+
}

‎transform/testdata/gc-stackslots.out.ll

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ target triple = "wasm32-unknown-unknown-wasm"
55

66
@runtime.stackChainStart = internal global %runtime.stackChainObject* null
77
@someGlobal = global i8 3
8+
@ptrGlobal = global i8** null
89

910
declare void @runtime.trackPointer(i8* nocapture readonly)
1011

@@ -25,9 +26,9 @@ define i8* @needsStackSlots() {
2526
%ptr = call i8* @runtime.alloc(i32 4, i8* null)
2627
%4 = getelementptr { %runtime.stackChainObject*, i32, i8* }, { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject, i32 0, i32 2
2728
store i8* %ptr, i8** %4, align 4
28-
store %runtime.stackChainObject* %1, %runtime.stackChainObject** @runtime.stackChainStart, align 4
2929
call void @someArbitraryFunction()
3030
%val = load i8, i8* @someGlobal, align 1
31+
store %runtime.stackChainObject* %1, %runtime.stackChainObject** @runtime.stackChainStart, align 4
3132
ret i8* %ptr
3233
}
3334

@@ -75,8 +76,8 @@ define i8* @fibNext(i8* %x, i8* %y) {
7576
%out.alloc = call i8* @runtime.alloc(i32 1, i8* null)
7677
%4 = getelementptr { %runtime.stackChainObject*, i32, i8* }, { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject, i32 0, i32 2
7778
store i8* %out.alloc, i8** %4, align 4
78-
store %runtime.stackChainObject* %1, %runtime.stackChainObject** @runtime.stackChainStart, align 4
7979
store i8 %out.val, i8* %out.alloc, align 1
80+
store %runtime.stackChainObject* %1, %runtime.stackChainObject** @runtime.stackChainStart, align 4
8081
ret i8* %out.alloc
8182
}
8283

@@ -141,3 +142,37 @@ define void @testGEPBitcast() {
141142
define void @someArbitraryFunction() {
142143
ret void
143144
}
145+
146+
define void @earlyPopRegression() {
147+
%gc.stackobject = alloca { %runtime.stackChainObject*, i32, i8* }, align 8
148+
store { %runtime.stackChainObject*, i32, i8* } { %runtime.stackChainObject* null, i32 1, i8* null }, { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject, align 4
149+
%1 = load %runtime.stackChainObject*, %runtime.stackChainObject** @runtime.stackChainStart, align 4
150+
%2 = getelementptr { %runtime.stackChainObject*, i32, i8* }, { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject, i32 0, i32 0
151+
store %runtime.stackChainObject* %1, %runtime.stackChainObject** %2, align 4
152+
%3 = bitcast { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject to %runtime.stackChainObject*
153+
store %runtime.stackChainObject* %3, %runtime.stackChainObject** @runtime.stackChainStart, align 4
154+
%x.alloc = call i8* @runtime.alloc(i32 4, i8* null)
155+
%4 = getelementptr { %runtime.stackChainObject*, i32, i8* }, { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject, i32 0, i32 2
156+
store i8* %x.alloc, i8** %4, align 4
157+
%x = bitcast i8* %x.alloc to i8**
158+
call void @allocAndSave(i8** %x)
159+
store %runtime.stackChainObject* %1, %runtime.stackChainObject** @runtime.stackChainStart, align 4
160+
ret void
161+
}
162+
163+
define void @allocAndSave(i8** %x) {
164+
%gc.stackobject = alloca { %runtime.stackChainObject*, i32, i8* }, align 8
165+
store { %runtime.stackChainObject*, i32, i8* } { %runtime.stackChainObject* null, i32 1, i8* null }, { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject, align 4
166+
%1 = load %runtime.stackChainObject*, %runtime.stackChainObject** @runtime.stackChainStart, align 4
167+
%2 = getelementptr { %runtime.stackChainObject*, i32, i8* }, { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject, i32 0, i32 0
168+
store %runtime.stackChainObject* %1, %runtime.stackChainObject** %2, align 4
169+
%3 = bitcast { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject to %runtime.stackChainObject*
170+
store %runtime.stackChainObject* %3, %runtime.stackChainObject** @runtime.stackChainStart, align 4
171+
%y = call i8* @runtime.alloc(i32 4, i8* null)
172+
%4 = getelementptr { %runtime.stackChainObject*, i32, i8* }, { %runtime.stackChainObject*, i32, i8* }* %gc.stackobject, i32 0, i32 2
173+
store i8* %y, i8** %4, align 4
174+
store i8* %y, i8** %x, align 4
175+
store i8** %x, i8*** @ptrGlobal, align 4
176+
store %runtime.stackChainObject* %1, %runtime.stackChainObject** @runtime.stackChainStart, align 4
177+
ret void
178+
}

0 commit comments

Comments
 (0)