Repository navigation
RegexSet references cannot be passed across unwind boundaries #576
Description
Activity
Note that this is an issue for Regex types as well, not just RegexSet.
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.
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?
- added 3 commits that reference this issue
on Sep 2, 2019 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).
The situation may have changed here because of changes upstream in thread_local. The statement in the
#[cfg(not(feature = "perf-cache"))]impl ofCached<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 whetherperf-cacheis 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.)
@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_localdependency. 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#3The main problem with removing the dependency is the reason why
thread_localexists 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 anyunsafecode, since the sort ofunsaferequired to makethread_localwork 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.- added a commit that references this issue
on Mar 12, 2021 - added a commit that references this issue
on Mar 12, 2021 This should be fixed in
regex 1.4.4. All regex types now implRefUnwindSafe.
The following minimal test case fails on regex 1.1.6:
with the following error output: