Skip to content
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

ReentrantLockGuard's Sync impl is unsound #125526

Closed
programmerjake opened this issue May 24, 2024 · 2 comments · Fixed by #125527
Closed

ReentrantLockGuard's Sync impl is unsound #125526

programmerjake opened this issue May 24, 2024 · 2 comments · Fixed by #125527
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@programmerjake
Copy link
Member

programmerjake commented May 24, 2024

I tried running this code in miri:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=4b45c8a022255be4404228c01922b632

#![feature(reentrant_lock)]
use std::cell::Cell;
use std::sync::ReentrantLock;
use std::thread::scope;

fn main() {
    let l = ReentrantLock::new(Cell::new(0u8));
    let lg = l.lock();
    scope(|s| {
        s.spawn(|| dbg!(lg.get()));
        lg.set(1);
    });
}

I expected to see this happen: compile error

Instead, this happened: compiled successfully and miri reported a data race

this is because the automatic impl<T: Send> Sync for ReentrantLockGuard<T> is incorrect:
https://doc.rust-lang.org/1.78.0/std/sync/struct.ReentrantLockGuard.html#impl-Sync-for-ReentrantLockGuard%3C'a,+T%3E

ReentrantLock's tracking issue: #121440

@programmerjake programmerjake added the C-bug Category: This is a bug. label May 24, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 24, 2024
@programmerjake
Copy link
Member Author

@rustbot label I-unsound

@rustbot rustbot added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 24, 2024
@programmerjake
Copy link
Member Author

@rustbot label requires-nightly

@rustbot rustbot added the requires-nightly This issue requires a nightly compiler in some way. label May 24, 2024
programmerjake added a commit to programmerjake/rust that referenced this issue May 24, 2024
@saethlin saethlin added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 24, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this issue May 25, 2024
…ubilee

Add manual Sync impl for ReentrantLockGuard

Fixes: rust-lang#125526
Tracking Issue: rust-lang#121440

this impl is even shown in the summary in the tracking issue, but apparently was forgotten in the actual implementation
workingjubilee added a commit to workingjubilee/rustc that referenced this issue May 25, 2024
…ubilee

Add manual Sync impl for ReentrantLockGuard

Fixes: rust-lang#125526
Tracking Issue: rust-lang#121440

this impl is even shown in the summary in the tracking issue, but apparently was forgotten in the actual implementation
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 25, 2024
…ubilee

Add manual Sync impl for ReentrantLockGuard

Fixes: rust-lang#125526
Tracking Issue: rust-lang#121440

this impl is even shown in the summary in the tracking issue, but apparently was forgotten in the actual implementation
@bors bors closed this as completed in f4b9ac6 May 25, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue May 25, 2024
Rollup merge of rust-lang#125527 - programmerjake:patch-2, r=workingjubilee

Add manual Sync impl for ReentrantLockGuard

Fixes: rust-lang#125526
Tracking Issue: rust-lang#121440

this impl is even shown in the summary in the tracking issue, but apparently was forgotten in the actual implementation
c4rrao pushed a commit to Vudvud/rust that referenced this issue May 25, 2024
c4rrao pushed a commit to Vudvud/rust that referenced this issue May 25, 2024
Add a fast-path to `Debug` ASCII `&str`

Instead of going through the `EscapeDebug` machinery, we can just skip over ASCII chars that don’t need any escaping.

Introduce printable-ASCII fast-path for `impl Debug for str`

Instead of having a single loop that works on utf-8 `char`s,
this splits the implementation into a loop that quickly skips over
printable ASCII, falling back to per-char iteration for other chunks.

Switch to primarily using `&str`

Surprisingly, benchmarks have shown that using `&str`
instead of `&[u8]` with some `unsafe` code is actually faster.

Process a single not-ASCII-printable `char` per iteration

This avoids having to collect a non-ASCII-printable run before processing it.

std: simplify key-based thread locals

std: clean up the TLS implementation

Make clamp inline

Run rustfmt on files that need it.

Somehow these files aren't properly formatted. By default `x fmt` and `x
tidy` only check files that have changed against master, so if an
ill-formatted file somehow slips in it can stay that way as long as it
doesn't get modified(?)

I found these when I ran `x fmt` explicitly on every `.rs` file in the
repo, while working on
rust-lang/compiler-team#750.

Fix the dead link in the bootstrap README

Notify kobzol after changes to `opt-dist`

Revert "Rollup merge of rust-lang#123979 - oli-obk:define_opaque_types7, r=compiler-errors"

This reverts commit f939d1f, reversing
changes made to 183c706.

Add regression tests

Only suppress binop error in favor of semicolon suggestion if we're in an assignment statement

compiler: const_eval/transform/validate.rs -> mir_transform/validate.rs

compiler: unnest rustc_const_eval::check_consts

clippy: unnest check_consts

miri: receive the blessings of validate.rs

Migrate `run-make/rustdoc-with-output-dir-option` to `rmake.rs`

Fix some SIMD intrinsics documentation

Actually just remove the special case altogether

rustdoc-json: Add test for keywords with `--document-private-items`

tag more stuff with `WG-trait-system-refactor`

Warn/error on self ctor from outer item in inner item

(Mostly) revert "Account for type param from other item in `note_and_explain`"

This mostly reverts commit 7449478.
It also removes an `opt_param_at` that really is unnecessary given our
ICE policy for malformed intrinsics.

Update cargo

use posix_memalign on most Unix targets

fix typo

Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>

Fail relating constants of different types

Use regular type equating instead of a custom query

Bump bootstrap compiler to the latest beta compiler

Remove now outdated comment since we bumped stage0

Stop using the avx512er and avx512pf x86 target features

They are no longer supported by LLVM 19.

Fixes rust-lang#125492

remove proof tree formatter, make em shallow

Don't eagerly monomorphize drop for types that are impossible to instantiate

Better ICE message for unresolved upvars

Structurally resolve before builtin_index in EUV

Add manual Sync impl for ReentrantLockGuard

Fixes: rust-lang#125526
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness requires-nightly This issue requires a nightly compiler in some way. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants