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

Fix "Go pointer to Go pointer" panics #97

Merged
merged 2 commits into from
Mar 8, 2021

Conversation

evanj
Copy link
Contributor

@evanj evanj commented Mar 5, 2021

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

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
@evanj evanj force-pushed the evan.jones/pointer-to-go-pointer branch from 881fb82 to 0ead11a Compare March 5, 2021 22:05
@evanj
Copy link
Contributor Author

evanj commented Mar 5, 2021

I verified on CircleCI that the tests failed without the fix: https://app.circleci.com/pipelines/github/DataDog/zstd/90/workflows/d1eb9c40-deac-4358-81f4-ff148a65940b/jobs/333

Thanks to @niamster for telling me about this! This is an interesting one. Cgo is annoying.

Copy link
Collaborator

@Viq111 Viq111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the tests and the fix!

Would you mind making it not a draft PR anymore so we can merge it ?

I think @niamster tested it well so it should be good to go on this front as well

This should fix #98 once merged

zstd_stream.go Outdated Show resolved Hide resolved
@evanj evanj marked this pull request as ready for review March 8, 2021 18:49
@Viq111 Viq111 merged commit e292af4 into 1.x Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants