Re: [bug-gnulib] clean-temp usage question

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Bruno Haible on 10/5/2006 6:30 AM:
> 
> It's the latter: You can choose arbitrary file names in the temp_dir. For
> example, you could safely unzip a file in the temp_dir (assuming none of
> the file names contain a ".." component :-)).

Hmm.  The gnulib mkdtemp module does not take umask into account.  Neither
does the mkdtemp variant of lib/tempname.c.  Shouldn't these modules be
guaranteeing that the directory created has full user permissions, in
spite of the current umask?  Or is it the caller's responsibility (such as
is documented by the mkdir-p module) to ensure that umask is 0 at the time
of calling mkdtemp/create_temp_dir/...?

> 
>> Also, if I keep M4's usage of FILE*, then I imagine I should ensure that 
>> each 
>> of the file objects is closed before the temp_dir cleanup functions get 
>> invoked.
> 
> Ouch, indeed. It was reported that on Woe32 you cannot delete an open file.
> The Woe32 function CreateFile accepts a FILE_SHARE_DELETE argument which would
> overcome this limitation, but neither the MSVC open() nor the Cygwin open()
> function provide a way to set this flag in the CreateFile call.

Actually, cygwin allows unlinking a file while it is open, unlike native
Windows (as it proves very useful for porting programs with typical
mkstemp/unlink usage patterns, in spite of the fact that POSIX allows
unlink to fail on an open file).  However, cygwin currently inherits
Windows limitations of not deleting a directory when any process has that
directory as its pwd, or when that directory contains an open fd, whether
or not that fd maps to an unlinked file (as evidenced by the patch I made
to gnulib-tool last month).  Interix seems to have come up with some
workaround for this, and maybe a future version of cygwin will be able to
borrow Interix's solution; but I don't use Interix enough to be familiar
with it.  But the point remains - if a fatal signal arrives, the current
implementation of clean-temp blindly calls unlink and rmdir on files that
may still be locked, and will fail to clean up everything on Windows if
the implementation still has any temp files open.

> 
> What I propose is to add a registry of open file descriptors to the clean-temp
> module, and wrappers temp_open, temp_close, temp_fopen, temp_fclose,
> temp_fwriteerror of open, close, fopen, fclose, fwriterror, so that this
> code gets simplified to

fwriterror is still controversial for the reasons Paul mentioned - it
writes to the file descriptor in some situations that close-stream can
currently avoid.

The rest of those wrappers sound reasonable.

Speaking of which, now that coreutils 6.3 is out, we should go ahead and
patch the closeout module to check for write errors on stderr.  That
proposed change was not as controversial as the changes to close-stream.

> 
>   register_temp_file (tmpdir, java_file_name);
>   java_file = temp_fopen (java_file_name, "w");

Would it make more sense for this to resemble openat semantics (passing
filenames relative to the reference point of the tmpdir), and look
something like:
  register_temp_file (tmpdir, java_file_base_name);
  java_file = temp_fopen (tmpdir, java_file_base_name);

with the implied semantics change to register_temp_file that if it is
passed a tmpdir-relative file name, it can construct the appropriate
absolute name rather than requiring users to construct the absolute name?

>   if (java_file == NULL)
>     {
>       error (0, errno, _("failed to create \"%s\""), java_file_name);
>       unregister_temp_file (tmpdir, java_file_name);
>       goto quit;
>     }
>   write_java_code (java_file);
>   if (temp_fwriteerror (java_file))
>     {
>       error (0, errno, _("error while writing \"%s\" file"), java_file_name);
>       goto quit;
>     }
> 
> Would this approach be fine for m4?

Yes, having a way to register open fds/open FILES alongside the filenames
to delete would be helpful.

> 
>> That makes it sound like I would need to register another fatal 
>> handler that gets invoked before the temp_dir fatal_handler (does 
>> at_fatal_handler guarantee LIFO execution of fatal handlers?).
> 
> There is no such guarantee. It is too dangerous to rely on this.
> Actually fatal-signal currently installs its handlers overwriting the
> previously installed signal handler (if any).

I agree that once you use at_fatal_signal, your program cannot manipulate
fatal signal handlers any more, because you have handed that over to the
fatal-signal module.  My question was more along the lines of if you use
at_fatal_signal multiple times, are all of its registrants executed in
LIFO order?  If so, then I can register a fatal signal handler after
calling create_temp_dir to do any cleanup that I know is needed before
temp_dir's cleanup.

>> It would be 
>> nice if fatal-signal could be augmented with a way to register a handler 
>> function that takes a void* argument along with the argument to invoke it 
>> with 
>> (similar to on_exit semantics), at which point I could repeatedly register 
>> the 
>> same handler but with a different FILE* object and let fatal-signal manage 
>> the 
>> list of registered FILE* to close.
> 
> When you do this, you will also need to unregister the (handler function,
> argument) pairs that you have registered. (Otherwise in a long-running
> program such pairs accumulate without bounds.) And now you have a race
> condition: If you do
>     unregister_pair (handler, fp);
>     fclose (fp);
> there is a small chance that the program gets SIGTERM between the two
> statements, and then the temporary file cannot be removed. If, on the
> other hand, you do
>     fclose (fp);
>     unregister_pair (handler, fp);
> then there is a small chance that a SIGTERM arrives between the two
> statements, and the handler tries to access the already freed memory
> at fp, and crashes.
> To avoid this race condition, you need block/unblock_fatal_signals().

Point taken.  So for now, don't worry about implementing an on_exit style
fatal signal handler interface.

- --
Life is short - so eat dessert first!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFJQSS84KuGfSFAYARAurDAKDHmCKzJLHYFwMw3nTnZGvmqQG0/wCffCMv
1I+Zm6es9Qw0HAWiMxCa49M=
=cHW0
-----END PGP SIGNATURE-----