Fix potential UB in ByteString.hashCode #208
Conversation
I thought it shouldn't, but:
|
bytestring/common/src/ByteString.kt
Outdated
| public constructor(data: ByteArray, startIndex: Int = 0, endIndex: Int = data.size) : | ||
| this(data.copyOfRange(startIndex, endIndex), null) | ||
|
|
||
| @Volatile |
There was a problem hiding this comment.
Does it make sense to add an expect annotation like VolatileInNative and actualize it with Volatile only in native?
Like in this patch
There was a problem hiding this comment.
The nicest alternative would be to somehow perform load/store w/ relaxed memory ordering, though:
https://godbolt.org/z/rEdcobsW3
There was a problem hiding this comment.
Pushed a slightly different version, please check it out.
The nicest alternative would be
Yep. I agree we ruled it out as "no use cases" during the initial design 😅
fzhinkin
left a comment
fzhinkin
left a comment
There was a problem hiding this comment.
Besides the small typo, everything LGTM. Thanks for resolving the issue!
Co-authored-by: Filipp Zhinkin <filipp.zhinkin@jetbrains.com>
It has a benign JVM data-race on the hashCode field, but on K/N LLVM has no guarantees it will read default or written value (e.g. OoTA is one of the possibilities). The recommendation from K/N folks is to use @volatile here
Fixes #190