Skip to content

RegexSet references cannot be passed across unwind boundaries #576

Description

@bdonlan

The following minimal test case fails on regex 1.1.6:

fn main() {
    let rs = regex::RegexSet::new(vec![""]).unwrap();
    let rrs = &rs;
    std::panic::catch_unwind(move || rrs.matches(""));
}

with the following error output:

error[E0277]: the type `std::cell::UnsafeCell<std::option::Option<std::boxed::Box<std::cell::RefCell<regex::exec::ProgramCacheInner>>>>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
 --> src/main.rs:4:5
  |
4 |     std::panic::catch_unwind(move || rrs.matches(""));
  |     ^^^^^^^^^^^^^^^^^^^^^^^^ `std::cell::UnsafeCell<std::option::Option<std::boxed::Box<std::cell::RefCell<regex::exec::ProgramCacheInner>>>>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
  |
  = help: within `regex::re_set::unicode::RegexSet`, the trait `std::panic::RefUnwindSafe` is not implemented for `std::cell::UnsafeCell<std::option::Option<std::boxed::Box<std::cell::RefCell<regex::exec::ProgramCacheInner>>>>`
  = note: required because it appears within the type `thread_local::CachedThreadLocal<std::cell::RefCell<regex::exec::ProgramCacheInner>>`
  = note: required because it appears within the type `regex::exec::Exec`
  = note: required because it appears within the type `regex::re_set::unicode::RegexSet`
  = note: required because of the requirements on the impl of `std::panic::UnwindSafe` for `&regex::re_set::unicode::RegexSet`
  = note: required because it appears within the type `[closure@src/main.rs:4:30: 4:53 rrs:&regex::re_set::unicode::RegexSet]`
  = note: required by `std::panic::catch_unwind`

error[E0277]: the type `std::cell::UnsafeCell<regex::exec::ProgramCacheInner>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
 --> src/main.rs:4:5
  |
4 |     std::panic::catch_unwind(move || rrs.matches(""));
  |     ^^^^^^^^^^^^^^^^^^^^^^^^ `std::cell::UnsafeCell<regex::exec::ProgramCacheInner>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
  |
  = help: within `regex::re_set::unicode::RegexSet`, the trait `std::panic::RefUnwindSafe` is not implemented for `std::cell::UnsafeCell<regex::exec::ProgramCacheInner>`
  = note: required because it appears within the type `std::cell::RefCell<regex::exec::ProgramCacheInner>`
  = note: required because it appears within the type `std::marker::PhantomData<std::cell::RefCell<regex::exec::ProgramCacheInner>>`
  = note: required because it appears within the type `thread_local::ThreadLocal<std::cell::RefCell<regex::exec::ProgramCacheInner>>`
  = note: required because it appears within the type `thread_local::CachedThreadLocal<std::cell::RefCell<regex::exec::ProgramCacheInner>>`
  = note: required because it appears within the type `regex::exec::Exec`
  = note: required because it appears within the type `regex::re_set::unicode::RegexSet`
  = note: required because of the requirements on the impl of `std::panic::UnwindSafe` for `&regex::re_set::unicode::RegexSet`
  = note: required because it appears within the type `[closure@src/main.rs:4:30: 4:53 rrs:&regex::re_set::unicode::RegexSet]`
  = note: required by `std::panic::catch_unwind`

error[E0277]: the type `std::cell::UnsafeCell<isize>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
 --> src/main.rs:4:5
  |
4 |     std::panic::catch_unwind(move || rrs.matches(""));
  |     ^^^^^^^^^^^^^^^^^^^^^^^^ `std::cell::UnsafeCell<isize>` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
  |
  = help: within `regex::re_set::unicode::RegexSet`, the trait `std::panic::RefUnwindSafe` is not implemented for `std::cell::UnsafeCell<isize>`
  = note: required because it appears within the type `std::cell::Cell<isize>`
  = note: required because it appears within the type `std::cell::RefCell<regex::exec::ProgramCacheInner>`
  = note: required because it appears within the type `std::marker::PhantomData<std::cell::RefCell<regex::exec::ProgramCacheInner>>`
  = note: required because it appears within the type `thread_local::ThreadLocal<std::cell::RefCell<regex::exec::ProgramCacheInner>>`
  = note: required because it appears within the type `thread_local::CachedThreadLocal<std::cell::RefCell<regex::exec::ProgramCacheInner>>`
  = note: required because it appears within the type `regex::exec::Exec`
  = note: required because it appears within the type `regex::re_set::unicode::RegexSet`
  = note: required because of the requirements on the impl of `std::panic::UnwindSafe` for `&regex::re_set::unicode::RegexSet`
  = note: required because it appears within the type `[closure@src/main.rs:4:30: 4:53 rrs:&regex::re_set::unicode::RegexSet]`
  = note: required by `std::panic::catch_unwind`

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0277`.
error: Could not compile `testapp`.

To learn more, run the command again with --verbose.

Activity

  1. bdonlan commented on Apr 18, 2019

    @bdonlan
    Author

    Note that this is an issue for Regex types as well, not just RegexSet.

  2. BurntSushi commented on Apr 18, 2019

    @BurntSushi
    Member

    Did this use to work? (I don't think it did.) So are you reported this as a feature request?

    It's not immediately obvious to me that this should work. But it's probably okay for regex internals to assert that it is unwind safe since interior mutability is never used for correctness, only performance.

  3. bdonlan commented on Apr 19, 2019

    @bdonlan
    Author

    I'm not sure if this was supported in the past; it was a bit of a sharp edge that bit me, as I'd assume normally that immutable objects would be unwind safe. If the interior mutability is only used for caching, perhaps the safest approach would be to drop the cache if a panic occurs inside the regex engine?

  4. rtkaratekid commented on Aug 31, 2020

    @rtkaratekid

    Just wanted to chime in that when using libfuzz or afl.rs this is also an issue because you need std::panic::RefUnwindSafe implemented for objects within the closure. If you're trying to fuzz something that involves regex matching, you just wont be able to (at least from my rust newbie perspective).

  5. dpathakj commented on Feb 27, 2021

    @dpathakj

    The situation may have changed here because of changes upstream in thread_local. The statement in the #[cfg(not(feature = "perf-cache"))] impl of Cached<T> states that "CachedThreadLocal impls Send, Sync and UnwindSafe, but NOT RefUnwindSafe." That doesn't appear to be the case on tip of master, because (Cached)ThreadLocal is now RefUnwindSafe. If you remove that PhantomData you'll get an implementation that passes the testcase above regardless of whether perf-cache is enabled. (Undoing f6f8276 breaks it again.) Should the PhantomData be removed or is there another reason specific to this crate that it should remain?

    (I noticed this because the build is broken due to a warning. CachedThreadLocal is deprecated and now just passes through to ThreadLocal. That relates to this issue because the fix (just use ThreadLocal) should probably also adjust the comment, but just replacing "CachedThreadLocal" with "ThreadLocal" in the comment doesn't seem to make it right.)

  6. BurntSushi commented on Mar 1, 2021

    @BurntSushi
    Member

    @dpathakj Yeah, I'm working on addressing this. I have a newborn at home though, so it's tough. Basically, my plan here is to actually remove the thread_local dependency. I've had this thought for a while since it is the root cause of some use cases resulting in bad memory leaks. Specifically, what happens is when a regex is used across lots of different threads and where (I suppose) some of those threads become idle but not actually destroyed, each thread winds up having a copy mutable scratch space used during matching. There have been a few issues about it I think. This one comes to mind: BurntSushi/rure-go#3

    The main problem with removing the dependency is the reason why thread_local exists in the first place: it's really fast. So I need to do a bit of a benchmark investigation to see what I can do to replace it without sacrificing too much performance. Ideally without using any unsafe code, since the sort of unsafe required to make thread_local work is really quite tricky from my perspective and I don't really trust myself to get it right without a lot of deep thought and effort that I don't really have time for at the moment.

  7. added a commit that references this issue on Mar 12, 2021
    1440041
  8. added a commit that references this issue on Mar 12, 2021
    e040c1b
  9. BurntSushi commented on Mar 12, 2021

    @BurntSushi
    Member

    This should be fixed in regex 1.4.4. All regex types now impl RefUnwindSafe.

  10. added a commit that references this issue on Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions