[bug #63157] Unlink temporary files.

[Martin Dorey]

Follow-up Comment #26, bug #63157 (project make):

Ach, it did reproduce for me, with or without Dmitry's modification.  I was
just looking in the wrong place, despite Frank providing copy-and-paste
instructions on where to look.  Perhaps I'd just read about MAKE_TMPDIR and
assumed that they'd end up in the nice clean directory I'd made for them, as
they do with:

 MAKE_TMPDIR=$(pwd) make -j2 -O 2>&1 | :

Sorry for the mislead.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #25, bug #63157 (project make):

This may not always reproduce, because there is a race between make writing
its output to the pipe and the reader exiting.
Sigpipe is sent when the reader exits before make is able to write its
output.
However, if you change it like this

echo 'all:; sleep 2' | make -f- -j2 -O |:

it'll reproduce.

Thanks for you report, Frank.
i opened https://savannah.gnu.org/bugs/index.php?63248 and attached a patch.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Frank Heckenbach]

Follow-up Comment #24, bug #63157 (project make):


[comment #23 comment #23:]
> Frank wrote [
> 
> I now found a way to reproduce it quite easily on my system.
> As I suspected, it seems to be SIGPIPE:
> 
> % rm -f /tmp/G*
> % cat Makefile
> all: ; sleep 1
> % make -j2 -O 2>&1 | :
> % ls -l /tmp/G*
> prw--- 1 frank frank 0 19. Okt 22:42 /tmp/GMfifo370973
> -rw--- 1 frank frank 0 19. Okt 22:42 /tmp/GmFkJIuq
> %
> 
> 
> ].  That looked like just the ticket but sadly it doesn't reproduce it for
me, on amd64 Debian Stretch.

I just reproduced it on a Linux 4.9.0-5-amd64 #1 SMP Debian 4.9.65-3+deb9u2
(2018-01-04) x86_64 GNU/Linux machine. So it doesn't seem to be the version.

Anyway, strace confirms that make is killed by SIGPIPE, so if you catch it (or
ignore it and then handle or ignore stdout/stderr write errors), it should
work.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Martin Dorey]

Follow-up Comment #23, bug #63157 (project make):

Frank wrote [

I now found a way to reproduce it quite easily on my system.
As I suspected, it seems to be SIGPIPE:

% rm -f /tmp/G*
% cat Makefile
all: ; sleep 1
% make -j2 -O 2>&1 | :
% ls -l /tmp/G*
prw--- 1 frank frank 0 19. Okt 22:42 /tmp/GMfifo370973
-rw--- 1 frank frank 0 19. Okt 22:42 /tmp/GmFkJIuq
%


].  That looked like just the ticket but sadly it doesn't reproduce it for me,
on amd64 Debian Stretch.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

Re: [bug #63157] Unlink temporary files.

[Frank Heckenbach]

Paul D. Smith wrote:

> Follow-up Comment #21, bug #63157 (project make):
> 
> The only places leftover files may happen is (a) on Windows, only when (b) you
> kill GNU make with ^C or similar.
> 
> Any other leftover files should not happen.  Please provide details of the
> names of the files that are left over and which circumstances you see them in.
>  We do have tests that verify that temp files are removed so maybe those tests
> are not complete.

I now found a way to reproduce it quite easily on my system.
As I suspected, it seems to be SIGPIPE:

% rm -f /tmp/G*
% cat Makefile
all: ; sleep 1
% make -j2 -O 2>&1 | :
% ls -l /tmp/G*
prw--- 1 frank frank 0 19. Okt 22:42 /tmp/GMfifo370973
-rw--- 1 frank frank 0 19. Okt 22:42 /tmp/GmFkJIuq
%

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #22, bug #63157 (project make):

Frank, can you please tell us how to reproduce?


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Paul D. Smith]

Follow-up Comment #21, bug #63157 (project make):

The only places leftover files may happen is (a) on Windows, only when (b) you
kill GNU make with ^C or similar.

Any other leftover files should not happen.  Please provide details of the
names of the files that are left over and which circumstances you see them in.
 We do have tests that verify that temp files are removed so maybe those tests
are not complete.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Frank Heckenbach]

Follow-up Comment #20, bug #63157 (project make):

Unfortunately, I still get leftover temp files and fifos with 4.3.91.

If I understand you right, it's not worth investigating it further now, is
it?

Just one guess: Since I pipe make's output to my tools, that might be the
cause. So you might want to make SIGPIPE a FATAL_SIG too, or ignore SIGPIPE
and make sure write errors to stdout and stderr are handled.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Paul D. Smith]

Update of bug #63157 (project make):

  Status:None => Fixed  
 Assigned to:None => psmith 
 Open/Closed:Open => Closed 
   Fixed Release:None => SCM
   Triage Status:None => Medium Effort  


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Paul D. Smith]

Follow-up Comment #19, bug #63157 (project make):

It's probably not worth spending a lot of time ruminating on these things. 
The reality is that currently make does many non-handler-safe things in its
signal handler, and there are Savannah bugs about hangs etc. while catching
SIGINT which prove it if you don't want to check the code.

The goal of my rewrite (slated for the next release) is to rework the signal
handling so that the handler does nothing but set a variable (volatile
sig_atomic_t) and then all the actual handling of the signal will be done in
"normal code".

As I'm sure you're aware this means a lot of careful changes to avoid race
conditions etc. but once complete these issues will not be relevant.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Frank Heckenbach]

Follow-up Comment #18, bug #63157 (project make):


[comment #17 comment #17:]
> Paul, if you decide to proceed with changes in comment 11, temp_stdin_fileno
should probably be of type volatile sig_atomic_t, rather than int.

Are you sure that's big enough? According to
https://cplusplus.com/reference/cstdint/ it might not be.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #17, bug #63157 (project make):

i think, it is fine (maybe even good) if the changes in comment 11 are
abandoned. The whole critical section only lasts while make is reading and
parsing the temp stdin file. The duration of this critical section is likely
to be a fraction of compilation time of even one source file.
Paul, if you decide to proceed with changes in comment 11, temp_stdin_fileno
should probably be of type volatile sig_atomic_t, rather than int.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Paul D. Smith]

Follow-up Comment #16, bug #63157 (project make):

Sorry I should have been more clear.  When I said "non-regression changes" I
was specifically referring to the extra effort needed to get files to be
deleted on Windows, as in comment #11

That change is related to the temporary stdin file, not to the jobserver, and
treatment of that hasn't changed since 4.3 on either Windows or POSIX, I don't
think.  The already-existing behavior is sufficient to remove these files on
POSIX.  The extra changes are only needed to remove them on Windows.

I do have Dmitry's patch already mostly applied, but needed some changes.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Frank Heckenbach]

Follow-up Comment #15, bug #63157 (project make):

FWIW, I do consider this a regression, since these things weren't even created
before.

Now, after some weeks of trying make 4.3.90, I get a number of empty files and
named pipes lying around -- probably from aborted make runs, but such is life
(sometimes you realise a mistake while compiling and abort it).

I hope this can be fixed in the next release, otherwise I'll have to go back
to 4.3, I'm afraid.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Paul D. Smith]

Follow-up Comment #14, bug #63157 (project make):

Thanks, I will look at this within the next few days.

Note, I'm not too concerned with non-regression changes for signal handling:
the fact is that our current signal handling is totally broken (see the 5-6
issues related to things not dying properly on SIGINT for example) and fixing
this was the last thing I was hoping to get done for this release but had to
leave it only mostly-complete as it was taking too long.  I will revisit this
after the release is done.

The goal is to modify our signal handling so it does nothing but set a
notification that the signal was received, then all handling of the signal
will happen in the main code not in the handler so that these types of issues
can be avoided.

The problem of course is that doing this without race conditions is the
classic tricky problem in POSIX programming so some things need to be
reworked.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #13, bug #63157 (project make):

Thank you, Eli.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Eli Zaretskii]

Follow-up Comment #12, bug #63157 (project make):

Thanks, the diffs look good (although I didn't actually test them).



___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #11, bug #63157 (project make):

> If temp_stdin is fclosed, then there's no need to call close on its fileno.

Right. We have to take care of the critical section between fopen and fclose.

The reasoning is the following.
1. The patch works on unices.
2. On windows the patch works if the file is closed.
3. eval_makefile uses fopen and then fclose to open and close the file.
4. On windowns the file is not deleted, if a signal arrives after the file is
opened and before the file is closed by eval_makefile.

One option to handle this critical section on windows is to set the fd of the
file to a file scope variable and then close this fd in temp_stdin_unlink.
Something like this (Not tested on windows).



diff --git a/src/main.c b/src/main.c
index 129dd661..cf87a6a0 100644
--- a/src/main.c
+++ b/src/main.c
@@ -306,6 +306,7 @@ unsigned long command_count = 1;
 /* Remember the location of the name of the batch file from stdin.  */

 static int stdin_offset = -1;
+static int temp_stdin_fileno = -1;

 ^L
 /* The usage output.  We write it this way to make life easier for the
@@ -3714,12 +3715,33 @@ clean_jobserver (int status)
 }
 }

+void
+temp_stdin_set_fileno (const char *name, FILE *f)
+{
+  if (temp_stdin_fileno == -1 && f &&
+  stdin_offset >= 0 && streq (makefiles->list[stdin_offset], name))
+temp_stdin_fileno = fileno (f);
+}
+
+void
+temp_stdin_reset_fileno (const char *name)
+{
+  if (temp_stdin_fileno >= 0 &&
+  stdin_offset >= 0 && streq (makefiles->list[stdin_offset], name))
+temp_stdin_fileno = -1;
+}
+
 void
 temp_stdin_unlink ()
 {
   /* This function is called from a signal handler.
* Keep async-signal-safe.  */
   /* Get rid of a temp file from reading a makefile from stdin.  */
+#ifdef WINDOWS32
+  /* Windows needs the file to be closed for unlink to succeed.  */
+  if (temp_stdin_fileno >= 0 && close (temp_stdin_fileno) == 0)
+temp_stdin_fileno = -1;
+#endif
   if (stdin_offset >= 0 && unlink (makefiles->list[stdin_offset]) == 0)
 stdin_offset = -1;
 }
diff --git a/src/makeint.h b/src/makeint.h
index d0e0fab3..cf2975c6 100644
--- a/src/makeint.h
+++ b/src/makeint.h
@@ -530,6 +530,8 @@ void out_of_memory () NORETURN;
  (_f), (_n), (_s))

 void decode_env_switches (const char*, size_t line);
+void temp_stdin_set_fileno (const char *name, FILE *f);
+void temp_stdin_reset_fileno (const char *name);
 void temp_stdin_unlink ();
 void die (int) NORETURN;
 void pfatal_with_name (const char *) NORETURN;
diff --git a/src/read.c b/src/read.c
index 79315503..b8e7baed 100644
--- a/src/read.c
+++ b/src/read.c
@@ -353,6 +353,7 @@ eval_makefile (const char *filename, unsigned short
flags)
   errno = 0;
   ENULLLOOP (ebuf.fp, fopen (filename, "r"));
   deps->error = errno;
+  temp_stdin_set_fileno (filename, ebuf.fp);

   /* Check for unrecoverable errors: out of mem or FILE slots.  */
   switch (deps->error)
@@ -441,6 +442,7 @@ eval_makefile (const char *filename, unsigned short
flags)
   reading_file = curfile;

   fclose (ebuf.fp);
+  temp_stdin_reset_fileno (filename);

   free (ebuf.bufstart);
   free_alloca ();


There is still a smaller critical section. If a signal arrives between fopen
and temp_stdin_set_fileno, the file is not deleted. Signals can be blocked
during this smaller critical section. But i think, that's an overkill.

> What do you mean by "We won't be able to use fclose, which is okay, since
make is exiting."?

Inside temp_stdin_unlink make can only do async signal safe calls. This rules
out fclose. Make can call close, because close is async signal safe. Calling
close, rather than fclose, leaves the internal FILE bookeeping messed up,
which is okay, because make is exiting.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Eli Zaretskii]

Follow-up Comment #10, bug #63157 (project make):

If temp_stdin is fclosed, then there's no need to call close on its fileno.

I don't think I follow your reasoning.  Could you tell more details, including
pointers to the source code?  What do you mean by "We won't be able to use
fclose, which is okay, since make is exiting."?

In general, yes, a file must be closed before we try to unlink it, otherwise
the deletion will fail and the file will not be deleted.



___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #7, bug #63157 (project make):

close is async signal safe and we call close before unlink.
Should make also calls close before unlink in all other places where make
unlinks this (or any other) file?


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #9, bug #63157 (project make):

In the case of temp_stdin eval_makefile fcloses this file.
We'll need to store temp_stdin_fileno in a file scope variable (next to
stdin_offset) and pass it to close in temp_stdin_unlink.
We won't be able to use fclose, which is okay, since make is exiting.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Eli Zaretskii]

Follow-up Comment #6, bug #63157 (project make):

> If the file is opened at the time of unlink, only file's name is removed
from the directory and the file itself stays. When make exits the last
reference to the file is gone and the file is deleted.

That's what happens on Unix.  On Windows, the unlink call will simply fail
(with EACCES or somesuch), and the file will remain.



___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #8, bug #63157 (project make):

we *can* call close before unlink


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #5, bug #63157 (project make):

> This should be tested on MS-Windows.  A file can only be deleted on
MS-Windows if it isn't open by any program, and the patch (AFAICT) doesn't
make sure the files are closed before unlinking them.  I'm especially worried
about the temp_stdin_unlink part.

If the file is opened at the time of unlink, only file's name is removed from
the directory and the file itself stays. When make exits the last reference to
the file is gone and the file is deleted.


> (Btw, why does the patch use `unlink` instead of a more standard `remove`?)

unlink is used, rather than remove, to stay uniform with the rest of the code,
which uses unlink.



___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Eli Zaretskii]

Follow-up Comment #4, bug #63157 (project make):

This should be tested on MS-Windows.  A file can only be deleted on MS-Windows
if it isn't open by any program, and the patch (AFAICT) doesn't make sure the
files are closed before unlinking them.  I'm especially worried about the
temp_stdin_unlink part.

(Btw, why does the patch use `unlink` instead of a more standard `remove`?)



___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #3, bug #63157 (project make):

Also, tested presence of PPID variable in ksh, bash, ash, zsh, sh.
The only shell that i encountered that lacks PPID is tcsh (likely csh as
well).
The test uses the default shell. This means that test will likely fail on a
system where /bin/sh is csh or tcsh.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #2, bug #63157 (project make):

Tested this patch on linux, sun and aix. All 64 bit.
This patch adds jobserver_unlink stub on windows. As far as i can visually
validate, this stub should compile fine. However, have not tested this on
windows.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Additional Item Attachment, bug #63157 (project make):

File name: sv63157_fix.diff   Size:4 KB


File name: sv63157_test.diff  Size:2 KB




___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

Follow-up Comment #1, bug #63157 (project make):

Make fails to unlink temporary files upon arrival of a fatal signal.
Specifically, the jobserver fifo, output sync lock and the stdin temp file.


___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/

[bug #63157] Unlink temporary files.

[Dmitry Goncharov]

URL:
  

 Summary: Unlink temporary files.
 Project: make
   Submitter: dgoncharov
   Submitted: Wed 05 Oct 2022 02:24:38 AM UTC
Severity: 3 - Normal
  Item Group: Bug
  Status: None
 Privacy: Public
 Assigned to: None
 Open/Closed: Open
 Discussion Lock: Any
   Component Version: SCM
Operating System: Any
   Fixed Release: None
   Triage Status: None


___

Follow-up Comments:


---
Date: Wed 05 Oct 2022 02:24:38 AM UTC By: Dmitry Goncharov 
.







___

Reply to this item at:

  

___
Message sent via Savannah
https://savannah.gnu.org/