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

Print stack overflow messages for Linux and OS X #16388

Merged
merged 2 commits into from
Oct 24, 2014
Merged

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Aug 9, 2014

This installs signal handlers to print out stack overflow messages on Linux. It also ensures the main thread has a guard page.

This will catch stack overflows in external code. It's done in preparation of switching to stack probes (#16012).

I've done some simple tests with overflowing the main thread, native threads and green threads (with and without UV) on x86-64.
This might work on ARM, MIPS and x86-32.

I've been unable to run the test suite on this because of #16305.

@huonw
Copy link
Member

huonw commented Aug 10, 2014

Is there a particular reason this can't be in Rust?

@alexcrichton
Copy link
Member

Yes, I believe this should all largely be written in rust rather than in C (other signal handlers are already written in rust). I'm also hesitant to land linux-only support for this feature. I would much rather implement everything in one sweep than be in a confusing state for awhile while we're waiting on the next few patches.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 10, 2014

I wrote it in C since I don't find bindings for signal things. I see there's some in libnative (why are those there?), but that's still missing sigaltstack, SIGSTKSZ, stack_t, siginfo_t.si_addr.

Isn't rust-bindgen good enough to handle system headers?

@huonw
Copy link
Member

huonw commented Aug 10, 2014

You can easily write the necessary bindings (or extend the ones that exist already), e.g.

struct stack_t {
    ss_sp: *mut libc::c_void,
    ss_flags: libc::c_int,
    ss_size: libc::size_t    
}
extern {
    fn sigaltstack(ss: *const stack_t, oss: *mut stack_t) -> libc::c_int
}

Isn't rust-bindgen good enough to handle system headers?

The in-tree extern bindings are hand-written, not created with rust-bindgen.

@pcwalton
Copy link
Contributor

I'm fine with making this Linux-only for now, as I prefer things to be in
tree when possible to reduce rebase pain.
On Aug 9, 2014 8:50 PM, "Alex Crichton" notifications@github.com wrote:

Yes, I believe this should all largely be written in rust rather than in C
(other signal handlers are already written in rust). I'm also hesitant to
land linux-only support for this feature. I would much rather implement
everything in one sweep than be in a confusing state for awhile while we're
waiting on the next few patches.


Reply to this email directly or view it on GitHub
#16388 (comment).

@Zoxc Zoxc changed the title Print stack overflow messages for Linux Print stack overflow messages for Linux and OS X Aug 11, 2014
@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 11, 2014

This works on Linux and OS X now.

@@ -249,6 +248,7 @@ mod signal {
pub sa_flags: libc::c_int,
}

#[cfg(target_os = "macos")]
Copy link
Member

Choose a reason for hiding this comment

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

My system (OSX 10.9) has this definition:

struct  __sigaction {                                                 
        union __sigaction_u __sigaction_u;  /* signal handler */      
        void    (*sa_tramp)(void *, int, int, siginfo_t *, void *);   
        sigset_t sa_mask;               /* signal mask to apply */    
        int     sa_flags;               /* see signal options below */
};                                                                    

How come you changed it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's __sigaction not sigaction.

Copy link
Member

Choose a reason for hiding this comment

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

Aha! I should remind myself that I have eyes! I suspect that ios is the same in this regard in that case and the upper one should probably just be deleted.

@alexcrichton
Copy link
Member

I'll try to take a closer look at this soon (sadly not before next week), but how much of an effort do you think this will be to extend this strategy to windows? I would hate to commit to a unix-only design only to find out later that it isn't compatible with windows for one reason or another. You may also want to download the arm/android toolchain as you should be able to just tweak a static here or there to get support. If you have a list of missing statics as well, I can look those up for you.

@Zoxc
Copy link
Contributor Author

Zoxc commented Aug 13, 2014

This should be simpler on Windows (with the exception of green threads which would need the same handling as Unix). However it Windows does not install guard pages on threads created by CreateThread, nor will it call the vectored exception handler I installed. I think we're doing something bad in the runtime somewhere (excluding writing to fs:0x14).

ops.stack_guard = rt::thread::main_guard_page();
task.put_runtime(ops);
return task;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not add a new public function for this, and this seems like something that callers of new need to worry about, so could you add an Option<uint> parameter to new instead?

@alexcrichton
Copy link
Member

Also, can you add tests for all the functionality added here? You should be able to just spawn a subprocess and then inspect the output to ensure that the handlers were triggered correctly. Some tests that should be written:

  • Native overflow in C code
  • Green overflow in C code
  • Native segfault (not an overflow)
  • Green segfault (not an overflow)

You can ignore the test for unimplemented platforms for now.

@alexcrichton
Copy link
Member

Another idea I thought of today is that you may be able to remove a good portion of the guard page logic by using stack::get_sp_limit() + stack::RED_ZONE for testing whether a fault is near a guard page or not.

@Zoxc Zoxc force-pushed the stmesg branch 2 times, most recently from 4dade58 to f56cef3 Compare October 23, 2014 05:30
@Zoxc
Copy link
Contributor Author

Zoxc commented Oct 23, 2014

This passes tests on try now (with the exception of BSD, which I hopefully fixed)

@Zoxc
Copy link
Contributor Author

Zoxc commented Oct 24, 2014

I see you test for Android too, so I added (untested) support for it.

@Zoxc
Copy link
Contributor Author

Zoxc commented Oct 24, 2014

All Android test fails with error: undefined reference to 'signal'. Any idea what's missing?

@Zoxc
Copy link
Contributor Author

Zoxc commented Oct 24, 2014

I'm just disabling Android for now. We can fix that later.

@alexcrichton
Copy link
Member

Sure, we can land this for now with android disabled. Could you open an issue on enabling for android @Zoxc?

FYI this is the definition in the header file:

extern __sighandler_t bsd_signal(int, __sighandler_t);          

/* the default is bsd */                                        
static __inline__ __sighandler_t signal(int s, __sighandler_t f)
{                                                               
    return bsd_signal(s,f);                                     
}                                                               

bors added a commit that referenced this pull request Oct 24, 2014
This installs signal handlers to print out stack overflow messages on Linux. It also ensures the main thread has a guard page.

This will catch stack overflows in external code. It's done in preparation of switching to stack probes (#16012).

I've done some simple tests with overflowing the main thread, native threads and green threads (with and without UV) on x86-64.
This might work on ARM, MIPS and x86-32.

I've been unable to run the test suite on this because of #16305.
@bors bors closed this Oct 24, 2014
@bors bors merged commit 70cef94 into rust-lang:master Oct 24, 2014
geofft added a commit to geofft/rust that referenced this pull request May 25, 2015
Both c.rs and stack_overflow.rs had bindings of libc's signal-handling
routines. It looks like the split dated from rust-lang#16388, when (what is now)
c.rs was in libnative but not libgreen. Nobody is currently using the
c.rs bindings, but they're a bit more accurate in some places.

Move everything to c.rs (since I'll need signal handling in process.rs,
and we should avoid duplication), clean up the bindings, and manually
double-check everything against the relevant system headers (fixing a
few things in the process).

Between the last commit and this one, we can now drop `#![allow(dead_code)]`
from c.rs, which should help avoid binding rot in the future.
geofft added a commit to geofft/rust that referenced this pull request May 25, 2015
Both c.rs and stack_overflow.rs had bindings of libc's signal-handling
routines. It looks like the split dated from rust-lang#16388, when (what is now)
c.rs was in libnative but not libgreen. Nobody is currently using the
c.rs bindings, but they're a bit more accurate in some places.

Move everything to c.rs (since I'll need signal handling in process.rs,
and we should avoid duplication), clean up the bindings, and manually
double-check everything against the relevant system headers (fixing a
few things in the process).

Between the last commit and this one, we can now drop `#![allow(dead_code)]`
from c.rs, which should help avoid binding rot in the future.
geofft added a commit to geofft/rust that referenced this pull request Jun 20, 2015
Both c.rs and stack_overflow.rs had bindings of libc's signal-handling
routines. It looks like the split dated from rust-lang#16388, when (what is now)
c.rs was in libnative but not libgreen. Nobody is currently using the
c.rs bindings, but they're a bit more accurate in some places.

Move everything to c.rs (since I'll need signal handling in process.rs,
and we should avoid duplication), clean up the bindings, and manually
double-check everything against the relevant system headers (fixing a
few things in the process).
geofft added a commit to geofft/rust that referenced this pull request Jun 22, 2015
Both c.rs and stack_overflow.rs had bindings of libc's signal-handling
routines. It looks like the split dated from rust-lang#16388, when (what is now)
c.rs was in libnative but not libgreen. Nobody is currently using the
c.rs bindings, but they're a bit more accurate in some places.

Move everything to c.rs (since I'll need signal handling in process.rs,
and we should avoid duplication), clean up the bindings, and manually
double-check everything against the relevant system headers (fixing a
few things in the process).
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2024
internal: Record FnAbi

This unfortunately breaks our lub coercions, so will need to look into fixing that first, though I am not sure what is going wrong where...

Stubbed some stuff out for the time being.
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.

7 participants