Skip to content

Commit

Permalink
Fix "Go pointer to Go pointer" panics
Browse files Browse the repository at this point in the history
The Cgo runtime verifies that Go never passes pointers to other Go
pointers, which is required for correct garbage collection.
Unfortunately, its checks are not perfect, and there are occasional
false positives. Our code triggers these false positives if the
slice passed to compression functions is in the same memory
allocation as Go pointers. This happened when trying to use zstd with
another package's Writer type, which has an internal buffer.

The tests added in this PR all fail with the following panic. The
fix is to ensure the expression unsafe.Pointer(&src[0]) is inside the
Cgo call, and not before. This is documented in the following issue:

golang/go#14210 (comment)

The remaining uses of the "var srcPtr *byte" pattern are safe: they
all pass the address of a byte slice that is allocated internally by
this library, so I believe they cannot cause this error.

Fixes the following panic:

panic: runtime error: cgo argument has Go pointer to Go pointer

goroutine 30 [running]:
panic(...)
  /usr/local/go/src/runtime/panic.go:969 +0x1b9
github.com/DataDog/zstd.(*ctx).CompressLevel.func1(...)
  /home/circleci/project/zstd_ctx.go:75 +0xd9
github.com/DataDog/zstd.(*ctx).CompressLevel(...)
  /home/circleci/project/zstd_ctx.go:75 +0xce
github.com/DataDog/zstd.TestCtxCompressLevelNoGoPointers.func1(...)
  /home/circleci/project/zstd_ctx_test.go:71 +0x77
github.com/DataDog/zstd.testCompressNoGoPointers(...)
  /home/circleci/project/zstd_test.go:130 +0xad
github.com/DataDog/zstd.TestCtxCompressLevelNoGoPointers(...)
  /home/circleci/project/zstd_ctx_test.go:69 +0x37
testing.tRunner(...)
  /usr/local/go/src/testing/testing.go:1123 +0xef
  • Loading branch information
evanj committed Mar 5, 2021
1 parent 12a1eb7 commit 0ead11a
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 27 deletions.
28 changes: 18 additions & 10 deletions zstd.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,26 @@ func CompressLevel(dst, src []byte, level int) ([]byte, error) {
dst = make([]byte, bound)
}

var srcPtr *byte // Do not point anywhere, if src is empty
if len(src) > 0 {
srcPtr = &src[0]
// We need unsafe.Pointer(&src[0]) in the Cgo call to avoid "Go pointer to Go pointer" panics.
// This means we need to special case empty input. See:
// https://github.com/golang/go/issues/14210#issuecomment-346402945
var cWritten C.size_t
if len(src) == 0 {
cWritten = C.ZSTD_compress(
unsafe.Pointer(&dst[0]),
C.size_t(len(dst)),
unsafe.Pointer(nil),
C.size_t(0),
C.int(level))
} else {
cWritten = C.ZSTD_compress(
unsafe.Pointer(&dst[0]),
C.size_t(len(dst)),
unsafe.Pointer(&src[0]),
C.size_t(len(src)),
C.int(level))
}

cWritten := C.ZSTD_compress(
unsafe.Pointer(&dst[0]),
C.size_t(len(dst)),
unsafe.Pointer(srcPtr),
C.size_t(len(src)),
C.int(level))

written := int(cWritten)
// Check if the return is an Error code
if err := getError(written); err != nil {
Expand Down
31 changes: 20 additions & 11 deletions zstd_ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,28 @@ func (c *ctx) CompressLevel(dst, src []byte, level int) ([]byte, error) {
dst = make([]byte, bound)
}

var srcPtr *byte // Do not point anywhere, if src is empty
if len(src) > 0 {
srcPtr = &src[0]
// We need unsafe.Pointer(&src[0]) in the Cgo call to avoid "Go pointer to Go pointer" panics.
// This means we need to special case empty input. See:
// https://github.com/golang/go/issues/14210#issuecomment-346402945
var cWritten C.size_t
if len(src) == 0 {
cWritten = C.ZSTD_compressCCtx(
c.cctx,
unsafe.Pointer(&dst[0]),
C.size_t(len(dst)),
unsafe.Pointer(nil),
C.size_t(0),
C.int(level))
} else {
cWritten = C.ZSTD_compressCCtx(
c.cctx,
unsafe.Pointer(&dst[0]),
C.size_t(len(dst)),
unsafe.Pointer(&src[0]),
C.size_t(len(src)),
C.int(level))
}

cWritten := C.ZSTD_compressCCtx(
c.cctx,
unsafe.Pointer(&dst[0]),
C.size_t(len(dst)),
unsafe.Pointer(srcPtr),
C.size_t(len(src)),
C.int(level))

written := int(cWritten)
// Check if the return is an Error code
if err := getError(written); err != nil {
Expand Down
7 changes: 7 additions & 0 deletions zstd_ctx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,13 @@ func TestCtxCompressLevel(t *testing.T) {
}
}

func TestCtxCompressLevelNoGoPointers(t *testing.T) {
testCompressNoGoPointers(t, func(input []byte) ([]byte, error) {
cctx := NewCtx()
return cctx.CompressLevel(nil, input, BestSpeed)
})
}

func TestCtxEmptySliceCompress(t *testing.T) {
ctx := NewCtx()

Expand Down
8 changes: 2 additions & 6 deletions zstd_stream.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,17 +168,13 @@ func (w *Writer) Write(p []byte) (int, error) {
srcData = w.srcBuffer
}

var srcPtr *byte // Do not point anywhere, if src is empty
if len(srcData) > 0 {
srcPtr = &srcData[0]
}

// &srcData[0] is safe: it is p or w.srcBuffer but only if len() > 0 checked above
C.ZSTD_compressStream2_wrapper(
w.resultBuffer,
w.ctx,
unsafe.Pointer(&w.dstBuffer[0]),
C.size_t(len(w.dstBuffer)),
unsafe.Pointer(srcPtr),
unsafe.Pointer(&srcData[0]),
C.size_t(len(srcData)),
)
ret := int(w.resultBuffer.return_code)
Expand Down
16 changes: 16 additions & 0 deletions zstd_stream_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,22 @@ func TestStreamDecompressionChunks(t *testing.T) {
}
}

func TestStreamWriteNoGoPointers(t *testing.T) {
testCompressNoGoPointers(t, func(input []byte) ([]byte, error) {
buf := &bytes.Buffer{}
zw := NewWriter(buf)
_, err := zw.Write(input)
if err != nil {
return nil, err
}
err = zw.Close()
if err != nil {
return nil, err
}
return buf.Bytes(), nil
})
}

func BenchmarkStreamCompression(b *testing.B) {
if raw == nil {
b.Fatal(ErrNoPayloadEnv)
Expand Down
37 changes: 37 additions & 0 deletions zstd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,43 @@ func TestCompressLevel(t *testing.T) {
}
}

// structWithGoPointers contains a byte buffer and a pointer to Go objects (slice). This means
// Cgo checks can fail when passing a pointer to buffer:
// "panic: runtime error: cgo argument has Go pointer to Go pointer"
// https://github.com/golang/go/issues/14210#issuecomment-346402945
type structWithGoPointers struct {
buffer [1]byte
slice []byte
}

// testCompressDecompressByte ensures that functions use the correct unsafe.Pointer assignment
// to avoid "Go pointer to Go pointer" panics.
func testCompressNoGoPointers(t *testing.T, compressFunc func(input []byte) ([]byte, error)) {
t.Helper()

s := structWithGoPointers{}
s.buffer[0] = 0x42
s.slice = s.buffer[:1]

compressed, err := compressFunc(s.slice)
if err != nil {
t.Fatal(err)
}
decompressed, err := Decompress(nil, compressed)
if err != nil {
t.Fatal(err)
}
if !bytes.Equal(decompressed, s.slice) {
t.Errorf("decompressed=%#v input=%#v", decompressed, s.slice)
}
}

func TestCompressLevelNoGoPointers(t *testing.T) {
testCompressNoGoPointers(t, func(input []byte) ([]byte, error) {
return CompressLevel(nil, input, BestSpeed)
})
}

func doCompressLevel(payload []byte, out []byte) error {
out, err := CompressLevel(out, payload, DefaultCompression)
if err != nil {
Expand Down

0 comments on commit 0ead11a

Please sign in to comment.