cmd/cgo: bad pointer detected in private zlib wrapper #13928
Labels
Milestone
Comments
|
Oh it may also be worth mentioning that I have my code compiled to a c-shared library a linked to a larger OS X application. |
|
Please run your program with the environment variable GOTRACEBACK=2 and show us the complete stack trace. |
|
|
Setting |
|
The error means that Go sees a pointer into the Go heap that the Go GC is not aware of. This is a serious error that you shouldn't cover up by setting GODEBUG=cgocheck. Or there is a bug in the checking code, of course, although it seems pretty simple. You could create an error of this sort by calling the C function Could you try running your program with GODEBUG=cgocheck=2 to see if the stricter checks find an earlier problem? Is there a way we can try running this program ourselves to recreate the problem? |
|
Although I understand you may not want to share the source for your whole app, if you can post your full zlib package (I assume just zlib.go), I suspect we'll be able to identify pretty quickly what is wrong (whether in that code or in the cgo checks) and how to fix it. Thanks. |
rsc
changed the title
|
I believe it's almost certainly a problem with the check. This very stripped down version causes the same panic: // Package zlib implements zlib compression using libz, which should
// be faster than the Go implementation
package zlib
// #cgo LDFLAGS: -lz
// #include <zlib.h>
// #include <stdlib.h>
//
// // inflateInit is a macro so we need to do this
// int InflateInit(z_streamp s) {
// return inflateInit(s);
// }
//
// // We do the zstream allocation in C, so that the Go
// // GC doesn't inspect the pointers inside that are
// // allocated by zlib and panic.
// z_streamp new_zstream() {
// return (z_streamp) calloc(sizeof(z_stream), 1);
// }
import "C"
import (
"errors"
"io"
"unsafe"
)
type reader struct {
s C.z_streamp
}
// NewReader creates a new ReadCloser. Reads from the returned ReadCloser
// read and decompress data from r. The implementation buffers input and
// may read more data than necessary from r. It is the caller's
// responsibility to call Close on the ReadCloser when done.
func NewReader(r io.Reader) (io.ReadCloser, error) {
rd := &reader{
// This allocates memory using C.calloc, so it's not go memory
s: C.new_zstream(),
}
err := C.InflateInit(rd.s) // <-- Occasional panic here
if err != C.Z_OK {
return nil, errors.New("Could not init inflate.")
}
return rd, nil
}
func (r *reader) Read(p []byte) (n int, err error) {
// Do nothing to show we're not putting any Go pointers
// in C memory.
return len(p), nil
}
// Close implements the io.Closer interface
//
// Calling Close does not close the wrapped io.Reader originally passed to NewReader
func (r *reader) Close() error {
err := C.inflateEnd(r.s)
if err != C.Z_OK {
return errors.New("Could not end inflate")
}
C.free(unsafe.Pointer(r.s))
r.s = nil
return nil
}In this case |
|
It doesn't like that line because it violates the rule that you can't store
Go pointers into non-Go memory indefinitely. There is an exception for
doing that during an individual call into C, but they must be passed into
the call and eliminated from the C structures before returning.
I don't see why that line would cause the problem in the original report,
though.
Even so to get your code past cgocheck=2 the right approach is to write a C
wrapper that takes the pointer to the buffers, does its work, forgets the
pointers, and returns.
Kernighan and Donovan's The Go Programming Language has a good example of
wrapping a bzip2 writer. Perhaps that can help illustrate the pattern. Even
if you don't have the book you can find the code at 'go get
gopl.io/ch13/bzip'. It's writing, not reading, but it should be roughly
equivalent. The key part is the C function bz2compress: Go pointers enter C
as arguments, but they don't stay once it returns.
Russ
|
|
Russ, I deleted those comments because I realized that was an issue. I nulled out the references before the function returned and it still presented the same issue. See the new stripped down version of the code that still has the panic. |
|
I tried your new program but I can't make it crash on darwin/386. This is the standalone version I am running: |
|
Yeah, that's the thing, it only crashes under very specific conditions in my application. If my application is in a certain state, it crashes every time consistently. In other states it doesn't crash at all. I believe it's highly dependant on the memory layout. |
|
What is the concrete type of the io.Reader you are passing to zlib.NewReader? Is that inner Reader allocated from Go or from C? |
|
Never mind, I see the problem. |
|
@rsc Don't leave me hanging |
|
Yup, well removing that panic would definitely fix it for me. |
|
CL https://golang.org/cl/18611 mentions this issue. |
|
@jtsylve Sorry I expected Gopherbot to mention the CL more quickly than it did. |
|
NP. It seems to only mention the CL if someone posts the mention in a comment on the CL, not when the CL is posted. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
I'm running tip on darwin/386 and I'm seeing an occasional panic.
I've got a wrapper for zlib that's quite a bit faster than the zlib reader in the standard library. Under specific conditions in my application I get a
panic: runtime error: cgo argument has invalid Go pointer.My code is similar to the following:
Sometimes (not every time) when
NewReaderis called a panic is triggered on the call toC.InflateInit. From what I understand this panic is only ever triggered bycgoCheckUnknownPointerif the pointer exists on the Go heap. The pointer is returned by a C call tocalloc, so should not exist on the Go heap, it should be C controlled memory.Is this a bug or am I doing something wrong?
The text was updated successfully, but these errors were encountered: