On Fri, Oct 23, 2009 at 12:05 AM, patrick keshishian
<[email protected]> wrote:
> On Sun, Oct 18, 2009 at 03:50:17PM -0700, Philip Guenther wrote:
...
>> @@ -298,7 +298,9 @@ main(int argc, char *argv[])
>> act.sa_flags = SA_RESTART;
>> act.sa_handler = onsig;
>> for (i = 0; sigtable[i]; ++i) /* always unlink toutpath
*/
>> - sigaction(sigtable[i], &act, 0);
>> + if (sigaction(sigtable[i], NULL, &oact) ||
>> + oact.sa_handler != SIG_IGN)
>> + sigaction(sigtable[i], &act, NULL);
>
> Ughh... I'm confused and I keep reading your explanation above
> to no avail.
>
> Are you saying that if parent of sort could set the signal
> handler to something other than SIG_IGN, in that case sort
> should not set its own handler? Wha...!?
Nope, the other way around: only catch SIGINT if it wasn't inherited as
SIG_IGN.
> Also, are you not making an assumption that call to sigaction()
> in the if()-statement you added will not fail? i.e., wouldn't
> this be safer ("more correct"):
>
> + if (0 == sigaction(sigtable[i], NULL, &oact) &&
> + oact.sa_handler != SIG_IGN)
> + sigaction(sigtable[i], &act, NULL);
There are four options for when the first sigaction() call fails:
1) treat it as if it had returned the signal as being ignored
2) treat it as if it had returned the signal as *not* being ignored
3) ignore the possibility and behave randomly
4) treat it as a fatal error
My diff did #2, yours does #1. Arguably, those are both silly choices
given that sigaction() only has two possible error results, EINVAL and
EFAULT, and both are "cannot occur" in this code. (I think EFAULT
would imply the process's stack is no longer mapped!) I don't think I
can cause trigger either error without running sort under a debugger
or systrace or something similar. Given that, ignoring the
possibility of an error (option #3) might be reasonable. After all,
the process is probably going to just croak with another dozen
instructions. Even writing an error message to stderr is probably
hopeless.
But we strive for bullet-proof code here, so we'll assume that someone
somewhere will find a magic way to run sort that makes the sigaction()
call fail. We should return a nice error message when that happens
and fail rather than possibly leave temp files lying around. So,
option #4:
Index: sort.c
===================================================================
RCS file: /cvs/src/usr.bin/sort/sort.c,v
retrieving revision 1.36
diff -u -p sort.c
--- sort.c 22 Aug 2007 06:56:40 -0000 1.36
+++ sort.c 25 Oct 2009 03:48:10 -0000
@@ -273,7 +273,7 @@ main(int argc, char *argv[])
outfile = outpath = toutpath;
} else if (!(ch = access(outpath, 0)) &&
strncmp(_PATH_DEV, outpath, 5)) {
- struct sigaction act;
+ struct sigaction oact, act;
int sigtable[] = {SIGHUP, SIGINT, SIGPIPE, SIGXCPU, SIGXFSZ,
SIGVTALRM, SIGPROF, 0};
int outfd;
@@ -298,7 +298,10 @@ main(int argc, char *argv[])
act.sa_flags = SA_RESTART;
act.sa_handler = onsig;
for (i = 0; sigtable[i]; ++i) /* always unlink toutpath */
- sigaction(sigtable[i], &act, 0);
+ if (sigaction(sigtable[i], NULL, &oact) < 0 ||
+ oact.sa_handler != SIG_IGN &&
+ sigaction(sigtable[i], &act, NULL) < 0)
+ err(2, "sigaction");
} else
outfile = outpath;
if (outfp == NULL && (outfp = fopen(outfile, "w")) == NULL)
Philip Guenther
