bugmake - Bugs: bug #63307, make 4.4 passes ignored SIGPIPE on...

 
 

bug #63307: make 4.4 passes ignored SIGPIPE on to children

Submitter:  None
Submitted:  Wed 02 Nov 2022 03:12:02 PM UTC
 
Severity:  3 - Normal Item Group:  Bug
Status:  Fixed Privacy:  Public
Assigned to:  psmith Open/Closed:  Closed
Component Version:  4.4 Operating System:  POSIX-Based
Fixed Release:  SCM Triage Status:  Small Effort
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       No canned response available

 

( Jump to the original submission )

Sat 12 Nov 2022 03:07:58 PM UTC, comment #19: 

In addition to the diffutils failure (which I am also observing), this change broke m4 tests:

https://savannah.gnu.org/support/index.php?110767

I'd err on the side of previous behavior, since this is not the last such failure for sure.

Alexander Kanavin <kanavin>
Wed 09 Nov 2022 08:36:54 PM UTC, comment #18: 

I can see an argument for both sides ("give up immediately" and "run to completion").  Most likely it's one of those things where different people legitimately want different behaviors.

But I think that since all previous versions of GNU Make used the "give up immediately" model, and it wasn't our intention to specifically change that, we should preserve that behavior until/unless someone wants the opposite behavior.

If we have "give up immediately" and you want to guard against that, you can do so (say, redirecting output to /dev/null or whatever).  If we have "run to completion" and that's not what you want, you don't have any way to get "give up immediately" (unless we introduce an option which I don't want to do).

Not to mention the advantage that it's a much simpler (trivial) change to go back to "give up immediately" :).

Paul D. Smith <psmith>
Project Administrator
Tue 08 Nov 2022 02:03:46 PM UTC, comment #17: 

> Speaking only for myself, the most realistic scenario I can think of where I would encounter this myself is if I run 'make 2>&1 | less' and then quit less. In that case I would not be interested in having any build continue, I would just want to get back to my prompt


In this scenario the user quit less.
i was thinking about automated builds (jenkins, build farms, etc). Those build systems get really messy with miriads of pieces of shell code. Should make abort a build if one of those pipelines fail? My thinking was, if the failed pipeline was supposed to produce something, then its failure will fail the build anyway.

Dmitry Goncharov <dgoncharov>
Tue 08 Nov 2022 01:58:13 PM UTC, comment #16: 

> It appears that make still performs the same build.  But maybe I just got something wrong in my test.


For example something like this

$ cat makefile
all: one
one: two
        @echo cp $< $@ >&2
        @cp $< $@
two:
        @echo touch $@ >&2
        @touch $@
$ rm one two
$ # this is make-4.3
$ (sleep 2 ; ~/src/make/4.3/make --debug=b)|:
$ ls one two
ls: cannot access 'one': No such file or directory
ls: cannot access 'two': No such file or directory
$ # this is make which handles sigpipe and completes the build
$ (sleep 2 ; ~/src/make/l64/make --debug=b)|:
received sigpipe
received sigpipe
received sigpipe
received sigpipe
received sigpipe
received sigpipe
received sigpipe
received sigpipe
received sigpipe
received sigpipe
received sigpipe
touch two
received sigpipe
received sigpipe
cp two one
received sigpipe
received sigpipe
received sigpipe
make: write error: stdout
$ ls one two
one  two

~/src/make/l64/make is a modified make which has the following sigpipe handler installed

static void
sigpipe_handler (int sig)
{
    static const char msg[] = "received sigpipe\n";
    write (STDERR_FILENO, msg, sizeof msg);
}

i remember you reported that on your system you had to modify make to set sigpipe to default, otherwise it was ignored.
What about something like

$ cat test.pl
#!/usr/bin/perl

$SIG{'PIPE'} = 'DEFAULT';
system('bash -c "(sleep 4 ; ~/src/make/l64/make --debug=b)|:"');

Dmitry Goncharov <dgoncharov>
Tue 08 Nov 2022 01:23:42 PM UTC, comment #15: 

Whether that's "equally likely" is something we are probably going to disagree over, especially in the case where output (or potentially error) is piped to another process, but not worth getting into a discussion about. Suffice to say, it is indeed possible that the spawned processes would produce no output, and letting make continue would potentially allow a build to complete successfully. Whether that's desirable is going to depend on the user. Speaking only for myself, the most realistic scenario I can think of where I would encounter this myself is if I run 'make 2>&1 | less' and then quit less. In that case I would not be interested in having any build continue, I would just want to get back to my prompt, but I'm happy to accept that other scenarios may exist where other users may want something different.

Anonymous
Tue 08 Nov 2022 10:21:53 AM UTC, comment #14: 

The SIGPIPE failure only happens when the process actually produces output to the broken pipe.  But it is equally likely that a recipe doesn't actually produce any output on stdout.

Andreas Schwab <schwab>
Tue 08 Nov 2022 10:04:11 AM UTC, comment #13: 

When SIGPIPE is set to the default action, then yes, writing to a closed pipe is expected to get make to die abnormally. That's how SIGPIPE is designed to work. :) This is different from a regular failure to write to stdout.

There is little benefit to continuing the build in that case: if the build continues, spawned processes will see the same closed pipe that make saw, and those spawned processes will likely fail anyway because they too would be terminated by SIGPIPE if they attempt to write to stdout.

Anonymous
Mon 07 Nov 2022 10:33:27 PM UTC, comment #12: 

> Should inability to write to stdout cause make to die abnormally?


I had the same thought but oddly, I can't seem to generate a use-case where make behaves differently with this FATAL_SIG set for SIGPIPE.

It appears that make still performs the same build.  But maybe I just got something wrong in my test.

Paul D. Smith <psmith>
Project Administrator
Mon 07 Nov 2022 10:25:52 PM UTC, comment #11: 

comment #10:

> I'm not sure how using a semaphore helps here.  We would still have to clean up the semaphore with sem_unlink().


indeed.

> The main goal of that change was to allow our cleanup operations to proceed even when SIGPIPE was found, and semaphores still need to be cleaned up.  To that end it seems like the idea to use FATAL_SIG() (which already handles SIG_IGN correctly) is the right one.


Should inability to write to stdout cause make to die abnormally?
Make can leave an error message and complete the build and exit normally.

Dmitry Goncharov <dgoncharov>
Sun 06 Nov 2022 08:05:07 PM UTC, comment #10: 

I'm not sure how using a semaphore helps here.  We would still have to clean up the semaphore with sem_unlink().

The main goal of that change was to allow our cleanup operations to proceed even when SIGPIPE was found, and semaphores still need to be cleaned up.  To that end it seems like the idea to use FATAL_SIG() (which already handles SIG_IGN correctly) is the right one.

Sorry for not seeing that when the change was originally proposed.

Let me know if there is disagreement.

Paul D. Smith <psmith>
Project Administrator
Sun 06 Nov 2022 12:58:30 PM UTC, comment #9: 

On Sat, Nov 5, 2022 at 6:54 PM Philip Guenther<guenther@gmail.com> wrote:

> This is a change in behavior:

The root of this change is not sigpipe itself, it is the new synchronisation mechanism which uses temporary files.
Paul, i remember we discussed using a semaphore for output sync. Do you see reasons to prefer files over a semaphore for that?

>  Can you just skip that if the disposition is SIG_IGN at start?

That is easy with sigaction. However, make also runs on systems where sigaction is not available. sv63307_fix2.diff does the same as proposed in update 7.

Dmitry Goncharov <dgoncharov>
Sun 06 Nov 2022 11:13:31 AM UTC, comment #8: 

There may be an even simpler solution than that. There was never any need to ignore SIGPIPE, we just need to make sure temporary files are cleaned up if we get SIGPIPE, it's okay and expected if make dies after that cleanup. There is already a mechanism in place for that for other signals. Re-using that for SIGPIPE works:

--- a/src/main.c
+++ b/src/main.c
@@ -1182,11 +1182,6 @@ main (int argc, char **argv, char **envp)
   /* Useful for attaching debuggers, etc.  */
   SPIN ("main-entry");

-  /* Don't die if our stdout sends us SIGPIPE.  */
-#ifdef SIGPIPE
-  bsd_signal (SIGPIPE, SIG_IGN);
-#endif
-
 #ifdef HAVE_ATEXIT
   if (ANY_SET (check_io_state (), IO_STDOUT_OK))
     atexit (close_stdout);
@@ -1285,6 +1280,8 @@ main (int argc, char **argv, char **envp)
   FATAL_SIG (SIGXFSZ);
 #endif

+  FATAL_SIG (SIGPIPE);
+
 #undef  FATAL_SIG

   /* Do not ignore the child-death signal.  This must be done before

Anonymous
Sun 06 Nov 2022 11:00:02 AM UTC, comment #7: 

> Other reasons are the desire to avoid complexity in make.


I agree with the desire to avoid complexity, but preserving an ignored SIGPIPE on top of your patch needs no more than:

  if (bsd_signal (SIGPIPE, handle_sigpipe) == SIG_IGN)
    bsd_signal (SIGPIPE, SIG_IGN);

Anonymous
Sat 05 Nov 2022 10:34:30 PM UTC, comment #6: 

> If SIGIGN was ignored before make was started though, it should remain ignored, even for make's children, see also <https://lists.gnu.org/archive/html/bug-make/2022-11/msg00015.html> in reply to Andreas Schwab's first patch. Correct me if I am wrong, but I think your patch re-enables it in that case?


My patch causes the disposition of sigpipe to be set to SIG_DFL in the new process. See https://pubs.opengroup.org/onlinepubs/9699919799/, specifically
"Signals set to be caught by the calling process image shall be set to the default action in the new process image (see <signal.h>)."

This desire to have the default disposition is intentional. Some of the reasons are explained in update 2. Other reasons are the desire to avoid complexity in make. Especially when there is no clear need for that complexity.

In regards to the specific impl of Andreas's 2nd patch there are 2 points.

1. That patch fails to restore the disposition in the case of make re-executing itself (in order to read an updated makefile).
This could be fixed by calling sigaction before execve, but that'd not be atomic.

2. That patch uses posix_spawnattr_setsigdefault to set the disposition for a child process (when make uses posix_spawn). This fails the purpose of the patch, if make was invoked with sigpipe disposition of SIG_HOLD.

Fixing those deficiencies is not that cheap. In the end, i didn't see any benefit for make to go through that trouble of restoring sigpipe disposition for its children and i proposed this simple fix.

Unlike attempts to restore, one nice property of the sighandler patch is that, if more calls to exec are introduced to make, the patch will still work for all of them.

Dmitry Goncharov <dgoncharov>
Sat 05 Nov 2022 05:37:27 PM UTC, comment #5: 

That previous comment was meant to read SIGPIPE, not SIGIGN, of course :)

Anonymous
Sat 05 Nov 2022 05:35:37 PM UTC, comment #4: 

Nice idea to just have a signal handler that does nothing. If SIGIGN was ignored before make was started though, it should remain ignored, even for make's children, see also <https://lists.gnu.org/archive/html/bug-make/2022-11/msg00015.html> in reply to Andreas Schwab's first patch. Correct me if I am wrong, but I think your patch re-enables it in that case?

Anonymous
Fri 04 Nov 2022 08:59:27 PM UTC, comment #3: 

Tested on linux, sun and aix.

Dmitry Goncharov <dgoncharov>
Fri 04 Nov 2022 08:58:41 PM UTC, comment #2: 

Is it useful to restore sigpipe before spawning children? Is that not good enough to have sigpipe disposition set to default for children?
There are signals for which it is important to preseve the disposition, notably sighup for nohup, but not sigpipe.
Unix programs assume that sigpipe is set to the default disposition. Is there a program which misbehaves if make sets sigpipe to the default?
With these considerations in mind i suggest the patch in the attachment.

Dmitry Goncharov <dgoncharov>
Thu 03 Nov 2022 12:23:22 AM UTC, comment #1: 

Can confirm the patch posted to the mailing list, <https://lists.gnu.org/archive/html/bug-make/2022-11/msg00016.html>, works for me to fix this. Thanks for the prompt response.

Anonymous
Wed 02 Nov 2022 03:12:02 PM UTC, original submission:  

Test case:

$ cat Makefile
all:
(sleep 1; echo foo; echo bar >&2) | :
$ make-4.3
(sleep 1; echo foo; echo bar >&2) | :
$ make-4.4
(sleep 1; echo foo; echo bar >&2) | :
/bin/sh: 1: echo: Broken pipe
bar
$

I asked in #63248 whether this was intentional. It was not, so here is a bug for it. What happens in make 4.3 is the sleep makes it so that the "echo foo" is extremely unlikely to start until the : has already terminated. Then, when "echo foo" attempts to write, the whole shell process receives SIGPIPE. What happens in make 4.4 is that the shell process does not receive SIGPIPE and continues processing.

This causes SuSE bug 33947 (https://lists.gnu.org/archive/html/bug-diffutils/2019-01/msg00000.html, diffutils testsuite failure when SIGPIPE is ignored) to appear in environments where it previously did not, after upgrading to make 4.4.

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #53932:  sv63307_fix2.diff added by dgoncharov (1KiB - application/octet-stream)
file #53933:  sv63307_test2.diff added by dgoncharov (3KiB - application/octet-stream)
file #53927:  sv63307_fix.diff added by dgoncharov (1KiB - application/octet-stream)
file #53928:  sv63307_test.diff added by dgoncharov (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kanavin (Posted a comment)
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by dgoncharov (Updated the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-11-13 psmith StatusNone Fixed
        Assigned toNone psmith
        Open/ClosedOpen Closed
        Fixed ReleaseNone SCM
        Triage StatusNone Small Effort
    2022-11-06 dgoncharov Attached File- Added sv63307_fix2.diff, #53932
        Attached File- Added sv63307_test2.diff, #53933
    2022-11-04 dgoncharov Attached File- Added sv63307_fix.diff, #53927
        Attached File- Added sv63307_test.diff, #53928

    Back to the top


    Powered by Savane 3.9