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

runtime: checkptrAlignment crash on nil pointer #47430

Closed
mdempsky opened this issue Jul 28, 2021 · 7 comments
Closed

runtime: checkptrAlignment crash on nil pointer #47430

mdempsky opened this issue Jul 28, 2021 · 7 comments
Assignees
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 28, 2021

With Go 1.17rc1 (and probably since 4bb0847), this program crashes when run with checkptr enabled (e.g., with -race):

package main

import (
	"runtime"
	"time"
	"unsafe"
)

func main() {
	go func() {
		for {
			runtime.GC()
		}
	}()

	go func() {
		for i := 0; ; i++ {
			recurse(i % 1024)
		}
	}()

	time.Sleep(time.Second)
}

func recurse(n int) {
	// Inflate the stack so runtime.shrinkstack gets called during GC
	if n > 0 {
		recurse(n - 1)
	}

	var p unsafe.Pointer
	_ = (*int)(p)
}

Easy fix is adding an explicit nil check at the start of checkptrAlignment.

@mdempsky mdempsky added this to the Go1.17 milestone Jul 28, 2021
@mdempsky mdempsky self-assigned this Jul 28, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 28, 2021

Change https://golang.org/cl/338029 mentions this issue: runtime: don't crash on nil pointers in checkptrAlignment

Loading

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jul 28, 2021

@golang/release Do I need to do anything to make sure b39e0f4 is included in Go 1.17?

It's a runtime bug that causes valid programs to sporadically fail, albeit only when checkptr is enabled (e.g., when the race detector or msan are used).

Loading

@mdempsky mdempsky reopened this Jul 28, 2021
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Jul 28, 2021

@mdempsky It's marked as a release blocker which means Go 1.17 will not be released until the issue is resolved. This will probably block a second release candidate as well.

Loading

@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Jul 28, 2021

I realize that I didn't answer the question explicitly. The @golang/release will ensure it is included in the Go 1.17 release.

Loading

@toothrot
Copy link
Contributor

@toothrot toothrot commented Jul 28, 2021

@mdempsky Is there more work to do on this issue, or is it resolved by the linked CL?

Loading

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jul 28, 2021

@cagedmantis Thanks!

@toothrot The issue is fixed by b39e0f4. I just reopened for discussion purposes to make sure it wasn't missed. I think we can close it if there's no procedural reason to keep it open any more.

Loading

@toothrot
Copy link
Contributor

@toothrot toothrot commented Jul 28, 2021

@mdempsky We've got it, thanks!

Loading

@toothrot toothrot closed this Jul 28, 2021
steeve added a commit to znly/go that referenced this issue Aug 19, 2021
Ironically, checkptrAlignment had a latent case of bad pointer
arithmetic: if ptr is nil, then `add(ptr, size-1)` might produce an
illegal pointer value.

The fix is to simply check for nil at the top of checkptrAlignment,
and short-circuit if so.

This CL also adds a more explicit bounds check in checkptrStraddles,
rather than relying on `add(ptr, size-1)` to wrap around. I don't
think this is necessary today, but it seems prudent to be careful.

Fixes golang#47430.

Change-Id: I5c50b2f7f41415dbebbd803e1b8e7766ca95e1fd
Reviewed-on: https://go-review.googlesource.com/c/go/+/338029
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
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
4 participants