bugmake - Bugs: bug #51237, Deadlock in Ctrl-C handler on...

 
 

bug #51237: Deadlock in Ctrl-C handler on Windows

Submitter:  Michael Builov <mbuilov>
Submitted:  Wed 14 Jun 2017 12:43:28 PM UTC
   
 
Severity:  3 - Normal Item Group:  Bug
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
Component Version:  4.2.1 Operating System:  MS Windows
Fixed Release:  None Triage Status:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 10 Jul 2017 07:23:47 PM UTC, comment #7: 

If you are going to refactor the code which handles SIGINT, then we should return to this issue only after the new code is written.

The only Windows-specific aspect of your idea is that the signal handler will still run in a separate thread on Windows, so we should be careful about examining that flag, perhaps using atomic operations for that.

Eli Zaretskii <eliz>
Group Member
Mon 10 Jul 2017 06:29:44 PM UTC, comment #6: 

It's not the exact same problem but it's caused by the same flaw in the make code: doing lots of work in a signal handler.

What I was hoping to do is change the signal handler to do nothing more than set a flag saying that the fatal signal was received, and move all the "stuff" into a separate function that is not a signal handler.

There are lots of issues with having significant work done in a signal handler.  Another bug I just fixed this past weekend is to correct blocking/unblocking fatal signals, which we need to do only because the signal handler is mucking about with the child lists.

Then it becomes the responsibility of the mainline code to check the fatal flag at various times and call the "do fatal stuff" function when it sees the flag is set.

The question of course is, what are the correct "various times".  We can probably make some fairly good guesses: right before we do something that might "take a long time".

I guess the real problem is the same one that led to lots of issues with the jobserver: even if we check immediately before we  start to wait for child to finish the signal could be received in the instant between when we check and we drop into the wait, and we would have to wait for the child to exit before we'd notice the fatal signal.  I'll have to think about that.

Paul D. Smith <psmith>
Group administrator
Mon 10 Jul 2017 06:03:56 PM UTC, comment #5: 


> First, we have this same type of issue in UNIX (see for example bug #50557) so I would prefer to not have a Windows-specific solution.


I'm not sure it's the same issue, although it maybe looks the same.  See below.

> Second, I don't see why we need a separate thread to handle this. Why can't we just modify the signal handler to set a flag saying that a fatal signal was received, then test that flag in appropriate places during the main processing of make. If the flag is set then we die. Of course, finding the "appropriate places" is the trick.


Maybe I'm missing something.  When the user presses Ctrl-C, Make gets a SIGINT.  This causes fatal_error_signal to be called, which then proceeds to do stuff.  That "stuff" is done entirely in the signal handler, so how can just setting a flag solve the issue?  Who will do all that "stuff" "during the main processing"?

> Maybe there's something about Windows that makes this less straightforward than on POSIX systems?


Yes, there is: on Windows, the SIGINT handler runs in a separate thread, created by the system.  That is why the MS-Windows code of the signal handler begins by suspending the main thread -- because this emulates more closely what would happen on Posix hosts.

> Finally, this implementation adds a lot more ifdefs to the code.


That should be the least of our problems.  I actually intended to move all the additional code to Windows-specific files.

But first we need to agree on the solution, so let's forget about ifdef's for now.

Eli Zaretskii <eliz>
Group Member
Mon 10 Jul 2017 01:13:37 AM UTC, comment #4: 

I would prefer this to be done differently.  Sorry for the late conversation.

First, we have this same type of issue in UNIX (see for example bug #50557) so I would prefer to not have a Windows-specific solution.

Second, I don't see why we need a separate thread to handle this.  Why can't we just modify the signal handler to set a flag saying that a fatal signal was received, then test that flag in appropriate places during the main processing of make.  If the flag is set then we die.  Of course, finding the "appropriate places" is the trick.

Maybe there's something about Windows that makes this less straightforward than on POSIX systems?

Finally, this implementation adds a lot more ifdefs to the code.  I'm (more slowly than I'd like) trying to remove ifdefs by moving system-specific code into system-specific source files and creating generic interfaces.

Paul D. Smith <psmith>
Group administrator
Thu 15 Jun 2017 02:34:01 PM UTC, comment #3: 

I have created test application, which detaches console and sleeps for 1000 seconds:

#include <windows.h>
int main(int argc, char *argv[]) {
    FreeConsole();
    Sleep(1000000);
    return 0;
}

And tested it with this makefile:

# run make -j -O, then press Ctrl+C
all: a b c d
CMD := C:\test.exe
a:;$(CMD)
b:;$(CMD)
c:;$(CMD)
d:;$(CMD)
.PHONY: all a b c d

I see next behaviors of original/pached make on Ctrl+C.

1) Original make-4.2.1.

- Main thread is suspended in WaitForMultipleObjects.
- Ctrl+C handler thread waits for one of 4 sub-processes in WaitForMultipleObjects.

2) Patched make-4.2.1.

- Main thread waits for one of 4 sub-processes in WaitForMultipleObjects.
- Ctrl+C handler thread waits for Main thread in WaitForSingleObject.

Both makes hang for 1000 seconds.

...

For this test, it is possible to fix Ctrl+C handler in original make, so it will kill sub-processes.

(replace SIGTERM with SIGINT in fatal_error_signal, because "The SIGILL and SIGTERM signals are not generated under Windows. They are included for ANSI compatibility").

And this is works, but only top child processes get killed:

If I use

CMD := cmd.exe /c "C:\test.exe"

Then by Ctrl+C, make kills only cmd.exe, and C:\test.exe continues to live.

Michael Builov <mbuilov>
Thu 15 Jun 2017 09:20:41 AM UTC, comment #2: 


>> ...Would you be willing to assign copyright for your changes to the FSF...


Yes, I will, no problem.

>> ...signaling the event you added will not interrupt that wait...


New event is not supposed to interrupt Main thread waiting for sub-processes.
Event is waited only by Ctrl+C handler thread and is signaled by Main thread just before it goes to infinite sleep.

By default, CTRL+C signal is passed to all console processes that are attached to the console.
So, normally, all console sub-processes spawned by make are stopped by CTRL+C.

Only processes without console (GUI apps) or that have explicitly detached console (via FreeConsole) are not stopped by Ctrl+C.
But that kind of processes are unlikely run by make, killing them is questionable.

Michael Builov <mbuilov>
Wed 14 Jun 2017 03:11:05 PM UTC, comment #1: 

Thanks for the analysis and the patch.
Your patch is too large to accept without legal paperwork.  Would you be willing to assign copyright for your changes to the FSF, so we could accept your contribution?  If so, I can send you the legal form and instructions to fill and submit it.

Also, I wonder if your implementation is the best one (or maybe I'm missing something).  You've inserted a call to check_need_sleep into the main loop of reap_children.  But if the 1st arg of reap_children is non-zero, Make will block inside process_wait_for_any, and AFAIU signaling the event you added will not interrupt that wait.  Wouldn't it be better to instead add the event handle to the handles on which process_wait_for_any waits?  Then the reaction to an interrupt should be much faster and more reliable, I think.  Or am I missing something?

Eli Zaretskii <eliz>
Group Member
Wed 14 Jun 2017 12:43:28 PM UTC, original submission:  

Hello.

There is a bug in processing of Ctrl+C event under WINDOWS.

Gnu make handles Ctrl+C in fatal_error_signal() function, which suspends Main thread.

But, if Main thread get suspended while it locks resources, where is a possibility for dead-lock.

It's easy to reproduce dead-lock with this makefile:

# run make -j -O, then press Ctrl+C
all: a b c d
CMD := cmd.exe /c "echo off & for /l %a in (0,1,1000) do echo %a"
a:;$(CMD)
b:;$(CMD)
c:;$(CMD)
d:;$(CMD)
.PHONY: all a b c d

(backtrace1 attached)


Even without job-server and output synchronization, it is possible to dead-lock make or put it to infinite loop:

# just run make without options, then press Ctrl+C
# (hard to reproduce dead-lock)
GOALS := a b c d e f g h i j k l m n o p q r s t u v w x y z
GOALS += $(GOALS:=1)
GOALS += $(GOALS:=2)
GOALS += $(GOALS:=3)
all: $(GOALS)
$(GOALS):;cmd.exe /c "echo off & echo 1 > NUL"
.PHONY: all $(GOALS)

(backtrace2 attached)


One of possible solutions to this dead-lock problem - instead of suspending Main thread, Ctrl+C handler thread may notify Main thread to stop and wait until it releases resources and goes to sleep.

Here is the patch:

https://github.com/mbuilov/gnumake-windows/blob/master/make-4.2.1-win32-ctrl-c.patch

Michael Builov <mbuilov>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #40917:  backtrace1 added by mbuilov (1KiB - application/octet-stream - backtraces of dead-locked Main and Ctrl+C threads)
file #40918:  backtrace2 added by mbuilov (2KiB - application/octet-stream - backtraces of dead-locked Main and Ctrl+C threads)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by eliz (Posted a comment)
  • -email is unavailable- added by mbuilov (Submitted 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 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-06-14 mbuilov Attached File- Added backtrace1, #40917
        Attached File- Added backtrace2, #40918

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code