On Mon, Oct 19, 2009 at 10:31:11AM +0200, Otto Moerbeek 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 <[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
>
> I'm not convinced. What behaviour do you consider wrong? I think that
> temp files created should always be cleaned up, even when the program
> ran in the background.
Ehh, please ignore that comment. Too little coffee. The diff looks good.
>
> -Otto
>
> >
> >
> > --- 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);
> > } else
> > outfile = outpath;
> > if (outfp == NULL && (outfp = fopen(outfile, "w")) == NULL)
