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

[Bruno Haible]

Eric Blake wrote:
> It looks like using mkstemp
> (<temp_dir.dir_name>/templateXXXXXX) is not safe; I must instead invent a 
> name, 
> call register_temp_file with that name, then create the file, then 
> unregister_temp_file if the creation failed.  When creating the name, do I 
> have 
> to worry about making the name random, or can I just use sequential file 
> names 
> under the assumption that the directory is already secure?

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

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

A typical use-case of clean-temp is like this:

  register_temp_file (tmpdir, java_file_name);
  java_file = fopen (java_file_name, "w");
  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 (fwriteerror (java_file))
    {
      error (0, errno, _("error while writing \"%s\" file"), java_file_name);
      goto quit;
    }

With the need to protect the open FILEs to temporary files the code looks
roughly like this:

  register_temp_file (tmpdir, java_file_name);
  block_fatal_signals ();
  java_file = fopen (java_file_name, "w");
  register_temp_FILE (java_file_name, java_file);
  unblock_fatal_signals ();
  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 (fwriteerror (temp_before_fclose (java_file)))
    {
      error (0, errno, _("error while writing \"%s\" file"), java_file_name);
      goto quit;
    }
  temp_after_fclose (java_file);

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

  register_temp_file (tmpdir, java_file_name);
  java_file = temp_fopen (java_file_name, "w");
  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?

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

> Right now, 
> at_fatal_handler does not pass an argument to the handler; so I would have to 
> register a single handler, and maintain my own list of FILE* objects that 
> mirrors temp_dir's list of files to clean up, which sounds messy.

Such a list needs to be maintained, because there is a single handler
for each of the signals and possibly many FILEs need to be closed.

> 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().
Now think about which alternative is better in case of a multithreaded
program... and which is better in case the block/unblock_fatal_signals()
is a nop...
Thus you get deeper and deeper in tricky code...

I believe in simple-to-use interfaces; that's why I modeled the
fatal-signals interface after atexit().

Bruno