x/tools/gopls: impossible crash in persistent.Map decref

[adonovan]

#!stacks
"runtime.sigpanic" &&
("(*mapNode).decref:+7" ||
 "(*mapNode).decref:+5") /* newer variant */

Issue created by stacks.

This stack XrmcvQ was reported by telemetry:

• crash/crash
• runtime.gopanic:+69
• runtime.panicmem:=262
• runtime.sigpanic:+19
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+7
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+13
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+13
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+12
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+12
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+12
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+12
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+12
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+13
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+12
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+12
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+12
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+13
• golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref:+12

golang.org/x/tools/gopls@v0.18.1 go1.23.6 linux/amd64 vscode (1)

Dups: sqtkfg

[adonovan]

func (node *mapNode) decref() {
	...
	if atomic.AddInt32(&node.refCount, -1) == 0 {
		if atomic.AddInt32(&node.value.refCount, -1) == 0 {
			if node.value.release != nil {
				node.value.release(node.key, node.value.value) <---------- panic here
}

golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref STEXT size=245 args=0x8 locals=0x28 funcid=0x0 align=0x0
	0x0000 00000 	L0115	TEXT	golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref(SB), ABIInternal, $40-8
	0x0000 00000 	L0115	CMPQ	SP, 16(R14)
	0x0004 00004 	L0115	PCDATA	$0, $-2
	0x0004 00004 	L0115	JLS	225
	0x000a 00010 	L0115	PCDATA	$0, $-1
	0x000a 00010 	L0115	PUSHQ	BP
	0x000b 00011 	L0115	MOVQ	SP, BP
	0x000e 00014 	L0115	SUBQ	$32, SP
	0x0012 00018 	L0115	FUNCDATA	$0, gclocals·wvjpxkknJ4nY1JtrArJJaw==(SB)
	0x0012 00018 	L0115	FUNCDATA	$1, gclocals·J26BEvPExEQhJvjp9E8Whg==(SB)
	0x0012 00018 	L0115	FUNCDATA	$5, golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref.arginfo1(SB)
	0x0012 00018 	L0115	FUNCDATA	$6, golang.org/x/tools/gopls/internal/util/persistent.(*mapNode).decref.argliveinfo(SB)
	0x0012 00018 	L0115	PCDATA	$3, $1
	0x0012 00018 	L0116	TESTQ	AX, AX
	0x0015 00021 	L0116	JEQ	219
	0x001b 00027 	L0119	MOVL	$-1, SI
	0x0020 00032 	L0119	LOCK
	0x0021 00033 	L0119	XADDL	SI, 32(AX)
	0x0025 00037 	L0119	DECL	SI
	0x0027 00039 	L0119	TESTL	SI, SI
	0x0029 00041 	L0119	JNE	213
	0x002f 00047 	L0116	MOVQ	AX, golang.org/x/tools/gopls/internal/util/persistent.node+48(SP)
	0x0034 00052 	L0116	PCDATA	$3, $-1
	0x0034 00052 	L0120	MOVQ	16(AX), SI
	0x0038 00056 	L0120	MOVL	$-1, R8
	0x003e 00062 	L0120	LOCK
	0x003f 00063 	L0120	XADDL	R8, (SI)
	0x0043 00067 	L0120	LEAL	-1(R8), SI
	0x0047 00071 	L0120	TESTL	SI, SI
	0x0049 00073 	L0120	JNE	190
	0x004b 00075 	L0121	MOVQ	16(AX), SI
	0x004f 00079 	L0121	MOVQ	24(SI), DX
	0x0053 00083 	L0121	TESTQ	DX, DX
	0x0056 00086 	L0121	JEQ	114
--- the trap happened on one of these lines --
; AX=node SI=&node->value DX=node.value.release
; AX and SI have been loaded through; DX is nonzero
	0x0058 00088 	L0122	MOVQ	(DX), R8		-- load func ptr from node.value.release closure
	0x005b 00091 	L0122	MOVQ	8(AX), BX	-- node.key.v
	0x005f 00095 	L0122	MOVQ	(AX), AX          -- node.key.t
	0x0062 00098 	L0122	MOVQ	8(SI), CX         -- node.value.value.t
	0x0066 00102 	L0122	MOVQ	16(SI), DI         -- node.value.value.v
	0x006a 00106 	L0122	PCDATA	$1, $0
	0x006a 00106 	L0122	CALL	R8
---
	0x006d 00109 	L0124	MOVQ	golang.org/x/tools/gopls/internal/util/persistent.node+48(SP), AX
	...

Possibilities:

• DX is a pointer to invalid memory. This means node.value.release is corrupt.
• R8 is a pointer to bad code. This means the node.value.release closure is corrupt.

[adonovan]

cc @prattmic

[gabyhelp]

Related Issues

• x/tools/gopls: sigpanic in persistent.(*mapNode).forEach (bad SP) #71115
• x/tools/gopls: type assertion panics in persistent.Map.Get #72027
• x/tools/gopls: xrefs.Index panics after "can't fail" pgf.PosRange #70446 (closed)
• x/tools/gopls: nil deref in Exporter.ProcessEvent #71928
• x/tools/gopls: analysisinternal.ZeroValue crash on (presumably) unsafe.Pointer #70585 (closed)
• x/tools/gopls: Hover: invalid nil entry in types.Defs map #69362 (closed)
• x/tools/gopls: Hover: nil deref in parseDocLink #70635 (closed)
• x/tools/gopls: Rename: crash due to Info.Defs[id]=nil bug in checkStructField #70968
• runtime: SEGV in mapaccess2 (go1.23.2 darwin/amd64) #71123
• x/tools/gopls: nil deref panic in go.test code action #72907 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[findleyr]

This is our third crash related to persistent.Map.

Does that indicate a data race to one of our maps? (I don't know if these are realistic manifestations of races).
Unfortunately, due to the recursive nature of map operations, we never get a useful call stack.

[adonovan]

Does that indicate a data race to one of our maps?

The map is a pure functional data structure: the only assignment to any of its nodes or values occurs when the refcount falls to zero in decref, and those are purely defensive stores that never need to be observed for correctness. I would expect this to make the map more robust against data races in the program than other code. I suspect the bug is somewhere else and the map is just getting clobbered, but I suppose it wouldn't hurt to write a concurrent test of Map.

[adonovan]

Yep, there's a race. It took 5 minutes to write the test and 5 seconds for it to find the race. So much for a priori reasoning.

Never mind, my test is wrong.

[gopherbot]

Change https://go.dev/cl/659015 mentions this issue: gopls/internal/util/persistent: add concurrency test