On Fri, Oct 23, 2009 at 12:05:20AM -0700, patrick keshishian wrote:
> On Sun, Oct 18, 2009 at 03:50:17PM -0700, Philip Guenther wrote:
> > On Sun, Oct 18, 2009 at 12:49 AM, Matt Fisher <mfisher...@maine.rr.com>
> > 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...!?
Lemme explain:
Either sigaction(&oact) returns 0 or not.
The first case: it returns zero, so the second part of the || is evaluated.
If oact.sa_handler != SIG_IGN, i.e. the parent did not ignore the
signal, we shouldn't ignore it as well, but of course install our own handler.
If the signal is being ignored, just continue to ignore it.
The second case is: sigaction(&oact) return nonzero. In that case it
failed, and we also set our own handler.
The || is making sure we only evaluate oact.sa_handler if it's initalized.
>
> 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);
That would not set the handler if the first sigaction failed.
-Otto
