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: incorrect sanity checks in the page allocator #38130

Closed
rasky opened this issue Mar 28, 2020 · 4 comments
Closed

runtime: incorrect sanity checks in the page allocator #38130

rasky opened this issue Mar 28, 2020 · 4 comments
Assignees
Milestone

Comments

@rasky
Copy link
Member

@rasky rasky commented Mar 28, 2020

While investigating the optimizations performed by my prove CL 196679, I found out these two instances of what looks like dead code:

j, searchIdx := s.chunkOf(ci).find(npages, 0)
if j < 0 {
// We couldn't find any space in this chunk despite the summaries telling
// us it should be there. There's likely a bug, so dump some state and throw.
sum := s.summary[len(s.summary)-1][i]
print("runtime: summary[", len(s.summary)-1, "][", i, "] = (", sum.start(), ", ", sum.max(), ", ", sum.end(), ")\n")
print("runtime: npages = ", npages, "\n")
throw("bad summary data")
}

j, searchIdx := s.chunkOf(i).find(npages, chunkPageIndex(s.searchAddr))
if j < 0 {
print("runtime: max = ", max, ", npages = ", npages, "\n")
print("runtime: searchIdx = ", chunkPageIndex(s.searchAddr), ", s.searchAddr = ", hex(s.searchAddr), "\n")
throw("bad summary data")
}

That branch is never taken because pallocBits.find() return an unsigned number:

// find searches for npages contiguous free pages in pallocBits and returns
// the index where that run starts, as well as the index of the first free page
// it found in the search. searchIdx represents the first known free page and
// where to begin the search from.
//
// If find fails to find any free space, it returns an index of ^uint(0) and
// the new searchIdx should be ignored.
//
// Note that if npages == 1, the two returned values will always be identical.
func (b *pallocBits) find(npages uintptr, searchIdx uint) (uint, uint) {

Given the documentation, it looks like the failure is reported as ^uint(0) so the above checks are probably wrong.

/cc @aclements @mknyszek

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Mar 28, 2020

Ah! Thank you. I'll fix that right away.

Earlier on in the review process the return value was indeed an int, but this was used as a shift in multiple places, so I changed the whole pallocBits API to return uints.

Luckily these are just sanity checks, but if there is a bug in the page allocator, it could lead to bad pointers being returned (and possibly memory corruption).

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 28, 2020

Change https://golang.org/cl/226297 mentions this issue: runtime: check the sanity condition in the page allocator

@mknyszek
Copy link
Contributor

@mknyszek mknyszek commented Mar 28, 2020

Uploaded a fix. We may want to backport this. Like I said before, if there's a bug in the page allocator that would cause these sanity checks to fail, it would manifest as memory corruption as opposed to the immediate failure it was supposed to be.

CC @bcmills who has been tracking a bunch of the memory corruption issues.
CC @randall77 @cherrymui who have been looking into some of the memory corruption issues.

@mknyszek mknyszek self-assigned this Mar 28, 2020
@mknyszek mknyszek added this to the Go1.15 milestone Mar 28, 2020
@mknyszek mknyszek changed the title runtime: dead code in mpagealloc.go? runtime: incorrect sanity checks in the page allocator Mar 28, 2020
@rasky
Copy link
Member Author

@rasky rasky commented Mar 28, 2020

I wonder whether this should become a vet check. I think with vet, we can at least find very simple instance (eg: unsigned number < 0); prove is able to find similar always-true or always-false conditions in more complicated situations which vet wouldn't be able to; but prove of course cannot emit a compile-time error.

Maybe this should go into what @dr2chase was doing, that is a log of "important information" written by the compiler during compilation.

@gopherbot gopherbot closed this in 89e13c8 Mar 30, 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
3 participants
You can’t perform that action at this time.