ExceptionHandler::SetupHandler passes uninitialized sa_mask to sigaction

[GoogleCodeExporter]

What steps will reproduce the problem?
1. run firefox under valgrind

What is the expected output?
No valgrind warnings related to sigaction.

What do you see instead?
==10434== Syscall param rt_sigaction(act->sa_mask) points to uninitialised
byte(s)
==10434==    at 0xA4CA77: __libc_sigaction (sigaction.c:94)
==10434==    by 0xA4CB62: sigaction (sigaction.c:44)

Please provide any additional information below.

The problem is apparent in
google-breakpad/src/client/linux/handler/exception_handler.cc (present in
SVN r241), where the following code is present:

void ExceptionHandler::SetupHandler(int signo) {
  struct sigaction act, old_act;
  act.sa_handler = HandleException;
  act.sa_flags = SA_ONSTACK;
  if (sigaction(signo, &act, &old_act) < 0)
    return;
  old_handlers_[signo] = old_act.sa_handler;
}

This code should initialize act.sa_mask to something.  A typical way to
initialize it would be the following:

sigset_t mset;
sigemptyset(&mset);
act.sa_mask = mset;

It seems pretty bad that this is uninitialized, as it means that the set of
signals that will be blocked during execution of breakpad's signal handler
could be random or could change based on compiler or compiler option changes.

Original issue reported on code.google.com by ldavidba...@gmail.com on 29 Feb 2008 at 1:21

[GoogleCodeExporter]

I'd suggest something like this; I have NOT compiled or tested it.

Original comment by ldavidba...@gmail.com on 29 Feb 2008 at 1:27

Attachments:

• breakpad-242.diff

[GoogleCodeExporter]

I'd also note that the solaris version of this function in
google-breakpad/src/client/solaris/handler/exception_handler.cc is exactly the 
same;
I have no idea if struct sigaction is the same on Solaris, though.

Original comment by ldavidba...@gmail.com on 29 Feb 2008 at 8:22

[GoogleCodeExporter]

I posted a similar patch in issue 288 ... I did compile that :)

Can someone add it please?

Original comment by craig.sc...@gmail.com on 7 Jan 2009 at 4:41

[GoogleCodeExporter]

Ok I've done some investigating and here is what I've found: the proposed fixes 
are
blocking each type of signal that we were handling, but I think it makes sense 
to
block all signals that we handle for any signal.  E.g., we would only block 
SIGILL if
an illegal instruction signal were delivered, but we also handle SIGBUS.  My fix
would block both in the case that either were delivered, for the duration of the
signal.  

Also when we reset the signal handler we don't save & restore the mask of the 
old
signal handlers, this needs to be fixed.  It's a little tricky because the
sigaction.sa_mask field is meant to be opaque, not meant to be treated as a 
scalar
for portability reasons so I can't just copy it.   There's no method to copy it,
either, so I guess I have to test it for every signal using sigismember().

Original comment by neal...@gmail.com on 10 Feb 2009 at 9:06

[GoogleCodeExporter]

Original comment by neal...@gmail.com on 11 Feb 2009 at 12:18

Attachments:

• linux-signal-handler-fixes.diff

[GoogleCodeExporter]

Thanks for looking into this Neal.

The calls to set up and tear down the handler are commented out in that patch. 
Is
that intentional?

While staring at the code again, I see there is some assembly code doing 
interesting
things that might be replaced by using SA_SIGINFO and sa_sigaction -  poking at 
the
hidden parameter seems to be deprecated these days according to my man pages.

Thank you.

Original comment by craig.sc...@gmail.com on 11 Feb 2009 at 6:01

[GoogleCodeExporter]

I looked into using that at one point but couldn't make it work. I think we'd be
happy to take a patch that used that approach though. Poking at the hidden 
parameter
using assembly is in fact awful, but it was the only thing I could get to work 
at the
time.

Original comment by ted.mielczarek on 11 Feb 2009 at 2:35

[GoogleCodeExporter]

Hi Craig, sorry for the delay, I need to set up better alerts for these bug 
updates.

I did comment out the setup & teardown of the signal handler because(and maybe 
you
can clarify something here) the signal handler is modifying the signal behavior 
of
the current signal, but, by default, the current signal is always blocked while
handling the signal(unless you specify a particular option during the 
sigaction() call)

So I wasn't sure of the intent of restoring the previous signal handler for 
some time
inside the current handler.  I tried asking around but couldn't get a lot of 
info on
this behavior, so I'm happy to back out the change if it is doing something 
that I
missed :-)

Original comment by neal...@gmail.com on 14 Feb 2009 at 2:56

[GoogleCodeExporter]

Hi Neal

Thanks for the comments. I'm not 100% sure of this but perhaps the original 
signal
handler (which would now be called via old_handler) might check to see if it is 
the
currently installed signal handler and depend on that in some way. Calling the
teardown and setup functions in the old code would I assume make the old handler
blissfully unaware that breakpad was calling it. I can't see that actually 
mattering
in practice.

I did spot one thing looking over the code again now btw. - the call to
sigprocmask(SIG_SETMASK, &sig_old, &sig_old) in InternalWriteMinidump should 
probably
have the final parameter as NULL?

Thank you!

Original comment by craig.sc...@gmail.com on 15 Feb 2009 at 6:15

[GoogleCodeExporter]

Hi Craig
Thanks for the comments. This turned out to be a little messier than I thought 
but think this diff addresses 
the original problem as well as others I found during inspection.  Also, since 
you pointed out the signal mas 
modification in InternalWriteMinidump, I made that fix; I thought for a second 
that that whole signal mask bit 
could be removed since now we set it in the init, but it turns out that 
InternalWriteMinidump can be called 
outside of the signal-handling path(to generate minidumps on demand, btw)

Thanks!

Neal

Original comment by neal...@gmail.com on 22 Feb 2009 at 1:19

Attachments:

• linux-signal-handler-fixes.diff-2

[GoogleCodeExporter]

Hi Neal

This looks good! Thank you!!!

--Craig

Original comment by craig.sc...@gmail.com on 23 Feb 2009 at 4:45

[GoogleCodeExporter]

hi Neal,
Looks good to me. Thanks for inspecting the code in such a detail, we hadn't 
been 
able to catch that before.

One minor questions:
+void ExceptionHandler::TeardownHandler(int signo, struct sigaction 
*final_handler) {
+  if (old_actions_[signo].sa_handler) {
+    struct sigaction *act = &old_actions_[signo];
+    sigaction(signo, act, NULL);
+    if (final_handler) {
+      sigaction(signo, NULL, final_handler);
+    }
Why not just: sigaction(signo, act, final_handler);

Original comment by lul...@gmail.com on 24 Feb 2009 at 2:16

[GoogleCodeExporter]

Thanks Liu. Good catch :-)

Original comment by neal...@gmail.com on 26 Feb 2009 at 9:30

[GoogleCodeExporter]

Committed, revision 315

Original comment by neal...@gmail.com on 26 Feb 2009 at 9:32

• Changed state: Fixed