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: writebarrier inheap check is racy #11324

Closed
dvyukov opened this issue Jun 22, 2015 · 3 comments
Closed

runtime: writebarrier inheap check is racy #11324

dvyukov opened this issue Jun 22, 2015 · 3 comments
Milestone

Comments

@dvyukov
Copy link
Member

dvyukov commented Jun 22, 2015

In heap does the following:

func inheap(b uintptr) bool {
    if b == 0 || b < mheap_.arena_start || b >= mheap_.arena_used {
        return false
    }
    ...
    s := h_spans[x]
    ...
}

while mHeap_SysAlloc does the following:

func mHeap_SysAlloc(h *mheap, n uintptr) unsafe.Pointer {
                h.arena_used = p + (-uintptr(p) & (_PageSize - 1))
...
        mHeap_MapSpans(h, p+n)
}

Now consider that there is a non-Go-heap mmap in the heap area. It is possible that mHeap_SysAlloc updates h.arena_used to include the non-Go-heap region, however h_spans are not yet mapped for that region. Now another goroutine invokes writebarrier for a pointer in non-Go-heap region. The writebarrier invokes inheap, inheap checks that the pointer value is within [arena_start, mheap_.arena_used) and accesses h_spans for the pointer. However, h_spans is not yet mapped for the region. Bang!

I believe that's what happens on the windows builders:
http://build.golang.org/log/85ba224f0fe7b0d2d4c5e63ca6f7643131b0dba3
http://build.golang.org/log/6242a4b521f0e5bcce48db07a8eb51d3b874b7c7
http://build.golang.org/log/20d249a8596c79b11043f704fda3aa404e1808b3
http://build.golang.org/log/16e89685e920d8af90b39a1f55e692650b07c121
http://build.golang.org/log/0b17582865258804b41b8157bb5dd09a245f2bc9

/cc @aclements @rsc

@dvyukov dvyukov added this to the Go1.5 milestone Jun 22, 2015
@aclements
Copy link
Member

aclements commented Jun 22, 2015 via email

@aclements
Copy link
Member

No thanks to GitHub's issue search, I finally found the corresponding FreeBSD issue: #10212.

I went through all of the updates to arena_used (there aren't many) and it looks like only this one slipped through the cracks.

aclements added a commit that referenced this issue Jun 22, 2015
In order to avoid a race with a concurrent write barrier or garbage
collector thread, any update to arena_used must be preceded by mapping
the corresponding heap bitmap and spans array memory. Otherwise, the
concurrent access may observe that a pointer falls within the heap
arena, but then attempt to access unmapped memory to look up its span
or heap bits.

Commit d57c889 fixed all of the places where we updated arena_used
immediately before mapping the heap bitmap and spans, but it missed
the one place where we update arena_used and depend on later code to
update it again and map the bitmap and spans. This creates a window
where the original race can still happen. This commit fixes this by
mapping the heap bitmap and spans before this arena_used update as
well. This code path is only taken when expanding the heap reservation
on 32-bit over a hole in the address space, so these extra mmap calls
should have negligible impact.

Fixes #10212, #11324.

Change-Id: Id67795e6c7563eb551873bc401e5cc997aaa2bd8
Reviewed-on: https://go-review.googlesource.com/11340
Run-TryBot: Austin Clements <[email protected]>
Reviewed-by: Rick Hudson <[email protected]>
Reviewed-by: Dmitry Vyukov <[email protected]>
@aclements
Copy link
Member

I meant to close this from the commit, but apparently didn't get the syntax quite right.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
@rsc rsc unassigned RLH Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants