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.

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

cmd/compile: nil check not scheduled correctly #42673

Open
randall77 opened this issue Nov 17, 2020 · 3 comments
Open

cmd/compile: nil check not scheduled correctly #42673

randall77 opened this issue Nov 17, 2020 · 3 comments
Milestone

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Nov 17, 2020

func f(p **int) int {
	return **p
}

compiles to

		MOVQ	"".p+8(SP), AX
		MOVQ	(AX), CX
		TESTB	AL, (AX)
		MOVQ	(CX), AX
		MOVQ	AX, "".~r1+16(SP)
		RET

Note the TESTB which is a nil check. It is redundant with the previous load, which would have faulted if AX is nil.

There are 2 nil checks in this code, but only one is removed (technically, not removed but merged with a load).
Looking at the code, I think it is because the two nil checks get scheduled out of order, so the elimination code gets confused. The elimination code should be more robust to ordering.

@randall77 randall77 added this to the Unplanned milestone Nov 17, 2020
@randall77
Copy link
Contributor Author

@randall77 randall77 commented Nov 17, 2020

This is actually more than a performance bug. The nil checks are not getting scheduled correctly.

func f(p *[1e6]*int) int {
	return *p[1e4]
}

This compiles to

		MOVQ	"".p+8(SP), AX
		MOVQ	80000(AX), CX
		TESTB	AL, (AX)
		MOVQ	(CX), AX
		MOVQ	AX, "".~r1+16(SP)
		RET

The read is happening before its corresponding nil check. The nil check elim pass is doing the right thing. It is the scheduling pass which is wrong.

I don't see an obvious way to defeat type safety with this bug, but it makes me nervous. We should fix it for 1.16.

@randall77 randall77 removed the Performance label Nov 17, 2020
@randall77 randall77 modified the milestones: Unplanned, Go1.16 Nov 17, 2020
@randall77 randall77 changed the title cmd/compile: nil check not eliminated cmd/compile: nil check not scheduled correctly Nov 17, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 17, 2020

Change https://golang.org/cl/270940 mentions this issue: cmd/compile: improve scheduling pass

@randall77
Copy link
Contributor Author

@randall77 randall77 commented Nov 19, 2020

This is looking trickier than first appears (see CL). I'm going to punt to 1.17.

I think the worst thing that can happen here is that we can read arbitrary parts of memory before a nil fault. In those cases it was going to fault anyway, so maybe we get a SIGBUS instead of a SIGSEGV. That's about as bad as it gets. Perhaps memory-mapped device drivers might treat reads as having side-effects, but that seems like a tiny attack surface.

Possibly also there's some spectre-ish vulnerability here, but without the speculation, if a chain of nilptr-delayed reads can be constructed. I think it would involve some very strange gadgets that would have to be in a binary, though, so it appears unlikely to matter.

@randall77 randall77 modified the milestones: Go1.16, Go1.17 Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.