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: minimally align variables in memory when -d=checkptr is used #35128

Open
mdempsky opened this issue Oct 24, 2019 · 3 comments
Open

cmd/compile: minimally align variables in memory when -d=checkptr is used #35128

mdempsky opened this issue Oct 24, 2019 · 3 comments

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 24, 2019

-d=checkptr should report a warning for:

var x [4]byte
p := (*uint32)(unsafe.Pointer(&x[0]))

In general, this is unsafe because x only requires 1-byte alignment, but *uint32 requires 4-byte alignment.

Currently, this isn't reliably reported, because efficiently allocating variables often results in over alignment. But when -d=checkptr is in use, cmd/compile and the runtime could burn a few bytes of memory to intentionally minimally align things in the heap and on the stack (e.g., 1-byte-alignment variables always on an odd address).

Relatedly, when the runtime allocates a chunk of memory for a heap variable allocation, it could align the variable as close to the end of the allocation (rather than at the beginning), so that we can more easily detect pointer arithmetic overflows (at the expense of pointer arithmetic underflows, which seem less common).

These both would have helped catch issues that I've noticed in CLs to address errors reported by -d=checkptr.

(Prior art: I think OpenBSD's malloc used to prefer aligning allocations towards the end of an mmap'ed page so that buffer overflows were more likely to trigger a segmentation fault. Not sure if it still does that. I don't see it mentioned specifically in the man page.)

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Oct 24, 2019

Hrm:

go/src/runtime/malloc.go

Lines 889 to 904 in 68981bf

// TODO(austin): This should be just
// align = uintptr(typ.align)
// but that's only 4 on 32-bit platforms,
// even if there's a uint64 field in typ (see #599).
// This causes 64-bit atomic accesses to panic.
// Hence, we use stricter alignment that matches
// the normal allocator better.
if size&7 == 0 {
align = 8
} else if size&3 == 0 {
align = 4
} else if size&1 == 0 {
align = 2
} else {
align = 1
}

@mdempsky mdempsky modified the milestones: Go1.14, Backlog Nov 12, 2019
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Nov 12, 2019

Bumping to Backlog, since I don't think this is going to happen for Go 1.14.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2020

Change https://golang.org/cl/222855 mentions this issue: sha3: mark xorInUnaligned with go:nocheckptr

gopherbot pushed a commit to golang/crypto that referenced this issue Mar 11, 2020
It is unclear whether unaligned reads should be allowed, or if they
are even actually a good idea here. However, while we figure that out,
we should un-break 'go test -race' for users of this package.

Updates golang/go#37644
Updates golang/go#37298
Updates golang/go#37715
Updates golang/go#34972
Updates golang/go#35128

Change-Id: I088f5703023e4f05ee274a6753e925973f12ac1b
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/222855
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
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.