Skip to content

Hackily fix thread safety issues in dynamic_lib #9713

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

Merged
merged 1 commit into from
Oct 5, 2013

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented Oct 4, 2013

The root issue is that dlerror isn't reentrant or even thread safe.

The solution implemented here is to make a yielding spin lock over an
AtomicFlag. This is pretty hacky, but the best we can do at this point.
As far as I can tell, it isn't possible to create a global mutex without
having to initialize it in a single threaded context.

The Windows code isn't affected since errno is thread-local on Windows
and it's running in an atomically block to ensure there isn't a green
thread context switch.

Closes #8156

@alexcrichton
Copy link
Member

Similarly to #9194, I'm hesitant to include more half-baked implementations into libstd when we've got enough of a half-baked runtime already. I don't think that this is a critical problem right now which needs to be fixed as soon as possible. This certainly needs to be fixed for this module to even be considered to move outside of unstable, however.

I do think that statically initialized mutexes would be a useful thing to have, and this is certainly a good place where they'd be nice to have.

That being said, this isn't the worst thing ever, so if there are indeed good reasons to get this patched over for now, I think that we could include it.

@brson
Copy link
Contributor

brson commented Oct 4, 2013

Other code that needs a global lock does it by adding that lock along with _lock and _unlock functions to rust_builtin.cpp. I would rather do that than hand-roll spinlocks.

@sfackler
Copy link
Member Author

sfackler commented Oct 4, 2013

Does that look better?

@@ -199,6 +197,8 @@ pub mod dl {

#[link_name = "dl"]
extern {
fn rust_take_dlerror_lock();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a little odd to put these functions in the link_name = "dl" block because that's not the library that they come from. I'd be ok with scoping the dlerror_lock functions to just the relevant function that uses them as well.

@sfackler
Copy link
Member Author

sfackler commented Oct 5, 2013

Updated

@alexcrichton
Copy link
Member

r+, thanks!

@alexcrichton
Copy link
Member

I think this just needs an update of rustrt.def.in

@sfackler
Copy link
Member Author

sfackler commented Oct 5, 2013

Should work now.

@sfackler
Copy link
Member Author

sfackler commented Oct 5, 2013

Oops, one more try c.c

The root issue is that dlerror isn't reentrant or even thread safe.

The Windows code isn't affected since errno is thread-local on Windows
and it's running in an atomically block to ensure there isn't a green
thread context switch.

Closes rust-lang#8156
bors added a commit that referenced this pull request Oct 5, 2013
The root issue is that dlerror isn't reentrant or even thread safe.

The solution implemented here is to make a yielding spin lock over an
AtomicFlag. This is pretty hacky, but the best we can do at this point.
As far as I can tell, it isn't possible to create a global mutex without
having to initialize it in a single threaded context.

The Windows code isn't affected since errno is thread-local on Windows
and it's running in an atomically block to ensure there isn't a green
thread context switch.

Closes #8156
@bors bors closed this Oct 5, 2013
@bors bors merged commit 1d19ad9 into rust-lang:master Oct 5, 2013
@sfackler sfackler deleted the dynamic_lib branch May 15, 2014 05:03
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.

dynamic_lib misuses task::atomically in the check_for_errors_in implementations
4 participants