You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@jtsylve, it looks like your problem is probably different, since it doesn't involve finalizers or dead spans, though it might be the same. The check that's failing is quite general.
Can you run and reproduce the problem with GODEBUG=gccheckmark=1? Also, it sounds like you can reproduce it fairly quickly. If that's right, I'd also like to know whether it happens with GODEBUG=gcstoptheworld=1,gcshrinkstackoff=1 and, if that stops it from happening, what happens with GODEBUG=gcstackbarrieroff=1.
Also, is there use of "unsafe" in your code? And is it possible to share the code or reduce it to a shareable test case?
Unfortunately I can't share the majority of the code and it's now a rather complicated codebase, so I'm not sure if I can reduce it to a POC to create the panic, but I'll see what I can do.
Bingo on the "unsafe" call. I'm using a custom Reader that uses zlib as a drop in replacement for compress/zlib (Because of performance). I get no such panic on compress/zlib I can at least share that code. (Although this did work fine on 1.4.2)
// #cgo LDFLAGS: -lz// #include <zlib.h>//// // inflateInit is a macro so we need to do this// int InflateInit(z_streamp s) {// return inflateInit(s);// }import"C"import (
"bytes""errors""io""unsafe"
)
typereaderstruct {
r io.Readerinbuf []bytes C.z_stream
}
// 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.funcNewReader(r io.Reader) (io.ReadCloser, error) {
rd:=&reader{
r: r,
}
err:=C.InflateInit(&rd.s)
iferr!=C.Z_OK {
returnnil, errors.New("Could not init inflate.")
}
returnrd, nil
}
func (r*reader) Read(p []byte) (nint, errerror) {
iflen(p) ==0 {
return0, nil
}
// We can assume that the input will be smaller than// output.maxIn:=int64(len(p))
ifr.inbuf==nil {
buf:=bytes.NewBuffer(make([]byte, 0, maxIn))
nIn, err:=io.CopyN(buf, r.r, maxIn)
iferr!=nil&&err!=io.EOF {
return0, err
}
ifnIn==0&&err==io.EOF {
return0, io.ErrUnexpectedEOF
}
r.s.avail_in=C.uInt(nIn)
r.s.next_in= (*C.Bytef)(unsafe.Pointer(&buf.Bytes()[0]))
} else {
// We still have input from the last callr.s.avail_in=C.uInt(len(r.inbuf))
r.s.next_in= (*C.Bytef)(unsafe.Pointer(&r.inbuf[0]))
}
varreadintvarret C.intforret!=C.Z_STREAM_END {
out:=p[read:]
r.s.avail_out=C.uInt(len(out))
r.s.next_out= (*C.Bytef)(unsafe.Pointer(&out[0]))
ret=C.inflate(&r.s, C.Z_NO_FLUSH)
ifret!=C.Z_STREAM_END&&ret!=C.Z_OK {
return0, errors.New("Could not deflate input.")
}
read=len(out) -int(r.s.avail_out)
ifr.s.avail_out==0 {
break
}
}
ifret==C.Z_STREAM_END {
returnread, io.EOF
}
ifr.s.avail_in==0 {
r.inbuf=nil
} else {
r.inbuf=C.GoBytes(unsafe.Pointer(r.s.next_in), C.int(r.s.avail_in))
}
returnread, nil
}
// Close implements the io.Closer interface//// Calling Close does not close the wrapped io.Reader originally passed to NewReaderfunc (r*reader) Close() error {
err:=C.inflateEnd(&r.s)
iferr!=C.Z_OK {
returnerrors.New("Could not end inflate")
}
returnnil
}
I'm not seeing much different in the outputs of setting those variables, but here they are. Perhaps I'm doing it wrong?
is definitely not safe. As soon as you leave that scope, the backing store of buf will be reachable only through the C heap (via the z_stream_s object malloc'd by inflateInit). Since the GC can't trace through the C heap, it's free to free buf. At that point all bets are off. I'm actually kind of surprised this works in 1.4.2. I think you're just getting lucky.
Depending on where p comes from, the same may be true of
So I fixed the scoping issue. That wasn't the problem that was causing panics; however, I was able to figure out what was.
The C.z_stream structure was being allocated by cgo. zlib then allocates pointers inside of that struct and the Go GC saw them as invalid (since it had no record of the allocations). The solution was to only store a pointer to z_stream sized data so that the GC wouldn't do pointer inspection on it.
I assume this is intended behavior for the GC, but it is a real "gotya" and should be added to that new cgo documentation that was mentioned.
For reference here's the updated (working) code.
diff:
@@ -23,11 +23,16 @@ 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);
// }
+//+// z_streamp new_zstream() {+// return (z_streamp) calloc(sizeof(z_stream), 1);+// }
import "C"
import (
@@ -40,7 +45,7 @@ import (
type reader struct {
r io.Reader
inbuf []byte
- s C.z_stream+ s *C.z_stream
}
// NewReader creates a new ReadCloser. Reads from the returned ReadCloser
@@ -50,9 +55,10 @@ type reader struct {
func NewReader(r io.Reader) (io.ReadCloser, error) {
rd := &reader{
r: r,
+ s: C.new_zstream(),
}
- err := C.InflateInit(&rd.s)+ err := C.InflateInit(rd.s)
if err != C.Z_OK {
return nil, errors.New("Could not init inflate.")
}
@@ -69,8 +75,9 @@ func (r *reader) Read(p []byte) (n int, err error) {
// output.
maxIn := int64(len(p))
+ var buf *bytes.Buffer
if r.inbuf == nil {
- buf := bytes.NewBuffer(make([]byte, 0, maxIn))+ buf = bytes.NewBuffer(make([]byte, 0, maxIn))
nIn, err := io.CopyN(buf, r.r, maxIn)
if err != nil && err != io.EOF {
@@ -96,7 +103,7 @@ func (r *reader) Read(p []byte) (n int, err error) {
r.s.avail_out = C.uInt(len(out))
r.s.next_out = (*C.Bytef)(unsafe.Pointer(&out[0]))
- ret = C.inflate(&r.s, C.Z_NO_FLUSH)+ ret = C.inflate(r.s, C.Z_NO_FLUSH)
if ret != C.Z_STREAM_END && ret != C.Z_OK {
return 0, errors.New("Could not deflate input.")
@@ -126,10 +133,12 @@ func (r *reader) Read(p []byte) (n int, err error) {
//
// Calling Close does not close the wrapped io.Reader originally passed to NewReader
func (r *reader) Close() error {
- err := C.inflateEnd(&r.s)+ err := C.inflateEnd(r.s)
if err != C.Z_OK {
return errors.New("Could not end inflate")
}
+ C.free(unsafe.Pointer(r.s))+
return nil
}
Full code:
// #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);// }//// z_streamp new_zstream() {// return (z_streamp) calloc(sizeof(z_stream), 1);// }import"C"import (
"bytes""errors""io""unsafe"
)
typereaderstruct {
r io.Readerinbuf []bytes*C.z_stream
}
// 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.funcNewReader(r io.Reader) (io.ReadCloser, error) {
rd:=&reader{
r: r,
s: C.new_zstream(),
}
err:=C.InflateInit(rd.s)
iferr!=C.Z_OK {
returnnil, errors.New("Could not init inflate.")
}
returnrd, nil
}
func (r*reader) Read(p []byte) (nint, errerror) {
iflen(p) ==0 {
return0, nil
}
// We can assume that the input will be smaller than// output.maxIn:=int64(len(p))
varbuf*bytes.Bufferifr.inbuf==nil {
buf=bytes.NewBuffer(make([]byte, 0, maxIn))
nIn, err:=io.CopyN(buf, r.r, maxIn)
iferr!=nil&&err!=io.EOF {
return0, err
}
ifnIn==0&&err==io.EOF {
return0, io.ErrUnexpectedEOF
}
r.s.avail_in=C.uInt(nIn)
r.s.next_in= (*C.Bytef)(unsafe.Pointer(&buf.Bytes()[0]))
} else {
// We still have input from the last callr.s.avail_in=C.uInt(len(r.inbuf))
r.s.next_in= (*C.Bytef)(unsafe.Pointer(&r.inbuf[0]))
}
varreadintvarret C.intforret!=C.Z_STREAM_END {
out:=p[read:]
r.s.avail_out=C.uInt(len(out))
r.s.next_out= (*C.Bytef)(unsafe.Pointer(&out[0]))
ret=C.inflate(r.s, C.Z_NO_FLUSH)
ifret!=C.Z_STREAM_END&&ret!=C.Z_OK {
return0, errors.New("Could not deflate input.")
}
read=len(out) -int(r.s.avail_out)
ifr.s.avail_out==0 {
break
}
}
ifret==C.Z_STREAM_END {
returnread, io.EOF
}
ifr.s.avail_in==0 {
r.inbuf=nil
} else {
r.inbuf=C.GoBytes(unsafe.Pointer(r.s.next_in), C.int(r.s.avail_in))
}
returnread, nil
}
// Close implements the io.Closer interface//// Calling Close does not close the wrapped io.Reader originally passed to NewReaderfunc (r*reader) Close() error {
err:=C.inflateEnd(r.s)
iferr!=C.Z_OK {
returnerrors.New("Could not end inflate")
}
C.free(unsafe.Pointer(r.s))
returnnil
}
Sorry, I'm in New Hampshire right now and my Internet barely works, but I'm
trying to keep up.
Joe, I'm afraid your code still has the problem I said. You're storing a
pointer to an object allocated on the Go heap in an object allocated on the
C heap. If you're going to do that, you must keep a reference to that
object from the Go heap for at least as long as it's accessible from C.
Otherwise there's simply no way the garbage collector can know it needs to
keep that object around. In general, it's much better to not do this at all
and to allocate any such pointers from the C heap.
Also, there's nothing wrong with storing a pointer to the C heap in a Go
object. In fact, you're still doing exactly that, you're just storing a
pointer to a pointer now.
BTW, @zeebo, if you have any other panics of this form (or other forms, for that matter), just having more samples would be very helpful for establishing or ruling out patterns.
Initially I wasn't storing a pointer to a C heap, I was allocating the struct directly on the Go heap (C.z_stream vs *C.z_stream) and passing it to C. Since CG had access to the entire struct it was causing panics by the pointers inside that struct that were initialized by zlib. Now I'm just storing a pointer to the C heap, so there are no such errors. This is likely the better way of doing it since none of those pointers need to be garbage collected anyway. Again, I assume that this is intended behavior, but it was non obvious (at least to me) so it might be nice to document it.
As far as the scoping issues, I may be missing something. The buf and p variables that I'm passing pointers to C don't go out of scope until the function returns. At that point libz is finished using them. I suppose those pointers are still stored in the z_stream struct, but they shouldn't be accessed. That being said, since there are no guarantees that a future version of libz won't invalidate this assumption, I suppose I should be storing them in the same scope as the z_stream pointer (In my case inside of the Reader struct).
In my copy of zlib, z_stream is a pointer type and inflateInit mallocs
memory and returns the z_stream pointer to it. Perhaps your version is
different.
And, again, even if it were a struct and it contained pointers initialized
by zlib, that would be perfectly fine because the Go heap is allowed to
point to the C heap.
Sorry, I was sloppy when I said "scope." In fact, the garbage collector is
free to free buf the moment after you last mention it in your code, which
is before the inflate call.
In my copy of zlib, z_stream is a pointer type and inflateInit mallocs
memory and returns the z_stream pointer to it. Perhaps your version is
different.
Indeed it must be (from zlib.h shipped with OS X)
typedefstructz_stream_s {
Bytef*next_in; /* next input byte */uIntavail_in; /* number of bytes available at next_in */uLongtotal_in; /* total nb of input bytes read so far */Bytef*next_out; /* next output byte should be put there */uIntavail_out; /* remaining free space at next_out */uLongtotal_out; /* total nb of bytes output so far */char*msg; /* last error message, NULL if no error */structinternal_stateFAR*state; /* not visible by applications */alloc_funczalloc; /* used to allocate the internal state */free_funczfree; /* used to free the internal state */voidpfopaque; /* private data object passed to zalloc and zfree */intdata_type; /* best guess about the data type: binary or text */uLongadler; /* adler32 value of the uncompressed data */uLongreserved; /* reserved for future use */
} z_stream;
typedefz_streamFAR*z_streamp;
ZEXTERNintZEXPORTinflateInitOF((z_streampstrm));
All the more reason not to make assumptions about library changes.
And, again, even if it were a struct and it contained pointers initialized
by zlib, that would be perfectly fine because the Go heap is allowed to
point to the C heap.
What about pointers to the Go heap that C adjusts? In the above case the next_in pointer is actually adjusted by the call to inflate. So now what the GC would see is a pointer into the middle of a GO allocation that it didn't set. I'm not familiar enough with the GC internals to know if that's a problem, but the panic suggests that it is. (Especially since this fix does prevent the panic)
Sorry, I was sloppy when I said "scope." In fact, the garbage collector is
free to free buf the moment after you last mention it in your code, which
is before the inflate call.
That is something I didn't know about the GC! I wouldn't have assumed that was the case since the reference is still being stored in the variable, but it does make sense.
Something else I just noticed is that some of those pointers in z_stream are actually function pointers (alloc_func & free_func), so they wouldn't likely be pointing at the C heap. The GC may have taken issue with those.
The text was updated successfully, but these errors were encountered:
I suspect that the reason moving the C.z_stream into the C heap fixed the problem is that the C.z_stream contains pointers back into the Go heap (next_in, next_out) that have been advanced beyond the original allocation. In C it is legal to allocate n bytes and then store p+n in a data structure. In Go it is not: p+n may point at the next allocated block in memory - it points outside the original allocation. By moving those pointers into the C heap you hide them from the GC and the GC no longer complains. It is an interesting failure mode for us to consider. (/cc @ianlancetaylor@RLH@dr2chase@aclements)
If your code is now working with the C.z_stream moved out, then I think there's not much more for us to do here. In that case please close this issue. We're going to work on defining exactly what to expect from cgo in Go 1.6, and this is good input for us. Thank you.
If you are still seeing crashes, please let us know. If you just want to move on, you can set GODEBUG=invalidptr=0 in your environment to turn off this diagnostic + crash. But it is probably not a good idea to ignore - it means something is wrong that could break in a more serious way later.
I'm not seeing crashes anymore, and I agree with your suspicion. I suppose this is the GC working as intended and is just another demonstration of how tricky it is to mix a type-safe garbage-collected language with one that is neither. At the very least I hope these "gotyas" are documented in the future. I don't know how to close the issue (since @rsc) is the one who opened it, but I consider my issue resolved.
Split from issue #12138 to separate amd64 report (this one) from arm report (now #12157).
Comment by @jtsylve (2015-08-13 23:53:57)
I'm seeing the same thing on Go 1.5rc1 on Darwin x86_64 with no such error on Go 1.4.2
Comment by @jtsylve (2015-08-13 23:59:55)
Full stack trace (Not sure it'll help without the source)
Comment by @jtsylve (2015-08-14 00:03:31)
This also panics on HEAD, so the bug hasn't been fixed since rc1.
Comment by @aclements (2015-08-14 15:03:24)
@jtsylve, it looks like your problem is probably different, since it doesn't involve finalizers or dead spans, though it might be the same. The check that's failing is quite general.
Can you run and reproduce the problem with GODEBUG=gccheckmark=1? Also, it sounds like you can reproduce it fairly quickly. If that's right, I'd also like to know whether it happens with GODEBUG=gcstoptheworld=1,gcshrinkstackoff=1 and, if that stops it from happening, what happens with GODEBUG=gcstackbarrieroff=1.
Also, is there use of "unsafe" in your code? And is it possible to share the code or reduce it to a shareable test case?
Comment by @jtsylve (2015-08-14 15:39:43)
Unfortunately I can't share the majority of the code and it's now a rather complicated codebase, so I'm not sure if I can reduce it to a POC to create the panic, but I'll see what I can do.
Bingo on the "unsafe" call. I'm using a custom
Reader
that uses zlib as a drop in replacement forcompress/zlib
(Because of performance). I get no such panic oncompress/zlib
I can at least share that code. (Although this did work fine on 1.4.2)I'm not seeing much different in the outputs of setting those variables, but here they are. Perhaps I'm doing it wrong?
GODEBUG=gccheckmark=1 go run ntfstest.go
GODEBUG=gcstoptheworld=1,gcshrinkstackoff=1 go run ntfstest.go
Comment by @aclements (2015-08-14 15:56:14)
Thanks for the quick reply, @jtsylve.
is definitely not safe. As soon as you leave that scope, the backing store of buf will be reachable only through the C heap (via the z_stream_s object malloc'd by inflateInit). Since the GC can't trace through the C heap, it's free to free buf. At that point all bets are off. I'm actually kind of surprised this works in 1.4.2. I think you're just getting lucky.
Depending on where p comes from, the same may be true of
Comment by @jtsylve (2015-08-14 18:29:20)
How can I achieve a similar result that is considered "safe" unsafe code?
Comment by @jtsylve (2015-08-15 01:20:52)
So I fixed the scoping issue. That wasn't the problem that was causing panics; however, I was able to figure out what was.
The
C.z_stream
structure was being allocated by cgo.zlib
then allocates pointers inside of that struct and the Go GC saw them as invalid (since it had no record of the allocations). The solution was to only store a pointer toz_stream
sized data so that the GC wouldn't do pointer inspection on it.I assume this is intended behavior for the GC, but it is a real "gotya" and should be added to that new cgo documentation that was mentioned.
For reference here's the updated (working) code.
diff:
Full code:
Comment by @aclements (2015-08-15 02:29:34)
Sorry, I'm in New Hampshire right now and my Internet barely works, but I'm
trying to keep up.
Joe, I'm afraid your code still has the problem I said. You're storing a
pointer to an object allocated on the Go heap in an object allocated on the
C heap. If you're going to do that, you must keep a reference to that
object from the Go heap for at least as long as it's accessible from C.
Otherwise there's simply no way the garbage collector can know it needs to
keep that object around. In general, it's much better to not do this at all
and to allocate any such pointers from the C heap.
Also, there's nothing wrong with storing a pointer to the C heap in a Go
object. In fact, you're still doing exactly that, you're just storing a
pointer to a pointer now.
Comment by @aclements (2015-08-15 02:36:01)
BTW, @zeebo, if you have any other panics of this form (or other forms, for that matter), just having more samples would be very helpful for establishing or ruling out patterns.
Comment by @jtsylve (2015-08-15 02:40:03)
Initially I wasn't storing a pointer to a C heap, I was allocating the struct directly on the Go heap (
C.z_stream
vs*C.z_stream
) and passing it to C. Since CG had access to the entire struct it was causing panics by the pointers inside that struct that were initialized byzlib
. Now I'm just storing a pointer to the C heap, so there are no such errors. This is likely the better way of doing it since none of those pointers need to be garbage collected anyway. Again, I assume that this is intended behavior, but it was non obvious (at least to me) so it might be nice to document it.As far as the scoping issues, I may be missing something. The
buf
andp
variables that I'm passing pointers to C don't go out of scope until the function returns. At that pointlibz
is finished using them. I suppose those pointers are still stored in thez_stream
struct, but they shouldn't be accessed. That being said, since there are no guarantees that a future version oflibz
won't invalidate this assumption, I suppose I should be storing them in the same scope as thez_stream
pointer (In my case inside of theReader
struct).Comment by @aclements (2015-08-15 03:04:43)
In my copy of zlib, z_stream is a pointer type and inflateInit mallocs
memory and returns the z_stream pointer to it. Perhaps your version is
different.
And, again, even if it were a struct and it contained pointers initialized
by zlib, that would be perfectly fine because the Go heap is allowed to
point to the C heap.
Sorry, I was sloppy when I said "scope." In fact, the garbage collector is
free to free buf the moment after you last mention it in your code, which
is before the inflate call.
Comment by @jtsylve (2015-08-15 03:29:35)
Indeed it must be (from zlib.h shipped with OS X)
All the more reason not to make assumptions about library changes.
What about pointers to the Go heap that C adjusts? In the above case the
next_in
pointer is actually adjusted by the call toinflate
. So now what the GC would see is a pointer into the middle of a GO allocation that it didn't set. I'm not familiar enough with the GC internals to know if that's a problem, but the panic suggests that it is. (Especially since this fix does prevent the panic)That is something I didn't know about the GC! I wouldn't have assumed that was the case since the reference is still being stored in the variable, but it does make sense.
Comment by @jtsylve (2015-08-15 03:40:11)
Something else I just noticed is that some of those pointers in
z_stream
are actually function pointers (alloc_func
&free_func
), so they wouldn't likely be pointing at the C heap. The GC may have taken issue with those.The text was updated successfully, but these errors were encountered: