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: "found pointer to free object" on Windows #44900

Closed
bcmills opened this issue Mar 10, 2021 · 15 comments
Closed

runtime: "found pointer to free object" on Windows #44900

bcmills opened this issue Mar 10, 2021 · 15 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 10, 2021

This came to my attention via a windows-amd64-longtest failure in a TryBot (https://storage.googleapis.com/go-build-log/ef7da83e/windows-amd64-longtest_1a18709a.log).

2021-03-09T20:35:41-48ddf70/windows-amd64-longtest
2021-03-09T18:35:29-48895d0/windows-amd64-longtest
2021-03-05T18:47:09-c082f9f/windows-amd64-longtest

The symptom resembles #44614 (@randall77 @mdempsky), but that was marked fixed before the first known Windows failure occurred.

CC @mknyszek @aclements @jeremyfaller

@bcmills
Copy link
Member Author

@bcmills bcmills commented Mar 10, 2021

Marking as release-blocker at least until we understand whether this is a regression.

@bcmills bcmills changed the title runtime: occasional failures with "found pointer to free object" on Windows runtime: "found pointer to free object" on Windows Mar 10, 2021
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 10, 2021

Could this be related to golang.org/cl/297389? All of those failure commits were after that CL was merged. /cc @zx2c4

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Mar 10, 2021

https://build.golang.org/log/dcb6544b3a999b160ede916b98d49fad0d55465a

Plan 9 isn't running any Windows code, so I don't think it's related.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 10, 2021

Fair point. Though there could also be separate issues affecting different platforms. The plan9-arm failures in #44903 date back to mid 2020, whereas the windows failures are all more recent. That makes me think there must have been something that affected Windows recently. E.g., I'm not saying that CL is incorrect, but maybe it's now tickling an underlying compiler/runtime issue that's been affecting plan9-arm and solaris too.

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Mar 10, 2021

Hmm... could be. Something like this should mirror minimally what's going on with defers and unsafe.Pointer and the GC in that commit:

package main

import (
	"unsafe"
)

func main() {
	a := make([]unsafe.Pointer, 2)
	defer func() { println(a[1]) }()
	a[0] = unsafe.Pointer(&struct{ _ int }{})
	a[1] = unsafe.Pointer(&struct{ _ int }{})
	println(a[0])
}
@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Mar 10, 2021

runtime: marked free object in span 0x2dff28935b0, elemsize=24 freeindex=32 (bad use of unsafe.Pointer? try -d=checkptr)

These all say elemsize=32. Does that mean it's the result of an allocation of structs size 24 in a list? That seems to be the case, because the code goes onto say:

        for i := uintptr(0); i < s.nelems; i++ {
                addr := s.base() + i*s.elemsize

So in that case, what struct of size 24 is being allocated in a list? It can't be those unsafe.Pointers, where elemsize=8.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 10, 2021

I think you meant they all say elemsize=24?

If so, yeah, the error message is that there's an object that was allocated on heap of size 24 bytes. Also, that GC previously ran and didn't find any references to the object and marked it dead; but a later run found a pointer to it again.

This could happen if a user converts a pointer to uintptr and then back to unsafe.Pointer in an unsafe way (hence the checkptr recommendation in the error message). But of course the compiler/runtime could be getting something wrong too.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 10, 2021

If I understand correctly, the calls to updateProcThreadAttribute are making Windows (i.e., non-Go code) store pointers into the []unsafe.Pointer slice allocated in newProcThreadAttributeList?

If so, I suspect that's the issue then. It's not enough for the heap bits to be allocated as pointer memory. Mutators also need to know to use write barriers when storing pointers into memory during certain GC phases. Windows wouldn't know to do that.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 10, 2021

I also suspect the 24-byte object that's being lost and found again is the fd slice allocated in StartProcess.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Mar 10, 2021

If I understand correctly, the calls to updateProcThreadAttribute are making Windows (i.e., non-Go code) store pointers into the []unsafe.Pointer slice allocated in newProcThreadAttributeList?

updateProcThreadAttribute is an Windows API that does not document it's implementation

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute

The doco says Updates the specified attribute in a list of attributes for process and thread creation.. It does not says that it stores pointers to our objects in that memory. Jason disassembled the UpdateProcThreadAttribute to discover how the function is implemented.

I am still against submitted CL

https://go-review.googlesource.com/c/go/+/297389/

because it is too complicated.

All we need to do here is to make sure that fd slice lives until the end of StartProcess.

So something like what I suggested at https://go-review.googlesource.com/c/go/+/297391/1/src/syscall/exec_windows.go#253 should work.

Alex

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Mar 10, 2021

If I understand correctly, the calls to updateProcThreadAttribute are making Windows (i.e., non-Go code) store pointers into the []unsafe.Pointer slice allocated in newProcThreadAttributeList?

If so, I suspect that's the issue then. It's not enough for the heap bits to be allocated as pointer memory. Mutators also need to know to use write barriers when storing pointers into memory during certain GC phases. Windows wouldn't know to do that.

Ahh, I wasn't aware of that (and apparently nobody mentioned that on the rather lengthy CL). I'll send two new CLs to fix this the straight forward way for go/src/syscall and a more usable way for x/sys/windows.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 10, 2021

Change https://golang.org/cl/300349 mentions this issue: syscall: use runtime.KeepAlive for ProcThreadAttributeList arguments

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 10, 2021

Change https://golang.org/cl/300369 mentions this issue: windows: use Go-managed pointer list for ProcThreadAttributeList

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Mar 10, 2021

Those two CLs should address the matter.

@gopherbot gopherbot closed this in b8e9ec8 Mar 11, 2021
@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented May 12, 2021

This should actually still be open as https://golang.org/cl/300369 hasn't been merged yet.

gopherbot pushed a commit to golang/sys that referenced this issue May 21, 2021
It turns out that if you write Go pointers to Go memory, the Go compiler
must be involved so that it generates various calls to the GC in the
process. Letting Windows write Go pointers to Go memory violated this.

We fix this by having all the Windows-managed memory be just a boring
[]byte blob. Then, in order to prevent the GC from prematurely cleaning
up the pointers referenced by that []byte blob, or in the future moving
memory and attempting to fix up pointers, we copy the data to the
Windows heap and then maintain a little array of pointers that have been
used. Every time the Update function is called with a new pointer, we
make a copy and append it to the list.  Then, on Delete, we free the
pointers from the Windows heap.

Updates golang/go#44900.

Change-Id: I42340a93fd9f6b8d10340634cf833fd4559a5f4f
Reviewed-on: https://go-review.googlesource.com/c/sys/+/300369
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@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
5 participants