On Sun, Oct 18, 2009 at 03:50:17PM -0700, Philip Guenther wrote:
> On Sun, Oct 18, 2009 at 12:49 AM, Matt Fisher <[email protected]> wrote:
> > You're correct. I got started on this train of thought after reading
> > an old USENIX paper[1]. The line I suggested for deletion is identical
> > to what section 1.2.1 of that paper recommended should always be
> > included. Recent material continues to recommend such code, but not
> > specifically in OpenBSD[2],[3].
> >
> > Since such code is only rarely used in OpenBSD -- 9 occurrences in the
> > whole tree -- I thought it might be an obsolete or inapplicable
> > recommendation. One such occurrence was in m4/main.c, so I contacted the
> > developer who had recently checked in that file.
> >
> > I did read manuals, study code, consider, and test first. But I knew my
> > expertise was limited, and that there could be other considerations.
>
> If all you have is a chunk of code that you don't understand the
> justification for, you have to work backwards to the problem. This
> can turn into the programming equivalent of disproving an existential,
> for which having a broad base of experience really helps. How do you
> get that base of experience? Well, start by tackling concrete
> problems ("program behaves incorrectly when I try <blah>") instead of
> jumping straight to the abstract cases.
>
> That, or you need to crank up your methodology and logic. The code in
> this case applies only when the process is started with SIGINT set to
> SIG_IGN. If the process was set by its parent to ignore SIGINT, why
> would it start paying attention to them now by catching them? Doing
> that might be fine if that was something that used to be normal
> practice but never happened nowadays. So what programs did that in
> the past? Didn't the paper say something about running stuff in the
> background from the shell with "&"? So let's check the sh(1) manpage
> to see what it says now:
> When an asynchronous command is started when job
> control is disabled (i.e. in most scripts), the command is started with
> signals SIGINT and SIGQUIT ignored
>
> Looks like the shell still does that. A little bit of noodling shows
> that this snippet of shell script
> #!/bin/sh
> ( : ; /some/program/here ) &
>
> Invokes /some/program/here with SIGINT ignored.
>
> So, it looks like running m4 in the background from a shell script
> will still result in it being invoked with SIGINT set to SIG_IGN. In
> such a case, if you interrupted the shell script using SIGINT,
> shouldn't m4 ignore the signal and continue? It would continue if the
> shell script exited normally, so why should signaling the script
> terminate it? Ergo, I think this code is correct and should be left
> as is.
>
> You wonder why only 9 programs in the tree have such code. Well, how
> programs does this problem apply to? It doesn't apply to any program
> that calls daemon(), for example, as they won't get SIGINTs a
> terminal. So rpc.statd's signal handling is fine, for example. It
> also doesn't apply to programs that can't be usefully backgrounded, so
> 'rain' is fine as is.
>
> But it looks like some programs do handle it wrong and could use
> fixing. For example, 'sort' appears to be wrong...but before I wrote
> that I figured out how to trigger the problem, both to confirm it and
> to verify the fix. Make sense?
>
> Philip Guenther
>
>
> --- sort.c 22 Aug 2007 06:56:40 -0000 1.36
> +++ sort.c 18 Oct 2009 22:44:47 -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,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...!?
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);
--patrick
