Signal handling question

[Matthew J. Fisher]

Stuart Henderson referred me to the tech list for this question.

Here's a diff showing one line of possible cruft deleted from m4.
This is one of 9 similar occurrences I've found in the src tree.

$ cd /usr/src/usr.bin/m4
$ diff -u main.c main.c.new
--- main.c      Thu Oct 15 21:42:14 2009
+++ main.c.new  Wed Oct 15 23:37:01 2009
@@ -172,8 +172,7 @@
        int n;
        char *p;

-       if (signal(SIGINT, SIG_IGN) != SIG_IGN)
-               signal(SIGINT, onintr);
+       signal(SIGINT, onintr);

        init_macros();
        initspaces();

If I understand correctly, the original code is setting a signal
behavior in order to check a signal behavior. I think the idea is to
only handle SIGINT if not ignoring it -- for example, to only catch ^C
when in the foreground.

A few days back I asked the misc list if such code should be *added*.
Theo replied this isn't necessary in BSD. So I've concluded that
existing code like this could in fact be deleted.

Is that right? Anybody want to see more diffs for deleted cruft?

Thanks.
--
Matt Fisher <mfisher_ix@...>

[Ingo Schwarze]

Hi Matt,

Matt Fisher wrote on Thu, Oct 15, 2009 at 11:39:39PM -0400:

> Stuart Henderson referred me to the tech list for this question.
>
> Here's a diff showing one line of possible cruft deleted from m4.
> This is one of 9 similar occurrences I've found in the src tree.

I guess you are changing semantics.

> $ cd /usr/src/usr.bin/m4
> $ diff -u main.c main.c.new
> --- main.c      Thu Oct 15 21:42:14 2009
> +++ main.c.new  Wed Oct 15 23:37:01 2009
> @@ -172,8 +172,7 @@
>         int n;
>         char *p;
>
> -       if (signal(SIGINT, SIG_IGN) != SIG_IGN)
> -               signal(SIGINT, onintr);

As far as i understand, that means:
 "In case SIGINT is being ignored at the time m4 is invoked,
  continue ignoring it.
  In case it is not being ignored at startup time,
  replace the existing handler by the function onintr."

> +       signal(SIGINT, onintr);

That means:
 "Always install onintr as the signal handler,
  even in case SIGINT is currently being ignored."

So, if i'm understanding correctly, you are removing the option
to ignore SIGINT while running m4.

> A few days back I asked the misc list if such code should be *added*.

As an aside, you didn't say where, or why.

> Theo replied this isn't necessary in BSD.

No, Theo wrote:

  "I assume you are talking about resetting the signals when
   they are caught.  That is not required in BSD unix."

It is *now* clear this was _not_ what you were talking about,
so Theo's answer doesn't apply.
In fact, you gave no context, so it was not obvious what you
were talking about.

> So I've concluded that
> existing code like this could in fact be deleted.

At least for the case at hand, i doubt that, but i do not know
that much about signal handling, so i would certainly not dare
to attempt cleanups in that region.

> Is that right?
> Anybody want to see more diffs for deleted cruft?

Thanks, not me.  ;)

Yours,
  Ingo

[Philip Guenther-2]

On Sat, Oct 17, 2009 at 8:07 AM, Ingo Schwarze <schwarze@...> wrote:
> Matt Fisher wrote on Thu, Oct 15, 2009 at 11:39:39PM -0400:
>> Stuart Henderson referred me to the tech list for this question.
>>
>> Here's a diff showing one line of possible cruft deleted from m4.
>> This is one of 9 similar occurrences I've found in the src tree.
>
> I guess you are changing semantics.

Yes, yes that would.

<...>
> As far as i understand, that means:
>  "In case SIGINT is being ignored at the time m4 is invoked,
>  continue ignoring it.
>  In case it is not being ignored at startup time,
>  replace the existing handler by the function onintr."

See, Ingo, you're thinking about what the change means.  Matt, do you
understand *why* that code was originally added to m4?  Do you
understand the circumstances under which it actually had an effect?
Have you investigated whether those circumstances still occur?

If the answers to those questions are "yes", then you should say so
and back it up.  If not, why should we apply a diff to remove code
from someone who doesn't know why the code is there?  As is, I believe
you don't understand why that code was added, don't understand when it
was triggered, and don't know whether or not it could still be
triggered.

>> A few days back I asked the misc list if such code should be *added*.
>
> As an aside, you didn't say where, or why.
>
>> Theo replied this isn't necessary in BSD.
>
> No, Theo wrote:
>
>  "I assume you are talking about resetting the signals when
>   they are caught.  That is not required in BSD unix."
>
> It is *now* clear this was _not_ what you were talking about,
> so Theo's answer doesn't apply.
> In fact, you gave no context, so it was not obvious what you
> were talking about.

Well, IIRC, he cited a usenix paper that described why this code was
needed.  From Theo's reply I would assume (there's that word again)
that he didn't chase the link.  I suspect Theo was talking about BSD
changing the behavior of signal() to not reset signals on entry to the
handler, but that's not what this code is about.  The job control
semantics (process-groups, etc) that BSD added *are* relevant, but are
not the entire picture.


>> So I've concluded that
>> existing code like this could in fact be deleted.
>
> At least for the case at hand, i doubt that, but i do not know
> that much about signal handling, so i would certainly not dare
> to attempt cleanups in that region.

I do know signals pretty well and I doubt it as well.


>> Is that right?
>> Anybody want to see more diffs for deleted cruft?

If they lack research like this one, I don't want to see them either.


Philip Guenther

[Otto Moerbeek]

On Sat, Oct 17, 2009 at 08:45:39PM -0700, Philip Guenther wrote:

[snip]

> If they lack research like this one, I don't want to see them either.


Thanks Philip, you just wrote what I was going to write.

Any diff, and signal diffs even more so, requires understanding of
what you are changing.

        -Otto

[Matthew J. Fisher]

On Sat, 17 Oct 2009 20:45:39 -0700
Philip Guenther <guenther@...> wrote:

Many solid points and:

>If the answers to those questions are "yes", then you should say so
>and back it up.  If not, why should we apply a diff to remove code
>from someone who doesn't know why the code is there?  As is, I believe
>you don't understand why that code was added, don't understand when it
>was triggered, and don't know whether or not it could still be
>triggered.

Although I'd rather crawl under a rock, I'll respond.

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.
I never said imperatively "delete this". I asked Stuart if it was a
valid change. To you, I said it was "possible cruft".

It's good that you are circumspect. Frankly, I was expecting the answer
to be known. But if it's doubtful, the change has to be rejected.

[1] http://www.darwinsys.com/history/canthappen.html
[2] http://oreilly.com/catalog/9780596009588
[3] http://www.cs.princeton.edu/courses/archive/spr06/cos217/lectures/23signals.pdf
--
Matt Fisher <mfisher_ix@...>

[Philip Guenther-2]

On Sun, Oct 18, 2009 at 12:49 AM, Matt Fisher <mfisher_ix@...> 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);
        } else
                outfile = outpath;
        if (outfp == NULL && (outfp = fopen(outfile, "w")) == NULL)

[Otto Moerbeek]

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_ix@...> 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.

        -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)

[Otto Moerbeek]

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 <mfisher_ix@...> 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)

[patrick keshishian-2]

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_ix@...> 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

[Otto Moerbeek]

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_ix@...> 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

[Philip Guenther-2]

On Fri, Oct 23, 2009 at 12:05 AM, patrick keshishian
<sidster@...> 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