Pass pointers to C as unsafe.Pointer, not uintptr (memory corruption) #91

evanj · 2021-02-07T17:01:31Z

The unsafe package says: "Conversion of a uintptr back to Pointer is
not valid in general." This code was violating this rule, by passing
uintptr value to C, which would then interpret them as pointers. This
causes memory corruption: #90

This change replaces all uses of uintptr with unsafe.Pointer to avoid
this memory corruption. This has the disadvantage of marking every
argument as escaping to heap. This means the buffers used to call the
zstd functions must be allocated on the heap. I suspect this should
not be a huge problem, since I would expect that high performance
code should already be managing its zstd buffers carefully.

The bug is as follows:

In zstd_stream_test.go: payload := []byte("Hello World!") is
marked as "does not escape to heap" (from go test -gcflags -m).
Therefore, it is allocated on the stack.
The test calls Writer.Write, which converts the argument to uintptr:
srcPtr = uintptr(unsafe.Pointer(&srcData[0]))
Writer.Write then calls Cgo: C.ZSTD_compressStream2_wrapper(...)
The Go runtime decides the stack needs to be larger, so it copies
it to a new location.
(Another thread): The Go runtime decides to reuse the old stack
location, so it replaces the "Hello World!" bytes with new data.
(Original thread): Calls zstd, which reads the wrong bytes.

This change adds a test which nearly always crashes for me. While
investigating the other uses of uintptr, I also was able to trigger
a similar crash when calling CompressLevel, but only with:
GODEBUG=efence=1 go test .

I also added tests for Ctx.CompressLevel because it was not
obviously being tested. I did not reproduce this problem with that
function, but I suspect the same bug exists, since it uses the same
pattern.

For a minimal reproduction of this bug, see:
https://github.com/evanj/cgouintptrbug

The unsafe package says: "Conversion of a uintptr back to Pointer is not valid in general." This code was violating this rule, by passing uintptr value to C, which would then interpret them as pointers. This causes memory corruption: #90 This change replaces all uses of uintptr with unsafe.Pointer to avoid this memory corruption. This has the disadvantage of marking every argument as escaping to heap. This means the buffers used to call the zstd functions must be allocated on the heap. I suspect this should not be a huge problem, since I would expect that high performance code should already be managing its zstd buffers carefully. The bug is as follows: * In zstd_stream_test.go: payload := []byte("Hello World!") is marked as "does not escape to heap" (from go test -gcflags -m). Therefore, it is allocated on the stack. * The test calls Writer.Write, which converts the argument to uintptr: srcPtr = uintptr(unsafe.Pointer(&srcData[0])) * Writer.Write then calls Cgo: C.ZSTD_compressStream2_wrapper(...) * The Go runtime decides the stack needs to be larger, so it copies it to a new location. * (Another thread): The Go runtime decides to reuse the old stack location, so it replaces the "Hello World!" bytes with new data. * (Original thread): Calls zstd, which reads the wrong bytes. This change adds a test which nearly always crashes for me. While investigating the other uses of uintptr, I also was able to trigger a similar crash when calling CompressLevel, but only with: GODEBUG=efence=1 go test . I also added tests for `Ctx.CompressLevel` because it was not obviously being tested. I did not reproduce this problem with that function, but I suspect the same bug exists, since it uses the same pattern. For a minimal reproduction of this bug, see: https://github.com/evanj/cgouintptrbug

evanj · 2021-02-07T17:02:48Z

Based on this test, we should probably change the CircleCI tests to run the tests with GODEBUG=efence=1 to check against these types of extremely subtle issues. I can make an attempt to do that change at some point later.

See #91 A bug was found that can only be consistently reproduced with efrence=1 so enable it in tests so we make sure to catch it

See #91 We need to add a test with efence enabled. I tried to add it directly with the current suite but unfortunatly efence uses a lot of memory so we can only test it with smaller payload. Take the license as an example

Viq111 · 2021-02-08T22:20:59Z

Thanks for the PR!
I tried to import the tests onto the 1.x (without the fix) and run with efence on CircleCI but unfortunatly it still looks like it's hard to replicate (i.e: the tests pass without the fix: https://app.circleci.com/pipelines/github/DataDog/zstd/61/workflows/a2a5fbd9-d76e-4de8-b981-5510372bf198/jobs/213)

evanj · 2021-02-09T14:39:51Z

Rats you are right. I swear this was crashing reliably for me! Let me investigate a bit more...

evanj · 2021-02-09T15:18:32Z

My test for TestCompressLevel was incorrect; I think I screwed up pulling the version of the test into my branch. I'll fix this PR in a moment, but it does fail in CircleCI: https://app.circleci.com/pipelines/github/DataDog/zstd/64/workflows/e8bc7fb9-a336-4f30-b077-42c6da4206b4/jobs/225

The TestStreamCompressionDecompressionParallel test does not fail with GODEBUG=efence=1 which ... surprises me? I get memory corruption pretty reliably when running that test by itself on my machine, but not on CircleCI. I'll investigate that a bit more ...

This bug is very sensitive to stack sizes. Calling Decompress caused that function to copy the stack, which then avoided the bug.

evanj · 2021-02-09T18:23:49Z

Okay, I have the other test for stream compression crashing in CircleCI as well: I needed to use more goroutines for some reason. https://app.circleci.com/pipelines/github/DataDog/zstd/67/workflows/c164d97f-9db4-46d5-83cf-2c9fa37b447f/jobs/235

This bug interestingly seems to go away with GODEBUG=efence=1; I think because that flag ensures that memory allocations never reuse the same memory. It only marks pages as "inaccessible" for large allocations, so I need to make the stack really big to get it to crash for me on multiple systems. I'm going to update the tests in this PR with the ones that seem to be more reliable.

In summary: this bug is very tricky, and may not happen in some cases!

Viq111

Thanks for finding this!
It's a bit unfortunate we had to walk back on some of the optimizations but it definitely make sense given the potential memory corruptions.

Let me know if you are able to get a test we can consistently reproduce on CircleCI (Ideally we'd want that before merging this PR (and if needed we can cleanup the tests like stackDepth if it turns out it cannot reproduce the bug))

Viq111 · 2021-02-11T15:45:20Z

I ran the benchmark against the same payload we run in the tests (i.e: mr from http://sun.aei.polsl.pl/~sdeor/index.php?page=silesia) which is a ~large file of 9Mb on a Macbook Pro 2019 and there is no notable change in benchmark. I'm guessing it will likely have a slight impact on very small payloads (when you need to do a lot of calls into cgo):

goos: darwin
goarch: amd64
ᐅ benchstat before uintptr-bug
name                    old time/op   new time/op   delta
CtxCompression-16        91.7ms ± 6%   90.9ms ± 3%    ~     (p=0.574 n=8+8)
CtxDecompression-16      11.7ms ± 1%   11.8ms ± 1%  +0.50%  (p=0.024 n=9+9)
StreamCompression-16     96.4ms ± 2%   95.3ms ± 2%    ~     (p=0.055 n=8+10)
StreamDecompression-16   14.0ms ± 2%   13.9ms ± 1%    ~     (p=0.720 n=10+9)
Compression-16           91.2ms ± 3%   90.5ms ± 3%    ~     (p=0.579 n=10+10)
Decompression-16         11.8ms ± 2%   11.8ms ± 2%    ~     (p=0.853 n=10+10)

name                    old speed     new speed     delta
CtxCompression-16       109MB/s ± 6%  110MB/s ± 3%    ~     (p=0.574 n=8+8)
CtxDecompression-16     851MB/s ± 1%  847MB/s ± 1%  -0.49%  (p=0.024 n=9+9)
StreamCompression-16    103MB/s ± 2%  105MB/s ± 2%    ~     (p=0.055 n=8+10)
StreamDecompression-16  713MB/s ± 2%  717MB/s ± 1%    ~     (p=0.720 n=10+9)
Compression-16          109MB/s ± 3%  110MB/s ± 2%    ~     (p=0.543 n=10+10)
Decompression-16        846MB/s ± 2%  847MB/s ± 2%    ~     (p=0.853 n=10+10)

evanj · 2021-02-11T21:54:18Z

I believe the tests that I added in commit 88ff5e1 do reproduce consistently on CircleCI. I needed to increase the number of goroutines for the parallel test, then use the recursive function stack depth trick to get it to crash with GODEBUG=efence=1. If you want to check that I didn't screw this up again, that would be great, but I did this separately:

https://app.circleci.com/pipelines/github/DataDog/zstd/64/workflows/e8bc7fb9-a336-4f30-b077-42c6da4206b4/jobs/225
https://app.circleci.com/pipelines/github/DataDog/zstd/67/workflows/c164d97f-9db4-46d5-83cf-2c9fa37b447f/jobs/235

Viq111 · 2021-02-12T14:43:50Z

Thanks! I ran the tests also on my branch and can confirm! https://app.circleci.com/pipelines/github/DataDog/zstd/79/workflows/a3fd4930-b663-4d4c-a57a-2a9c00740a70

go-efence seems to catch it. Without efence, go-1.15 failed but go-1.14 succeeded so it seems we can't replicate 100% of the time. I'm going to import the circleci config to add the efence test to make sure we catch those in the future as well and good to go!

See #91 We caught a nasty bug because of misusage of uintptr in Go. This was fixed in that PR but add a test case in the circleci matrix to make sure we catch those in the future if they happen again

Unfortunately linux32 limits us to 3GB of memory for 32 bits systems. We will need to disable the memory-hungry tests there. We also only need 4GB for the docker instance

Viq111 · 2021-02-12T15:45:04Z

@evanj I think I fixed the remaining test problems with the last 4 commits.
Let me know if they look good to you and I can merge

evanj · 2021-02-16T16:05:09Z

It is very annoying that this bug depends on many implementation-specific details, so is hard to replicate. These test fixes look good to me. Thanks for checking and being so persistent with this!

evanj mentioned this pull request Feb 7, 2021

TestStreamCompressionDecompressionParallel frequently fails on machines with many cpus #90

Closed

Viq111 added a commit that referenced this pull request Feb 8, 2021

[tests] Add efence=1 & race testing

b2c9676

See #91 A bug was found that can only be consistently reproduced with efrence=1 so enable it in tests so we make sure to catch it

Viq111 added a commit that referenced this pull request Feb 8, 2021

[tests] Add efence=1 & race testing

47b4fea

See #91 A bug was found that can only be consistently reproduced with efrence=1 so enable it in tests so we make sure to catch it

zstd_test.go: Fix TestCompressLevelStackCgoBug to actually crash

8f7b34a

This bug is very sensitive to stack sizes. Calling Decompress caused that function to copy the stack, which then avoided the bug.

evanj added 2 commits February 9, 2021 13:27

make the stream test reproducible on CircleCI

88ff5e1

remove dangerous comment: this is never safe to use

fb6ff35

Viq111 approved these changes Feb 11, 2021

View reviewed changes

Viq111 added 2 commits February 11, 2021 12:08

Merge branch '1.x' into uintptr-bug

0ad1251

Merge branch '1.x' into uintptr-bug

ac10cc4

Viq111 mentioned this pull request Feb 11, 2021

Add Go module file #89

Closed

Viq111 added blocker Something that needs to be fixed before next release bug labels Feb 11, 2021

Viq111 added 4 commits February 12, 2021 09:45

[circleci] Add efence test

e264729

See #91 We caught a nasty bug because of misusage of uintptr in Go. This was fixed in that PR but add a test case in the circleci matrix to make sure we catch those in the future if they happen again

[circleci] Give more memory to 32bits runner

dc93d13

[circleci] Give more memory again to 32bits runner

9107448

[tests] Disable memory-hungry tests on 32bits

5b15ce1

Unfortunately linux32 limits us to 3GB of memory for 32 bits systems. We will need to disable the memory-hungry tests there. We also only need 4GB for the docker instance

Viq111 merged commit 8cb8bac into 1.x Feb 16, 2021

Viq111 deleted the uintptr-bug branch February 16, 2021 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass pointers to C as unsafe.Pointer, not uintptr (memory corruption) #91

Pass pointers to C as unsafe.Pointer, not uintptr (memory corruption) #91

evanj commented Feb 7, 2021

evanj commented Feb 7, 2021

Viq111 commented Feb 8, 2021

evanj commented Feb 9, 2021

evanj commented Feb 9, 2021

evanj commented Feb 9, 2021

Viq111 left a comment

Viq111 commented Feb 11, 2021

evanj commented Feb 11, 2021

Viq111 commented Feb 12, 2021

Viq111 commented Feb 12, 2021

evanj commented Feb 16, 2021

Pass pointers to C as unsafe.Pointer, not uintptr (memory corruption) #91

Pass pointers to C as unsafe.Pointer, not uintptr (memory corruption) #91

Conversation

evanj commented Feb 7, 2021

evanj commented Feb 7, 2021

Viq111 commented Feb 8, 2021

evanj commented Feb 9, 2021

evanj commented Feb 9, 2021

evanj commented Feb 9, 2021

Viq111 left a comment

Choose a reason for hiding this comment

Viq111 commented Feb 11, 2021

evanj commented Feb 11, 2021

Viq111 commented Feb 12, 2021

Viq111 commented Feb 12, 2021

evanj commented Feb 16, 2021