5 ms·

> What issues we did find were mistakes in the implementations of low-level and explicitly unsafe multithreading abstractions (emphasis mine) The important th

> What issues we did find were mistakes in the implementations of low-level and explicitly unsafe multithreading abstractions (emphasis mine) The important thing is that the races only happened in unsafe { } blocks. It's well-established that these blocks should basically be treated like C++ in terms of scrutiny, but (I believe) you can still have roughly "absolute confidence" in non-unsafe code. It's true and interesting that unsafe { } blocks deserve sanitization analysis like C++ code does - and maybe this hasn't been discussed enough - but I don't think it's fair to suggest that the enormous language benefits are outweighed by some false sense of confidence. The proportions in that idea just seem way out of whack.

benschulz 6y ago
AFAIU one can create races in safe code using atomics with insufficient ordering. (That notwithstanding, I agree that Rust is a vast improvement over C/++.)
- steveklabnik 6y ago
  "races" is too general. Safe Rust cannot have data races. It can have race conditions.
  - benschulz 6y ago
    I'm not convinced this is a meaningful distinction in this context. Assuming a developer consciously creates a race which turns out to be a bug, how does it matter whether it was a data race or some other kind of race condition?
    - steveklabnik 6y ago
      Data races are undefined behavior, and race conditions are a logic error. Yes, both are bugs, and both are important to fix. But they have (at least to me) different severities. (Worth noting that data races have a clear, unambiguous definition, and therefore are possible to eliminate through tooling. Race conditions rely on program intent or logic, and are therefore... not really.) Also, I think it matters, generally, that we are truthful about what Rust prevents and does not prevent.
      - brundolf 6y ago
        > Worth noting that data races have a clear, unambiguous definition, and therefore are possible to eliminate through tooling. Race conditions rely on program intent or logic, and are therefore... not really. This is a great point. Usually people only talk about the two categories in terms of vague severity, but this is a hard distinction which has lots of implications for how each can be dealt with.
      - benschulz 6y ago
        > Data races are undefined behavior, and race conditions are a logic error. Yes, both are bugs, and both are important to fix. But they have (at least to me) different severities. Some data races really are benign while some other race conditions can lead to data corruption or security vulnerabilities. Why then should data races be strictly more severe than data races? What am I missing?
      - pjmlp 6y ago
        Any kind of race can lead to data corruption, or being very drastic, possible death if the outcome is related to any computer system with direct influence over human lives like factory automation systems. This is why I think Rust discussions about this subject don't focus enough that the language only validates a very tiny subset of races.
    - deleted 6y ago
      [deleted]
    - bcoates 6y ago
      In general data races can break program invariants, violate memory safety and do other undefined behavior. In C++, data races can cause jumps to invalid addresses and the compiler will assume data races away and delete checks that are redundant in the single-thread case. The equivalent problem happens in most languages where data races are possible (two Python threads writing at once can put a value into an impossible/illegal state) An ordinary race condition is just an observable effect of the nondeterministic progress of threads, so it's possible to have a truly benign race condition, so long as the correctness of the whole program doesn't depend on the outcome of the race. I/O causes nondeterminism anyway, so it's not really practical to avoid all ordinary races.
      - benschulz 6y ago
        It's possible to have truly benign data races too, e.g. lazily computing a hash of immutable data. One of the reasons that Rust's ownership model is necessary is most data races cannot otherwise be statically detected. The compiler will be unable to apply any of its theoretical UB optimizations. It will emit its native code and you get the defined behavior of the underlying hardware. There will be no undefined behavior. If that ever changes, the bigger news, I imagine, will be that we solved the halting problem.
    - anp 6y ago
      "This context" is the original article, which is discussing data races specifically.
      - benschulz 6y ago
        Actually "this context" was my previous reply. Much like I can nest a lexical scope inside a program and have it reach up and past the containing scope, I can create a new context in a discussion. Please at least try to understand my position before you reply.
  - rurban 6y ago
    And deadlocks
- volta83 6y ago
  You can also create races in safe Rust by using the file system, no concurrency primitives necessary (just create a new file, close it, and re-open it assuming it's still there..). What safe Rust doesn't have is _data races_.
nyanpasu64 6y ago
The thing is, the linked data race occurred in safe code! Looking at https://bugzilla.mozilla.org/show_bug.cgi?id=1686158 https://bugzilla.mozilla.org/show_bug.cgi?id=1686158, the patch changes a non-atomic read through a &SwCompositeGraphNode and a subsequent non-atomic write through a &mut SwCompositeGraphNode (both safe code). They should not race, and the only reason they do is because there wasn't proper RwLock-style synchronization when handing out &SwCompositeGraphNode and &mut SwCompositeGraphNode. Also I think merely creating an unsynchronized &mut is undefined behavior in of itself (in addition to allowing UB in safe code). If so, even the fixed code is still technically wrong (just not miscompiled or misbehaving in practice). Looking at https://searchfox.org/mozilla-central/rev/ee9dab6aa95f167a34cb178960f7375210a0bba4/gfx/wr/webrender/src/compositor/sw_compositor.rs https://searchfox.org/mozilla-central/rev/ee9dab6aa95f167a34..., SwCompositeGraphNodeRef has "safe" methods (line 286) with unsafe blocks, handing out raw &SwCompositeGraphNode and &mut SwCompositeGraphNode to the interior of an UnsafeCell, without locking or runtime checking. I think SwCompositeGraphNodeRef "caused" the UB because it was invoked by some other caller to create a &mut SwCompositeGraphNode without proper synchronization... And worse yet, SwCompositeGraphNodeRef has Deref and DerefMut implementations that implicitly create & and &mut, unsynchronized, at call sites that merely look like method calls... My theory as to how this happened: - C++ atomics are not mutable through a const *, so the authors used &mut to indicate mutability (even though Rust's &mut has stronger guarantees, according to the Stacked Borrows memory model, "all other pointers to this object are invalidated, except the pointers we reborrowed from"... though Stacked Borrows may change to make async fn and intrusive linked lists sound, I suspect it won't make this code sound). - Maybe they thought "we want to prevent calling &mut SwCompositeGraphNode methods from a &SwCompositeGraphNodeRef". IDK. Semi-related: SwCompositeGraphNodeRef is a #[derive(Clone)] Arc<UnsafeCell<SwCompositeGraphNode>> but !Sync, so only threads that own a SwCompositeGraphNodeRef can create either &SwCompositeGraphNodeRef or &mut SwCompositeGraphNodeRef. And I don't see &SwCompositeGraphNodeRef floating around, so I don't see why access control would matter. (EDIT: The commit introducing the unsound abstraction is https://hg.mozilla.org/integration/autoland/rev/8d47e408c578 https://hg.mozilla.org/integration/autoland/rev/8d47e408c578, filed under the bug at https://bugzilla.mozilla.org/show_bug.cgi?id=1670328 https://bugzilla.mozilla.org/show_bug.cgi?id=1670328.)

benschulz 6y ago

AFAIU one can create races in safe code using atomics with insufficient ordering. (That notwithstanding, I agree that Rust is a vast improvement over C/++.)

steveklabnik 6y ago
"races" is too general. Safe Rust cannot have data races. It can have race conditions.
- benschulz 6y ago
  I'm not convinced this is a meaningful distinction in this context. Assuming a developer consciously creates a race which turns out to be a bug, how does it matter whether it was a data race or some other kind of race condition?
  - steveklabnik 6y ago
    Data races are undefined behavior, and race conditions are a logic error. Yes, both are bugs, and both are important to fix. But they have (at least to me) different severities. (Worth noting that data races have a clear, unambiguous definition, and therefore are possible to eliminate through tooling. Race conditions rely on program intent or logic, and are therefore... not really.) Also, I think it matters, generally, that we are truthful about what Rust prevents and does not prevent.
    - brundolf 6y ago
      > Worth noting that data races have a clear, unambiguous definition, and therefore are possible to eliminate through tooling. Race conditions rely on program intent or logic, and are therefore... not really. This is a great point. Usually people only talk about the two categories in terms of vague severity, but this is a hard distinction which has lots of implications for how each can be dealt with.
    - benschulz 6y ago
      > Data races are undefined behavior, and race conditions are a logic error. Yes, both are bugs, and both are important to fix. But they have (at least to me) different severities. Some data races really are benign while some other race conditions can lead to data corruption or security vulnerabilities. Why then should data races be strictly more severe than data races? What am I missing?
    - pjmlp 6y ago
      Any kind of race can lead to data corruption, or being very drastic, possible death if the outcome is related to any computer system with direct influence over human lives like factory automation systems. This is why I think Rust discussions about this subject don't focus enough that the language only validates a very tiny subset of races.
  - deleted 6y ago
    [deleted]
  - bcoates 6y ago
    In general data races can break program invariants, violate memory safety and do other undefined behavior. In C++, data races can cause jumps to invalid addresses and the compiler will assume data races away and delete checks that are redundant in the single-thread case. The equivalent problem happens in most languages where data races are possible (two Python threads writing at once can put a value into an impossible/illegal state) An ordinary race condition is just an observable effect of the nondeterministic progress of threads, so it's possible to have a truly benign race condition, so long as the correctness of the whole program doesn't depend on the outcome of the race. I/O causes nondeterminism anyway, so it's not really practical to avoid all ordinary races.
    - benschulz 6y ago
      It's possible to have truly benign data races too, e.g. lazily computing a hash of immutable data. One of the reasons that Rust's ownership model is necessary is most data races cannot otherwise be statically detected. The compiler will be unable to apply any of its theoretical UB optimizations. It will emit its native code and you get the defined behavior of the underlying hardware. There will be no undefined behavior. If that ever changes, the bigger news, I imagine, will be that we solved the halting problem.
  - anp 6y ago
    "This context" is the original article, which is discussing data races specifically.
    - benschulz 6y ago
      Actually "this context" was my previous reply. Much like I can nest a lexical scope inside a program and have it reach up and past the containing scope, I can create a new context in a discussion. Please at least try to understand my position before you reply.
- rurban 6y ago
  And deadlocks
volta83 6y ago
You can also create races in safe Rust by using the file system, no concurrency primitives necessary (just create a new file, close it, and re-open it assuming it's still there..). What safe Rust doesn't have is _data races_.

nyanpasu64 6y ago

The thing is, the linked data race occurred in safe code! Looking at https://bugzilla.mozilla.org/show_bug.cgi?id=1686158 https://bugzilla.mozilla.org/show_bug.cgi?id=1686158, the patch changes a non-atomic read through a &SwCompositeGraphNode and a subsequent non-atomic write through a &mut SwCompositeGraphNode (both safe code). They should not race, and the only reason they do is because there wasn't proper RwLock-style synchronization when handing out &SwCompositeGraphNode and &mut SwCompositeGraphNode. Also I think merely creating an unsynchronized &mut is undefined behavior in of itself (in addition to allowing UB in safe code). If so, even the fixed code is still technically wrong (just not miscompiled or misbehaving in practice). Looking at https://searchfox.org/mozilla-central/rev/ee9dab6aa95f167a34cb178960f7375210a0bba4/gfx/wr/webrender/src/compositor/sw_compositor.rs https://searchfox.org/mozilla-central/rev/ee9dab6aa95f167a34..., SwCompositeGraphNodeRef has "safe" methods (line 286) with unsafe blocks, handing out raw &SwCompositeGraphNode and &mut SwCompositeGraphNode to the interior of an UnsafeCell, without locking or runtime checking. I think SwCompositeGraphNodeRef "caused" the UB because it was invoked by some other caller to create a &mut SwCompositeGraphNode without proper synchronization... And worse yet, SwCompositeGraphNodeRef has Deref and DerefMut implementations that implicitly create & and &mut, unsynchronized, at call sites that merely look like method calls... My theory as to how this happened: - C++ atomics are not mutable through a const *, so the authors used &mut to indicate mutability (even though Rust's &mut has stronger guarantees, according to the Stacked Borrows memory model, "all other pointers to this object are invalidated, except the pointers we reborrowed from"... though Stacked Borrows may change to make async fn and intrusive linked lists sound, I suspect it won't make this code sound). - Maybe they thought "we want to prevent calling &mut SwCompositeGraphNode methods from a &SwCompositeGraphNodeRef". IDK. Semi-related: SwCompositeGraphNodeRef is a #[derive(Clone)] Arc<UnsafeCell<SwCompositeGraphNode>> but !Sync, so only threads that own a SwCompositeGraphNodeRef can create either &SwCompositeGraphNodeRef or &mut SwCompositeGraphNodeRef. And I don't see &SwCompositeGraphNodeRef floating around, so I don't see why access control would matter. (EDIT: The commit introducing the unsound abstraction is https://hg.mozilla.org/integration/autoland/rev/8d47e408c578 https://hg.mozilla.org/integration/autoland/rev/8d47e408c578, filed under the bug at https://bugzilla.mozilla.org/show_bug.cgi?id=1670328 https://bugzilla.mozilla.org/show_bug.cgi?id=1670328.)