bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Bruno Haible
Subject: Re: [bug-gnulib] clean-temp usage question
Date: Fri, 6 Oct 2006 14:14:45 +0200
User-agent: KMail/1.9.1

Eric Blake wrote:
> 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/...?

It is on purpose that these temporary directories are created with mode 700.
Many people have an umask of 002, which means that they make many of their
files group-writable, but not all of them. If a temporary directory was
created with mode 777 & ~002 = 775, anyone in the group could place a
symlink in the temporary directory, such that when a file is created in
there, it erase the symlink's target. So an attacker could erase even files
of mode 600 (private files of the user). This is called a "priviledge
escalation".

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

I don't see a point in doing that. 'clean-temp' has one purpose: cleaning up
temporary directories and files. For this, it does not need to do pathname
manipulations. "Let it do one thing and do it well", is the philosophy.
You can do pathname manipulations yourself. The 'pathname' is handy enough.

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

OK. Can you try the patch below?

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

The answer is that you can currently call at_fatal_signal only once.

> fwriterror is still controversial for the reasons Paul mentioned

But I need it in GNU gettext.

Bruno


2006-10-05  Bruno Haible  <address@hidden>

        * lib/clean-temp.h (open_temp, fopen_temp, close_temp, fclose_temp,
        fwriteerror_temp): New declarations.
        * lib/clean-temp.c (uintptr_t): Provide fallback definition.
        (descriptors): New variable.
        (cleanup): First, close the descriptors.
        (register_fd, unregister_fd, open_temp, fopen_temp, close_temp,
        fclose_temp, fwriteerror_temp): New functions.
        * modules/fwriteerror (configure.ac): Define GNULIB_FWRITEERROR.

*** gnulib-20060928/lib/clean-temp.h    2006-07-02 13:55:40.000000000 +0200
--- gnulib-20060928-modified/lib/clean-temp.h   2006-10-06 03:32:01.000000000 
+0200
***************
*** 20,25 ****
--- 20,27 ----
  #define _CLEAN_TEMP_H
  
  #include <stdbool.h>
+ #include <stdio.h>
+ #include <sys/types.h>
  
  #ifdef __cplusplus
  extern "C" {
***************
*** 32,37 ****
--- 34,43 ----
     also - if no signal blocking is used - leaves a time window where a fatal
     signal would not clean up the temporary file.
  
+    Also, open file descriptors need to be closed before the temporary files
+    and the temporary directories can be removed, because only on Unix
+    (excluding Cygwin) one can remove directories containing open files.
+ 
     This module provides support for temporary directories and temporary files
     inside these temporary directories.  Temporary files without temporary
     directories are not supported here.  */
***************
*** 98,103 ****
--- 104,123 ----
     DIR cannot be used any more after this call.  */
  extern void cleanup_temp_dir (struct temp_dir *dir);
  
+ /* Open a temporary file in a temporary directory.
+    Registers the resulting file descriptor to be closed.  */
+ extern int open_temp (const char *file_name, int flags, mode_t mode);
+ extern FILE * fopen_temp (const char *file_name, const char *mode);
+ 
+ /* Close a temporary file in a temporary directory.
+    Unregisters the previously registered file descriptor.  */
+ extern int close_temp (int fd);
+ extern int fclose_temp (FILE *fp);
+ 
+ /* Like fwriteerror.
+    Unregisters the previously registered file descriptor.  */
+ extern int fwriteerror_temp (FILE *fp);
+ 
  
  #ifdef __cplusplus
  }
*** gnulib-20060928/lib/clean-temp.c    2006-09-19 00:51:16.000000000 +0200
--- gnulib-20060928-modified/lib/clean-temp.c   2006-10-06 03:37:05.000000000 
+0200
***************
*** 41,46 ****
--- 41,50 ----
  
  #define _(str) gettext (str)
  
+ #ifndef uintptr_t
+ # define uintptr_t unsigned long
+ #endif
+ 
  
  /* The use of 'volatile' in the types below (and ISO C 99 section 5.1.2.3.(5))
     ensure that while constructing or modifying the data structures, the field
***************
*** 70,75 ****
--- 74,82 ----
    size_t tempdir_allocated;
  } cleanup_list /* = { NULL, 0, 0 } */;
  
+ /* List of all open file descriptors to temporary files.  */
+ static gl_list_t /* <int> */ volatile descriptors;
+ 
  
  /* For the subdirs and for the files, we use a gl_list_t of type LINKEDHASH.
     Why?  We need a data structure that
***************
*** 154,159 ****
--- 161,185 ----
  {
    size_t i;
  
+   /* First close all file descriptors to temporary files.  */
+   {
+     gl_list_t fds = descriptors;
+ 
+     if (fds != NULL)
+       {
+       gl_list_iterator_t iter;
+       const void *element;
+ 
+       iter = gl_list_iterator (fds);
+       while (gl_list_iterator_next (&iter, &element, NULL))
+         {
+           int fd = (int) (uintptr_t) element;
+           close (fd);
+         }
+       gl_list_iterator_free (&iter);
+       }
+   }
+ 
    for (i = 0; i < cleanup_list.tempdir_count; i++)
      {
        struct tempdir *dir = cleanup_list.tempdir_list[i];
***************
*** 481,483 ****
--- 507,646 ----
    /* The user passed an invalid DIR argument.  */
    abort ();
  }
+ 
+ 
+ /* Register a file descriptor to be closed.  */
+ static void
+ register_fd (int fd)
+ {
+   if (descriptors == NULL)
+     descriptors = gl_list_create_empty (GL_LINKEDHASH_LIST, NULL, NULL, 
false);
+   gl_list_add_first (descriptors, (void *) (uintptr_t) fd);
+ }
+ 
+ /* Unregister a file descriptor to be closed.  */
+ static void
+ unregister_fd (int fd)
+ {
+   gl_list_t fds = descriptors;
+   gl_list_node_t node;
+ 
+   if (fds == NULL)
+     /* descriptors should already contain fd.  */
+     abort ();
+   node = gl_list_search (fds, (void *) (uintptr_t) fd);
+   if (node == NULL)
+     /* descriptors should already contain fd.  */
+     abort ();
+   gl_list_remove_node (fds, node);
+ }
+ 
+ /* Open a temporary file in a temporary directory.
+    Registers the resulting file descriptor to be closed.  */
+ int
+ open_temp (const char *file_name, int flags, mode_t mode)
+ {
+   int fd;
+   int saved_errno;
+ 
+   block_fatal_signals ();
+   fd = open (file_name, flags, mode);
+   saved_errno = errno;
+   if (fd >= 0)
+     register_fd (fd);
+   unblock_fatal_signals ();
+   errno = saved_errno;
+   return fd;
+ }
+ 
+ /* Open a temporary file in a temporary directory.
+    Registers the resulting file descriptor to be closed.  */
+ FILE *
+ fopen_temp (const char *file_name, const char *mode)
+ {
+   FILE *fp;
+   int saved_errno;
+ 
+   block_fatal_signals ();
+   fp = fopen (file_name, mode);
+   saved_errno = errno;
+   if (fp != NULL)
+     {
+       /* It is sufficient to register fileno (fp) instead of the entire fp,
+        because at cleanup time there is no need to do an fflush (fp); a
+        close (fileno (fp)) will be enough.  */
+       int fd = fileno (fp);
+       if (!(fd >= 0))
+       abort ();
+       register_fd (fd);
+     }
+   unblock_fatal_signals ();
+   errno = saved_errno;
+   return fp;
+ }
+ 
+ /* Close a temporary file in a temporary directory.
+    Unregisters the previously registered file descriptor.  */
+ int
+ close_temp (int fd)
+ {
+   if (fd >= 0)
+     {
+       /* No blocking of signals is needed here, since a double close of a
+        file descriptor is harmless.  */
+       int result = close (fd);
+       int saved_errno = errno;
+ 
+       /* No race condition here: we assume a single-threaded program, hence
+        fd cannot be re-opened here.  */
+ 
+       unregister_fd (fd);
+ 
+       errno = saved_errno;
+       return result;
+     }
+   else
+     return close (fd);
+ }
+ 
+ /* Close a temporary file in a temporary directory.
+    Unregisters the previously registered file descriptor.  */
+ int
+ fclose_temp (FILE *fp)
+ {
+   int fd = fileno (fp);
+   /* No blocking of signals is needed here, since a double close of a
+      file descriptor is harmless.  */
+   int result = fclose (fp);
+   int saved_errno = errno;
+ 
+   /* No race condition here: we assume a single-threaded program, hence
+      fd cannot be re-opened here.  */
+ 
+   unregister_fd (fd);
+ 
+   errno = saved_errno;
+   return result;
+ }
+ 
+ #if GNULIB_FWRITEERROR
+ /* Like fwriteerror.
+    Unregisters the previously registered file descriptor.  */
+ int
+ fwriteerror_temp (FILE *fp)
+ {
+   int fd = fileno (fp);
+   /* No blocking of signals is needed here, since a double close of a
+      file descriptor is harmless.  */
+   int result = fwriteerror (fp);
+   int saved_errno = errno;
+ 
+   /* No race condition here: we assume a single-threaded program, hence
+      fd cannot be re-opened here.  */
+ 
+   unregister_fd (fd);
+ 
+   errno = saved_errno;
+   return result;
+ }
+ #endif
*** gnulib-20060928/modules/fwriteerror 2005-05-06 19:22:45.000000000 +0200
--- gnulib-20060928-modified/modules/fwriteerror        2006-10-06 
03:31:43.000000000 +0200
***************
*** 9,14 ****
--- 9,16 ----
  stdbool
  
  configure.ac:
+ AC_DEFINE([GNULIB_FWRITEERROR], 1,
+   [Define to 1 when using the gnulib fwriteerror module.])
  
  Makefile.am:
  lib_SOURCES += fwriteerror.h fwriteerror.c




reply via email to

[Prev in Thread] Current Thread [Next in Thread]