-
Notifications
You must be signed in to change notification settings - Fork 143
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
Test GoThemis with Go 1.11~1.14 on CircleCI #595
Conversation
We have found out that GoThemis seems to be broken with Go 1.11. Currently we test it versus whatever version is in the image. Instead, download latest supported versions of Go and test against them. We would like to make use of CircleCI caches -- because that's around 500 MB that we're downloading -- but it turns out that it does not provide much of a speedup, so we simply pull tarballs from Google. Ideally, they should be a part of Docker image we use.
Add a test which expemplifies the bug in CGo 1.11 and earlier versions (at least since 1.6). It has been fixed in 1.12 but we still have to support some earlier versions. The bug causes the following test to panic. For some unknown reason it seems that only Secure Cell is susceptible to it, and only with "context" argument being used with bytes.Buffer vOv The panic itself is caused by a bug in CGo implementation which triggers a false positive in a pointer check, which erroenously verifies not only the context []byte buffer, but the entire bytes.Buffer object. Read more in the linked GitHub issue if you are interested.
We can work around the bug by copying each and every []byte slice that will get passed into C code via CGo. This removes association with bytes.Buffer and avoids any possible false positives in CGo. Since the bug has been fixed in Go 1.12, we copy the buffer only for earlier versions, making SanitizeBuffer() a no-op in later versions.
gothemis/message/message.go
Outdated
if nil != private && 0 < len(private.Value) { | ||
priv = unsafe.Pointer(&private.Value[0]) | ||
privLen = C.size_t(len(private.Value)) | ||
if private != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting to make the benchmark, how much slower it works with copying data and without.
maybe we can skip copying values of private/public keys because in most cases it generated by our code keys.New
where we don't use bytes.Buffer or something like that and we don't accept any buffer which can be used for new generated keys. So I can't imagine a case where public/private.Value will be something else than raw byte slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting to make the benchmark, how much slower it works with copying data and without
Well, memcpy()
is generally very cheap compared to more advanced processing so I don't think it will have any measurable impact. But sure, I'll add some simple benchmarks and report the findings.
maybe we can skip copying values of private/public keys because in most cases it generated by our code keys.New where we don't use bytes.Buffer or something like that
It is true that keys.New()
will produce well-formed []byte
slices, but we are not dealing with newly generated keys all the time. For example, we will need to deal with slices of unknown origin when the keys are received from external storage, as typically this goes by something like this:
keyBytes, _ := ioutil.ReadAll("my_key.pub")
key := &keys.PublicKey{Value: keyBytes}
smessage := message.New(nil, key)
smessage.Verify(data)
It's unlikely that any system utility will use bytes.Buffer
because Buffer()
returns a slice of some internal buffer, so it's not suitable as a general API. However, I can easily imagine some custom-made protocol parser which uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some numbers.
$ go version
go version go1.11.13 darwin/amd64
Three successive runs of go test -bench
.
With workaround
goos: darwin
goarch: amd64
pkg: github.com/cossacklabs/themis/gothemis/message
BenchmarkSMessageEncryptRSA-4 20000 77981 ns/op
BenchmarkSMessageEncryptEC-4 3000 457969 ns/op
BenchmarkSMessageDecryptRSA-4 1000 1754761 ns/op
BenchmarkSMessageDecryptEC-4 3000 459720 ns/op
BenchmarkSMessageSignRSA-4 1000 1665766 ns/op
BenchmarkSMessageSignEC-4 10000 190236 ns/op
BenchmarkSMessageVerifyRSA-4 20000 67589 ns/op
BenchmarkSMessageVerifyEC-4 5000 258329 ns/op
PASS
ok github.com/cossacklabs/themis/gothemis/message 19.787s
goos: darwin
goarch: amd64
pkg: github.com/cossacklabs/themis/gothemis/message
BenchmarkSMessageEncryptRSA-4 20000 86262 ns/op
BenchmarkSMessageEncryptEC-4 3000 478931 ns/op
BenchmarkSMessageDecryptRSA-4 1000 1946130 ns/op
BenchmarkSMessageDecryptEC-4 3000 442622 ns/op
BenchmarkSMessageSignRSA-4 1000 1807819 ns/op
BenchmarkSMessageSignEC-4 10000 191501 ns/op
BenchmarkSMessageVerifyRSA-4 20000 65709 ns/op
BenchmarkSMessageVerifyEC-4 10000 245958 ns/op
PASS
ok github.com/cossacklabs/themis/gothemis/message 19.979s
goos: darwin
goarch: amd64
pkg: github.com/cossacklabs/themis/gothemis/message
BenchmarkSMessageEncryptRSA-4 20000 79789 ns/op
BenchmarkSMessageEncryptEC-4 3000 444484 ns/op
BenchmarkSMessageDecryptRSA-4 1000 1611965 ns/op
BenchmarkSMessageDecryptEC-4 3000 428861 ns/op
BenchmarkSMessageSignRSA-4 1000 1672447 ns/op
BenchmarkSMessageSignEC-4 10000 185019 ns/op
BenchmarkSMessageVerifyRSA-4 20000 64634 ns/op
BenchmarkSMessageVerifyEC-4 10000 242627 ns/op
PASS
ok github.com/cossacklabs/themis/gothemis/message 18.179s
Without workaround
goos: darwin
goarch: amd64
pkg: github.com/cossacklabs/themis/gothemis/message
BenchmarkSMessageEncryptRSA-4 20000 78610 ns/op
BenchmarkSMessageEncryptEC-4 3000 433074 ns/op
BenchmarkSMessageDecryptRSA-4 1000 1625414 ns/op
BenchmarkSMessageDecryptEC-4 3000 438958 ns/op
BenchmarkSMessageSignRSA-4 1000 1619214 ns/op
BenchmarkSMessageSignEC-4 10000 180392 ns/op
BenchmarkSMessageVerifyRSA-4 20000 65154 ns/op
BenchmarkSMessageVerifyEC-4 5000 249397 ns/op
PASS
ok github.com/cossacklabs/themis/gothemis/message 17.320s
goos: darwin
goarch: amd64
pkg: github.com/cossacklabs/themis/gothemis/message
BenchmarkSMessageEncryptRSA-4 20000 78407 ns/op
BenchmarkSMessageEncryptEC-4 3000 431794 ns/op
BenchmarkSMessageDecryptRSA-4 1000 1621143 ns/op
BenchmarkSMessageDecryptEC-4 3000 427959 ns/op
BenchmarkSMessageSignRSA-4 1000 1606592 ns/op
BenchmarkSMessageSignEC-4 10000 186542 ns/op
BenchmarkSMessageVerifyRSA-4 20000 66023 ns/op
BenchmarkSMessageVerifyEC-4 5000 252454 ns/op
PASS
ok github.com/cossacklabs/themis/gothemis/message 18.223s
goos: darwin
goarch: amd64
pkg: github.com/cossacklabs/themis/gothemis/message
BenchmarkSMessageEncryptRSA-4 20000 83979 ns/op
BenchmarkSMessageEncryptEC-4 3000 576246 ns/op
BenchmarkSMessageDecryptRSA-4 1000 1728592 ns/op
BenchmarkSMessageDecryptEC-4 3000 462749 ns/op
BenchmarkSMessageSignRSA-4 1000 1774446 ns/op
BenchmarkSMessageSignEC-4 10000 197490 ns/op
BenchmarkSMessageVerifyRSA-4 20000 73172 ns/op
BenchmarkSMessageVerifyEC-4 5000 245907 ns/op
PASS
ok github.com/cossacklabs/themis/gothemis/message 18.621s
Measurements have pretty high variance but I don't see any significant difference in them. Benchmarks don't give us raw data and I don't want to do tons of runs to do proper statistics.
We've got curious about impact of copying on Secure Message performance so here are some primitive benchmarks to measure the impact (if any).
gothemis/utils/sanitize_go1.11.go
Outdated
if data == nil { | ||
return nil | ||
} | ||
// Copying the data before passing it into CGo avoids the problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't copying data in memory increase amount of private keys stored in memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will, obviously.
We can probably zero out some of the copies, but we should be careful to not wipe the slices given to us. Doing so will break the API as some users might assume that the slices and keys are unchanged.
gothemis/utils/sanitize_go1.12+.go
Outdated
|
||
package utils | ||
|
||
// SanitizeBuffer applies a workaround for CGo 1.11 and earlier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// SanitizeBuffer applies a workaround for CGo 1.11 and earlier. | |
// SanitizeBuffer applies a workaround for CGo 1.11 and earlier. | |
// Since Go 1.12 this workaround does nothing. |
I’ve just realized that we can handle keys in Secure Messages slightly better, avoiding copying them on every operation and doing it only on Secure Message constructions. Though, this will break an (arguably silly) use case when |
This reverts commit 2914dd7.
This reverts commit 5b2f623.
After discussion we have decided to not include a fix for this behavior because of miscellaneous complications. However, the test case is still there. Just update it so that it fails when our code unexpectedly panics but passes if the panic is raised for Go 1.11 and below.
Okay, everyone! After offline discussion and some prototyping we found out that a proper workaround for this issue is too ugly and hard to maintain, so we will not add it. Selective copying and zeroing out of those copies is hard to code correctly. Apparently correct solution ( I have reverted the workaround and removed it from this PR. Other updates are still there, like testing with multiple versions of Go. Regression test also remains, I tweaked it so that it passes with Go 1.11 (expecting a panic in this case). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes we can't fix all bugs, but adding more tests is always a good idea
Recent development in Acra has uncovered a bug in CGo which caused Secure Cell calls to panic on CircleCI builds like this:
It can be triggered by passing GoThemis byte slices returned by
bytes.Buffer
in go 1.11 and some earlier versions. It does not seem to be triggered every time and with every CGo call but I have found that “context” argument of Secure Cell calls causes a panic reliably.Add a workaround for this bug so that we don’t panic randomly. Since we can’t know whether
bytes.Buffer
has been used, we simply copy each and every byte slice that we operate on before passing it to Themis Core via CGo. The bug has been fixed in go 1.12, so we use conditional compilation to make that workaround a no-op on versions that are not affected.utils.SanitizeBuffer()
is technically exported by GoThemis and available to the users, but this function is considered an implementation detail and may be removed in the future (e.g., once we decide that we no longer wish to support go 1.11 and stop maintaining this workaround).CircleCI has been running tests only with the version that is shipped with the Docker image. Update the configuration to test GoThemis with go 1.11—1.14. Our policy is to provide full support for the current stable versions of Go (those are 1.14 and 1.13.8 right now), and best effort support for earlier ones.
Checklist