-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
slices of an array of bytes: runtime mutation of source data variables when using file IO (ReadFile()) without pointers and unsafe.* operations in user code #28585
Comments
From https://play.golang.org/p/G4uMh6pCwOx, lines 21 and 22 EncryptedContent := EncryptedContentMut
copy(EncryptedContent, EncryptedContentMut) The type of I guess the intent of this code is to have the two variables not share the backing array. To do that: EncryptedContent := append([]byte(nil), EncryptedContentMut...) |
This code is just for experimenting to check the behaviour/semantics if we apply the assignment operation or even copy the slice of bytes (I guess in this operation Go only creates poiter to the same array of bytes). So, if we even disable this code and rename |
The point is that the code assumes slices work in a way they actually don't. I don't think there's any bug in Go involved here. At least, can you please make the reproducer smaller, simpler and remove all the printing noise? (Byte literals are also usually written as eg. And you must clearly state where you see the bug. As in 'What happens' vs 'What do you expect to happen instead?'. Nowhere I found any indication of this. You're familiar with the code, others are not. |
It sounds like there's a misunderstanding of this package's API and/or the Go language here, and not a bug in Go. Unfortunately we don't have the resources in this bug tracker to help with all problems. Let's bring this to golang-nuts or another forum for now (see https://golang.org/wiki/Questions). If/when a concrete, small problem is identified (even if it's an omission in the docs), then feel free to file a new bug here or comment on this one. Thanks. |
Here's an even more simplified two examples, and now we have mutation of source data in https://play.golang.org/p/K5_XMqTh9LA https://play.golang.org/p/gRcqFywbHL6 Idk, how to make it simpler and explain the problem in a even more concise way - I edited and structured all description of the problem in the first message, now it's stated in a more precise manner, so you may read it carefully above. About notation:
https://golang.org/ref/spec#Conversions_to_and_from_a_string_type '\x' and '\u' — is a hex and unicode (respectively) representation of char/byte in a string and in a https://golang.org/ref/spec#Rune_literals |
IDK why three different playground links, I'd expect just one. Still a lot of noisy prints yet not a single one of them ever cares to reveal what's perceived wrong. Can there be at least/just one that prints something like "Here we expect 0x42 0x31 but we got 0x12 0x13 instead"? (The
No string literal is converted in the above quoted code. The []byte{0xAB, 0xCD} is completely equivalent, just shorter and thus arguably more readable. |
Solution So, now in nearly every function we create and use with passing It's most common pattern for Go semantics for slices and maps, which is passed by reference. In the beginning of
And all issues gone, 'cause we explicitly instantiate new array with slice and pointer to this new instance. Thanks a lot for help! PS: This implicit semantics and behaviour is not so obvious from the first sight. Especially when you expect passing by value (#28554) and immutability, 'cause has a common experience in functional languages with immutability in mind and system languages where we use pointers explicitly to pass by reference. #28554 - here we must use https://golang.org/pkg/builtin/#make
#28585 - and here we must use https://golang.org/pkg/builtin/#append
Also there's a https://golang.org/pkg/builtin/#copy
And https://golang.org/pkg/builtin/#new
|
That's not commonly seen and it's probably not necessary. It will have negative performance impact. There's possibly a better solution for this particular issue, but sadly you have never shown us the exact problem and its location (as in here we expected X but got Y instead). |
As pointed above, let us use golang-nuts or another forum, which is more suited for conversations such as these. And when there is a concrete bug request, then file an issue. Thank you. |
What did you do?
Here's the earlier two simplified examples (based on production code, in which we observed such unexpected behaviour initially) - this code use file IO, instead of buffers (as in examples in previous issue - #28554).
When we use file IO (
ioutil.ReadFile()
) to read encrypted content we have mutation of source data (EncryptedContent
andEncryptedContentMut
) after decryption.It's maybe because
ReadFile()
returns slice, passed by reference, through pointer, as usually (#28554) and source data mutate over slices through pointers, but I'm not sure. So maybe it's depend on internal working ofReadFile()
routine.I think this is a bug, 'cause it' has incorrect and unexpected semantics. And if not so - please, can you explain why and how does it happen, why it's work as it does and data mutates?
https://play.golang.org/p/G4uMh6pCwOx
Otherwise, when we use encryption without file IO (
ioutil.ReadFile()
) and encrypt data in-place (encryptAES256CBC()
) or just use in-source hard-coded slice of an array of bytes in hex representation (of encrypted data) - we don't have a mutation of the source encrypted data inEncryptedContent
andEncryptedContentMut
variables after decryption routine returns:https://play.golang.org/p/TnUmiHFy7iC
https://play.golang.org/p/3kfHf1NpE2I
What did you expect to see?
Source encrypted data in
EncryptedContent
and inEncryptedContentMut
variables must not mutate after passing todecryptAES256CBC()
and decryption operations, 'cause mutation happen only when source encrypted data gets from file throughioutil.ReadFile()
routine.What did you see instead?
In both cases we use slices of byte arrays, i.e. similar data-types, but have different semantics - mutation of source encrypted data (
EncryptedContent
,EncryptedContentMut
variables) in case of reading them from file usingioutil.ReadFile()
in first example, and in second example we doesn't observe mutation (which is normal behaviour) ofEncryptedContent
andEncryptedContentMut
variables in case of using in-place encryption of source data withencryptAES256CBC()
or using in-source hard-coded encrypted slice of an array of bytes.What impact this induce?
The open decrypted text in
DecryptedContent
are gets into encrypted data (EncryptedContent
andEncryptedContentMut
variables) and replaces upper bytes. This cause malfunction of cryptosystem of our product and even goes to malicious using of this data (personal information, secret data, etc.).============
Just to mention:
@ianlancetaylor @dvyukov @davecheney @rsc @bradfitz @adg @aclements @griesemer @robpike
@llorephie @luckyrider @Parabellum1905y @sabbakumov @KanybekMomukeyev
The text was updated successfully, but these errors were encountered: