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

Fix locking soundness in PersistentPlugin #12182

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

devyn
Copy link
Contributor

@devyn devyn commented Mar 12, 2024

Description

There were two problems in PersistentPlugin which could cause a deadlock:

  1. There were two mutexes being used, and get() could potentially hold both simultaneously if it had to spawn. This won't necessarily cause a deadlock on its own, but it does mean that lock order is sensitive

  2. set_gc_config() called flush() while still holding the lock, meaning that the GC thread had to proceed before the lock was released. However, waiting for the GC thread to proceed could mean waiting for the GC thread to call stop(), which itself would try to lock the mutex. So, it's not safe to wait for the GC thread while the lock is held. This is fixed now.

I've also reverted #12177, as @IanManske reported that this was also happening for him on Linux, and it seems to be this problem which should not be platform-specific at all. I believe this solves it.

User-Facing Changes

None

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

There were two problems in `PersistentPlugin` which could cause a
deadlock:

1. There were two mutexes being used, and `get()` could potentially hold
   both simultaneously if it had to spawn. This won't necessarily cause
   a deadlock on its own, but it does mean that lock order is sensitive

2. `set_gc_config()` called `flush()` while still holding the lock,
   meaning that the GC thread had to proceed before the lock was
   released. However, waiting for the GC thread to proceed could mean
   waiting for the GC thread to call `stop()`, which itself would try
   to lock the mutex. So, it's not safe to wait for the GC thread while
   the lock is held. This is fixed now.

I've also reverted nushell#12177, as Ian reported that this was also happening
for him on Linux, and it seems to be this problem which should not be
platform-specific at all.
@fdncred
Copy link
Collaborator

fdncred commented Mar 12, 2024

Thanks for digging in and figuring it out!

@fdncred fdncred merged commit e1cfc96 into nushell:main Mar 12, 2024
15 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Mar 12, 2024

Not that it's related but right after this PR I get this error compiling on macos now. So, back to using --locked :(

error[E0658]: use of unstable library feature 'ptr_from_ref'
   --> /Users/fdncred/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libproc-0.14.5/src/libproc/proc_pid.rs:241:22
    |
241 |     let buffer_ptr = std::ptr::from_mut::<T>(&mut pidinfo).cast::<c_void>();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #106116 <https://github.com/rust-lang/rust/issues/106116> for more information

error[E0658]: use of unstable library feature 'ptr_from_ref'
   --> /Users/fdncred/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libproc-0.14.5/src/libproc/pid_rusage.rs:389:22
    |
389 |     let buffer_ptr = std::ptr::from_mut::<T>(&mut pidrusage).cast::<c_void>();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #106116 <https://github.com/rust-lang/rust/issues/106116> for more information

error[E0658]: use of unstable library feature 'ptr_from_ref'
   --> /Users/fdncred/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libproc-0.14.5/src/libproc/file_info.rs:168:22
    |
168 |     let buffer_ptr = std::ptr::from_mut::<T>(&mut pidinfo).cast::<c_void>();
    |                      ^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #106116 <https://github.com/rust-lang/rust/issues/106116> for more information

@devyn
Copy link
Contributor Author

devyn commented Mar 12, 2024

I haven't made any dependency changes today, so unfortunately I think it's probably upstream again

@devyn
Copy link
Contributor Author

devyn commented Mar 12, 2024

rust-lang/rust#106116

Looks to me like that got stabilized with rustc 1.76.0 fyi, so it's using something newer than it should

@fdncred
Copy link
Collaborator

fdncred commented Mar 12, 2024

yup, agreed, it's upstream. A little aggravating that we're a couple rust releases from that being in stable for us. We try to stay two versions behind.

@devyn
Copy link
Contributor Author

devyn commented Mar 12, 2024

Issue submitted

@WindSoilder
Copy link
Collaborator

@devyn Thank you for tackling all of plugin stuff! It's hard to get it right with bug free at once. I'm happy to see all improvements on plugin system

@hustcer hustcer added this to the v0.92.0 milestone Mar 13, 2024
@devyn devyn deleted the persistentplugin-locking-soundness branch March 15, 2024 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants