add collection of deliberate cases of UB #448
Conversation
RalfJung
force-pushed
the
deliberate-ub
branch
2 times, most recently
from
e868b0a to
05daf0a
Compare
RalfJung
force-pushed
the
deliberate-ub
branch
from
05daf0a to
a1192d1
Compare
There was a problem hiding this comment.
LGTM, though two potential future refinements:
- Differentiate somehow between cases which cause LLVM UB and which are UB in the frontend but not in the backend yet; the former is more pressing than the latter, which could be argued as "just" relying on implementation details.
- Add some examples of patterns that SB considers UB but people want to do anyway (e.g.
&Header, probably "work in progress" given TB resolves it).
But this is useful even without, and these both are more about the "potentially UB" class than the "definite UB" class that this is more about.
resources/deliberate-ub.md
Outdated
| To fix this we need to be able to perform atomic loads at type `MaybeUninit<u32>`. | ||
| * Similar to the previous case, the `atomic-memcpy` crate uses the [standard `Atomic*` types to load potential padding or pointer bytes](https://github.com/taiki-e/atomic-memcpy/blob/b7b82bface0f24c5a9171f2d0626871c61302152/src/lib.rs#L323). | ||
| This is a user-space attempt to implement bytewise atomic memcpy, so [adding that as a native operation](https://github.com/rust-lang/rfcs/pull/3301) should fix this. | ||
| * `bytes` does [a non-atomic plain load that races](https://github.com/tokio-rs/bytes/blob/dea868a4b0eec28877e9013702c0ae12dbc40c4b/src/bytes.rs#L2508), |
There was a problem hiding this comment.
IIRC, this no longer exists in the latest version of bytes.
There was a problem hiding this comment.
Interesting, I can't find it in current versions either. Do you know what they are doing instead? I can't find any atomics in BytesMut, did they just give up on having the mutable version be thread-safe?
There was a problem hiding this comment.
This seems to have happened with a big refactor. @seanmonstar is the old problem with atomics in BytesMut::kind completely gone or is that now just somehow moved somewhere else?
We already have an issue open for that, so not sure it's worth also going out and hunting for examples. Other people can make PRs after this one lands. :) |
RalfJung
force-pushed
the
deliberate-ub
branch
from
eec1983 to
64979ed
Compare
|
If the |
|
|
||
| * crossbeam's `AtomicCell` implements an SeqLock, which [is well-known to not be compatible with the C++ memory model](http://www.hpl.hp.com/techreports/2012/HPL-2012-68.pdf). | ||
| Specifically the problem is [this non-atomic volatile read](https://github.com/crossbeam-rs/crossbeam/blob/5d07fe43540d7f21517a51813acd9332744e90cb/crossbeam-utils/src/atomic/atomic_cell.rs#L980) which can cause data races and hence UB. | ||
| This would be fine if we either (a) adopted LLVM's handling of memory races (then the problematic read would merely return `undef` instead of UB due to a data race), or (b) added [bytewise atomic memcpy](https://github.com/rust-lang/rfcs/pull/3301) and used that instead of the non-atomic volatile load. |
There was a problem hiding this comment.
@taiki-e if I have you around anyway, could you briefly say if this is correct -- if Rust gets atomic bytewise memcpy, then AtomicCell could use them?
There was a problem hiding this comment.
Yeah, RFC 3301 should be enough to fix it.
That said, I think we will likely fix it by adopting "(c) use inline assembly instead of the non-atomic volatile load" before using the implementation of RFC 3301.
There was a problem hiding this comment.
Sure, inline assembly is always an option, though usually not a nice one.
|
I explored I'll merge this now, we can always edit this further later. This was definitely a useful update! |
|
|
||
| ### Cases related to concurrency | ||
|
|
||
| * crossbeam's `AtomicCell` "fallback path" uses a SeqLock, which [is well-known to not be compatible with the C++ memory model](http://www.hpl.hp.com/techreports/2012/HPL-2012-68.pdf). |
There was a problem hiding this comment.
This link doesn't load for me, I think because of the http:// rather than https://.
There was a problem hiding this comment.
Good catch, I'll fix it in the repo. (My browser is in https-always mode so even the http link works here...)
Fixes #158