Skip to content

Commit dbaa2d3

Browse files
committed
cmd/compile: do nil check before calling duff functions, on arm64 and amd64
On these platforms, we set up a frame pointer record below the current stack pointer, so when we're in duffcopy or duffzero, we get a reasonable traceback. See #73753. But because this frame pointer record is below SP, it is vulnerable. Anything that adds a new stack frame to the stack might clobber it. Which actually happens in #73748 on amd64. I have not yet come across a repro on arm64, but might as well be safe here. The only real situation this could happen is when duffzero or duffcopy is passed a nil pointer. So we can just avoid the problem by doing the nil check outside duffzero/duffcopy. That way we never add a frame below duffzero/duffcopy. (Most other ways to get a new frame below the current one, like async preempt or debugger-generated calls, don't apply to duffzero/duffcopy because they are runtime functions; we're not allowed to preempt there.) Longer term, we should stop putting stuff below SP. #73753 will include that as part of its remit. But that's not for 1.25, so we'll do the simple thing for 1.25 for this issue. Fixes #73748 Change-Id: I913c49ee46dcaee8fb439415a4531f7b59d0f612 Reviewed-on: https://go-review.googlesource.com/c/go/+/676916 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Cherry Mui <[email protected]> Reviewed-by: Keith Randall <[email protected]>
1 parent 6160fa5 commit dbaa2d3

File tree

5 files changed

+92
-34
lines changed

5 files changed

+92
-34
lines changed

src/cmd/compile/internal/ssa/_gen/AMD64Ops.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -897,8 +897,8 @@ func init() {
897897
inputs: []regMask{buildReg("DI")},
898898
clobbers: buildReg("DI"),
899899
},
900-
faultOnNilArg0: true,
901-
unsafePoint: true, // FP maintenance around DUFFCOPY can be clobbered by interrupts
900+
//faultOnNilArg0: true, // Note: removed for 73748. TODO: reenable at some point
901+
unsafePoint: true, // FP maintenance around DUFFCOPY can be clobbered by interrupts
902902
},
903903

904904
// arg0 = address of memory to zero
@@ -935,10 +935,10 @@ func init() {
935935
inputs: []regMask{buildReg("DI"), buildReg("SI")},
936936
clobbers: buildReg("DI SI X0"), // uses X0 as a temporary
937937
},
938-
clobberFlags: true,
939-
faultOnNilArg0: true,
940-
faultOnNilArg1: true,
941-
unsafePoint: true, // FP maintenance around DUFFCOPY can be clobbered by interrupts
938+
clobberFlags: true,
939+
//faultOnNilArg0: true, // Note: removed for 73748. TODO: reenable at some point
940+
//faultOnNilArg1: true,
941+
unsafePoint: true, // FP maintenance around DUFFCOPY can be clobbered by interrupts
942942
},
943943

944944
// arg0 = destination pointer

src/cmd/compile/internal/ssa/_gen/ARM64Ops.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -554,8 +554,8 @@ func init() {
554554
inputs: []regMask{buildReg("R20")},
555555
clobbers: buildReg("R16 R17 R20 R30"),
556556
},
557-
faultOnNilArg0: true,
558-
unsafePoint: true, // FP maintenance around DUFFZERO can be clobbered by interrupts
557+
//faultOnNilArg0: true, // Note: removed for 73748. TODO: reenable at some point
558+
unsafePoint: true, // FP maintenance around DUFFZERO can be clobbered by interrupts
559559
},
560560

561561
// large zeroing
@@ -595,9 +595,9 @@ func init() {
595595
inputs: []regMask{buildReg("R21"), buildReg("R20")},
596596
clobbers: buildReg("R16 R17 R20 R21 R26 R30"),
597597
},
598-
faultOnNilArg0: true,
599-
faultOnNilArg1: true,
600-
unsafePoint: true, // FP maintenance around DUFFCOPY can be clobbered by interrupts
598+
//faultOnNilArg0: true, // Note: removed for 73748. TODO: reenable at some point
599+
//faultOnNilArg1: true,
600+
unsafePoint: true, // FP maintenance around DUFFCOPY can be clobbered by interrupts
601601
},
602602

603603
// large move

src/cmd/compile/internal/ssa/opGen.go

Lines changed: 17 additions & 23 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

test/fixedbugs/issue73748a.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// run
2+
3+
// Copyright 2025 The Go Authors. All rights reserved.
4+
// Use of this source code is governed by a BSD-style
5+
// license that can be found in the LICENSE file.
6+
7+
package main
8+
9+
import (
10+
"context"
11+
"io"
12+
"runtime/trace"
13+
)
14+
15+
type T struct {
16+
a [16]int
17+
}
18+
19+
//go:noinline
20+
func f(x *T) {
21+
*x = T{}
22+
}
23+
24+
func main() {
25+
trace.Start(io.Discard)
26+
defer func() {
27+
recover()
28+
trace.Log(context.Background(), "a", "b")
29+
30+
}()
31+
f(nil)
32+
}

test/fixedbugs/issue73748b.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// run
2+
3+
// Copyright 2025 The Go Authors. All rights reserved.
4+
// Use of this source code is governed by a BSD-style
5+
// license that can be found in the LICENSE file.
6+
7+
package main
8+
9+
import (
10+
"context"
11+
"io"
12+
"runtime/trace"
13+
)
14+
15+
type T struct {
16+
a [16]int
17+
}
18+
19+
//go:noinline
20+
func f(x, y *T) {
21+
*x = *y
22+
}
23+
24+
func main() {
25+
trace.Start(io.Discard)
26+
defer func() {
27+
recover()
28+
trace.Log(context.Background(), "a", "b")
29+
30+
}()
31+
f(nil, nil)
32+
}

0 commit comments

Comments
 (0)