Retry on EINVAL from pthread_attr_setstacksize() #11885

bnoordhuis · 2014-01-28T18:22:59Z

EINVAL means that the requested stack size is either not a multiple
of the system page size or that it's smaller than PTHREAD_STACK_MIN.
Figure out what the case is, fix it up and retry. If it still fails,
give up, like before.

Suggestions for future improvements:

don't fail!() but instead signal a condition, or
silently ignore the error and use a default sized stack.

Fixes #11694.

The first two commits put the framework in place, the third one contains the meat.

alexcrichton · 2014-01-28T19:23:00Z

This looks great, thanks!

We're generally trying to remove conditions, and I'd rather fail for now instead of silently going to the default stack size. We can see what this buys us down the road, but I would imagine that you've covered most use cases with rounding/clamping.

One thing that I also wanted to take care of here was dealing with the std::unstable::stack::RED_ZONE constant. Right now a stack requests N bytes of stack, but it is considered as having overflowed the stack when it consumes (N - RED_ZONE) bytes of stack. This red zone is used to allow the stack-overflow code to run, and also in theory allow some user destructors to run (run during unwinding). We currently don't make much use of it (except for printing that you've overflowed), but it should probably get factored into these computations.

What'll happen now is that if you request a stack size of 4096 you'll immediately overflow (probably in some runtime code) and trigger a runtime abort. Could you alter the min_stack_size() return value to also add in the RED_ZONE constant? Something like that at least. The idea would be to always allow at least some code to run regardless of the stack size provided.

Does that make sense? I may have wandered off a bit on some tangents...

This is also is a problem for allocating green stacks, but that can be dealt with in a separate commit.

bnoordhuis · 2014-01-28T19:34:29Z

think this might have a better home in std::rt::thread for now. I'm not sure how relevant it is outside of the thread-spawning context, but we should in general probably try to slim down the std::os api regardless (an issue for another time).

No problem, I'll move it.

This could use os::last_os_error() to avoid having to deal with str.

Cheers, I was looking for something like that.

We're generally trying to remove conditions

I'll update the commit log.

Right now a stack requests N bytes of stack, but it is considered as having overflowed the stack when it consumes (N - RED_ZONE) bytes of stack.

I'll update it to to take the red zone into account. That makes RED_ZONE + PTHREAD_MIN_STACK the absolute lower limit, right?

alexcrichton · 2014-01-28T19:35:38Z

That makes RED_ZONE + PTHREAD_MIN_STACK the absolute lower limit, right?

Yeah, you may have to make the RED_ZONE static public, but that's fine.

brson · 2014-01-29T09:42:00Z

Thanks!

huonw · 2014-01-29T10:48:30Z

Does this fix #6233 too?

bnoordhuis · 2014-01-29T19:21:03Z

Updated. PTAL?

Does this fix #6233 too?

It didn't but as of bnoordhuis/rust@b09dd90 it does.

alexcrichton · 2014-01-29T19:33:35Z

src/libstd/rt/thread.rs

        assert_eq!(pthread_attr_setdetachstate(&mut attr,
                                               PTHREAD_CREATE_JOINABLE), 0);

+        // Reserve room for the red zone, the runtime's stack of last resort.
+        let stack_size = cmp::max(stack, RED_ZONE + __pthread_get_minstack(&attr) as uint);


I'd be a little uncomfortable going through dlopen/dlsym/dlclose for all created threads. This would mean that all native task spawns would take these hits.

I'm not entirely sure, but is there a difference between PTHREAD_STACK_MIN and __pthread_get_minstack?

That being said, I would be very comfortable merging this without fixing #6233 just yet, but I would imagine that a solution to #6233 would likely involve using dlsym to figure out if you can actually call it.

I'd be a little uncomfortable going through dlopen/dlsym/dlclose for all created threads. This would mean that all native task spawns would take these hits.

I suppose it would be possible to cache the result somewhere though that could blow up if someone gets creative with unloading libpthread.so, then loading it again..

I'm not entirely sure, but is there a difference between PTHREAD_STACK_MIN and __pthread_get_minstack?

The return value of __pthread_get_minstack() is PTHREAD_STACK_MIN plus whatever glibc needs for thread-local storage (which is not per se a fixed quantity.)

That being said, I would be very comfortable merging this without fixing #6233 just yet, but I would imagine that a solution to #6233 would likely involve using dlsym to figure out if you can actually call it.

I think so. You can't rely on __pthread_get_minstack() being there, it was added in glibc 2.15. Most distros ship older versions.

Alright, let's merge this and then we can see how big the impact is.

alexcrichton · 2014-01-29T19:34:44Z

This'll also need a rebase on top of the current master before merging.

bnoordhuis · 2014-01-29T20:34:50Z

Rebased.

alexcrichton · 2014-01-29T21:45:41Z

src/libstd/rt/thread.rs

+        use option;
+        use result::{Err, Ok};
+        use unstable::dynamic_lib;
+        match dynamic_lib::DynamicLibrary::open(option::None) {


Stylistically, it'd be better to import option::None and then use None here.

bnoordhuis · 2014-01-30T16:03:13Z

Updated and rebased. I've added a comment about how the dlopen/dlsym/dlclose hit can be avoided if/when Rust grows weak symbol support. (Assuming it's not already there; I couldn't find anything to that effect.)

bnoordhuis · 2014-01-31T02:45:58Z

@alexcrichton Are the failures related to my changes? It doesn't look like it but...

huonw · 2014-01-31T02:48:04Z

It's failed twice in the same way, which seems suspicious. Once more for luck.

alexcrichton · 2014-01-31T03:53:33Z

This is suspicious, and I don't understand why this is happening. I'm building locally in hope that I can reproduce it

alexcrichton · 2014-01-31T05:03:19Z

Hm, I couldn't reproduce this at stage0/1/2 on my local mac. I've seen a number of "failure to unwind" failures on the bots recently, so this may just be unlucky.

alexcrichton · 2014-01-31T05:22:47Z

The flakiness may be fixed by #11947

Represents the minimum size of a thread's stack. As such, it's both platform and architecture-specific. I put it under posix01 even though it predates POSIX.1-2001 by some years. I believe it was first formalized in SUSv2. I doubt anyone cares, though.

Enforce that the stack size is > RED_ZONE + PTHREAD_STACK_MIN. If the call to pthread_attr_setstacksize() subsequently fails with EINVAL, it means that the platform requires the stack size to be a multiple of the page size. In that case, round up to the nearest page and retry. Fixes #11694.

glibc >= 2.15 has a __pthread_get_minstack() function that returns PTHREAD_STACK_MIN plus however many bytes are needed for thread-local storage. Use it when it's available because just PTHREAD_STACK_MIN is not enough in applications that have big thread-local storage requirements. Fixes #6233.

bnoordhuis · 2014-01-31T13:59:28Z

Rebased against master. I can confirm that i686-darwin-apple works okay locally for me. Can you give it one more try?

EINVAL means that the requested stack size is either not a multiple of the system page size or that it's smaller than PTHREAD_STACK_MIN. Figure out what the case is, fix it up and retry. If it still fails, give up, like before. Suggestions for future improvements: * don't fail!() but instead signal a condition, or * silently ignore the error and use a default sized stack. Fixes #11694. The first two commits put the framework in place, the third one contains the meat.

alexcrichton reviewed Jan 29, 2014
View reviewed changes

bnoordhuis added 3 commits January 31, 2014 13:47

bors closed this Feb 1, 2014

bors merged commit 431edac into rust-lang:master Feb 1, 2014

bnoordhuis deleted the issue11694 branch February 1, 2014 11:03

bnoordhuis mentioned this pull request Aug 8, 2014

CI security nodejs/build#3

Closed

andersk mentioned this pull request Mar 23, 2015

Use of __pthread_get_minstack leads to unnecessarily strict libc6 versioned dependency on Debian #23628

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry on EINVAL from pthread_attr_setstacksize() #11885

Retry on EINVAL from pthread_attr_setstacksize() #11885

bnoordhuis commented Jan 28, 2014

alexcrichton commented Jan 28, 2014

bnoordhuis commented Jan 28, 2014

alexcrichton commented Jan 28, 2014

brson commented Jan 29, 2014

huonw commented Jan 29, 2014

bnoordhuis commented Jan 29, 2014

alexcrichton Jan 29, 2014

bnoordhuis Jan 29, 2014

alexcrichton Jan 29, 2014

alexcrichton commented Jan 29, 2014

bnoordhuis commented Jan 29, 2014

alexcrichton Jan 29, 2014

bnoordhuis commented Jan 30, 2014

bnoordhuis commented Jan 31, 2014

huonw commented Jan 31, 2014

alexcrichton commented Jan 31, 2014

alexcrichton commented Jan 31, 2014

alexcrichton commented Jan 31, 2014

bnoordhuis commented Jan 31, 2014

Retry on EINVAL from pthread_attr_setstacksize() #11885

Retry on EINVAL from pthread_attr_setstacksize() #11885

Conversation

bnoordhuis commented Jan 28, 2014

alexcrichton commented Jan 28, 2014

bnoordhuis commented Jan 28, 2014

alexcrichton commented Jan 28, 2014

brson commented Jan 29, 2014

huonw commented Jan 29, 2014

bnoordhuis commented Jan 29, 2014

alexcrichton Jan 29, 2014

Choose a reason for hiding this comment

bnoordhuis Jan 29, 2014

Choose a reason for hiding this comment

alexcrichton Jan 29, 2014

Choose a reason for hiding this comment

alexcrichton commented Jan 29, 2014

bnoordhuis commented Jan 29, 2014

alexcrichton Jan 29, 2014

Choose a reason for hiding this comment

bnoordhuis commented Jan 30, 2014

bnoordhuis commented Jan 31, 2014

huonw commented Jan 31, 2014

alexcrichton commented Jan 31, 2014

alexcrichton commented Jan 31, 2014

alexcrichton commented Jan 31, 2014

bnoordhuis commented Jan 31, 2014