171343 - nsProfileAccess should use SA_SIGINFO and sa_sigaction

Status: VERIFIED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[Brian Ryner (not reading)]

Per the POSIX specification, you can pass SA_SIGINFO as a flag to sigaction()
and get your handler called with 3 paramters instead of 1, i.e.:

void handler(int signum, siginfo_t* info, void* context);

nsProfileAccess currently does not set this flag.  The problem is that if you
have another signal handler installed (before nsProfileAccess's handler) which
wants the extra parameters, it will not receive them.

Since there's no reason not to get the extra info passed to our handler, we
should do this, so we can pass it up to the next handler in the chain (if it
uses SA_SIGINFO).

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Brian Ryner (not reading)]

Note that I specifically do not assume in this patch that sa_handler and
sa_sigaction are a union, even though it seems to be in almost all cases.  POSIX
does not require this.  So I always check the SA_SIGINFO flag and access the
appropriate member based on that.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 100983 [details] [diff] [review]
patch

> #define CATCH_SIGNAL(signame)                                                 \
>     PR_BEGIN_MACRO                                                            \
>         if (sigaction(signame, NULL, &oldact) == 0 &&                         \
>-            oldact.sa_handler != SIG_IGN)                                     \
>+            (((oldact.sa_flags & SA_SIGINFO) &&                               \
>+               oldact.sa_sigaction != (my_sigaction_t) SIG_IGN) ||            \
>+             (!(oldact.sa_flags & SA_SIGINFO) && oldact.sa_handler != SIG_IGN))) \

This violates the 80th column (79 is the last usable one due to wrapping bugs
in some Emacses), but what's more, it should use ?: like so:

+	     ((oldact.sa_flags & SA_SIGINFO)				      
\
+	      ? oldact.sa_sigaction != (my_sigaction_t) SIG_IGN)	      
\
+	      : oldact.sa_handler != SIG_IGN))) 			      
\

Fix that nit, and sr=[e-mail], esp. if your latest testing shows
that it works :-)

/be

Comment 4

[Brian Ryner (not reading)]

Attachment: patch #2

Address brendan's comments; #include <signal.h> from nsProfileAccess.h for
access to the siginfo_t typedef (this must have been getting brought in some
other way on OS X).

Comment 5

[Brian Ryner (not reading)]

Comment on attachment 100984 [details] [diff] [review]
patch #2

carrying over sr=brendan

Comment 6

[Conrad Carlen (not reading bugmail)]

Comment on attachment 100984 [details] [diff] [review]
patch #2

r=ccarlen

Comment 7

[Brian Ryner (not reading)]

siginfo_t and SA_SIGINFO are not defined on OS X 10.1.  I backed out the patch
until I can come up with an autoconf test.

Comment 8

[Brian Ryner (not reading)]

Attachment: patch #3

Add an autoconf test for siginfo_t; use the old behavior if the type is not
present.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 102126 [details] [diff] [review]
patch #3

More of a cls thing, and who knew OSX was evolving so rapidly to catch up to
Unixes of 10 years ago? ;-)  sr=[e-mail] if it helps.

/be

Comment 10

[hacker formerly known as [e-mail]]

Comment on attachment 102126 [details] [diff] [review]
patch #3

r=cls

Comment 11

[chris hofmann]

Comment on attachment 102126 [details] [diff] [review]
patch #3

a=chofmann

Comment 12

[Brian Ryner (not reading)]

checked into the trunk and CHIMERA_M1_0_1_BRANCH.

Comment 13

[K'Trina Medina]

Verified code fix

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It seems like this might have broken talkback on Linux.  See bug 176886,
especially bug 176886 comment 10.