Re: Patch for tmpfile bug https://sourceware.org/bugzilla/show_bug.cgi?id=21530

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 06/06/2017 04:12, Petr Skočík wrote:
> Hi,
> 
> I haven't contributed before and I've made a patch for
> https://sourceware.org/bugzilla/show_bug.cgi?id=21530.
> 
> It also fixes a filename leak into the filesystem in the fallback
> (current) code which happens if a signal gets delivered in between open
> and unlink.
> 
> Regards,
> Petr Skocik

Thanks for you time, however I would like to ask you to send any patch to
libc-alpha maillist instead (which is the intended one for patch submission).

With current patch size it is a non-trivial change, so you will probably
need a copyright assignment. Check the step on GLIBC contribution checklist [1].

Some general comments:

  - O_TMPFILE is Linux specific, so it would be better to parametrize the
    open flags using a Linux specific file by providing either a Linux
    specific __gen_tempname or either a open_dir_tmp (with a Linux
    specific O_TMPFILE usage).

  - weakref as not the correct way to call nptl on dynamic mode.  We have
    __libc_ptf_call exactly for this.  Also, for signal blocking and restore
    check the functions at nptl-signals.h.

  - You need to format correctly this patch basing on GNU code guidelines.

[1] https://sourceware.org/glibc/wiki/Contribution%20checklist


> 
> 
> tmpfile.patch
> 
> 
> diff --git a/stdio-common/tmpfile.c b/stdio-common/tmpfile.c
> index e6030be0af..a6c29ed741 100644
> --- a/stdio-common/tmpfile.c
> +++ b/stdio-common/tmpfile.c
> @@ -26,27 +26,49 @@
>  # define tmpfile __new_tmpfile
>  #endif
>  
> +#ifndef O_TMPFILE
> +__attribute__((__weakref__("pthread_sigmask")))
> +static int weakref_to__pthread_sigmask (int How,  sigset_t const *Set, sigset_t *Oldset);
>  
> -/* This returns a new stream opened on a temporary file (generated
> -   by tmpnam).  The file is opened with mode "w+b" (binary read/write).
> -   If we couldn't generate a unique filename or the file couldn't
> -   be opened, NULL is returned.  */
> -FILE *
> -tmpfile (void)
> +static int some_sigmask(int How,  sigset_t const *Set, sigset_t *Oldset)
> +{
> +    return weakref_to__pthread_sigmask ?
> +        weakref_to__pthread_sigmask (How,Set,Oldset)
> +        : sigprocmask (How,Set,Oldset);
> +}
> +
> +static void block_sigs(sigset_t *Oldset)
> +{
> +    sigset_t all_set;
> +    sigfillset (&all_set);
> +    some_sigmask (SIG_SETMASK, &all_set, Oldset);
> +}
> +static void unblock_sigs(sigset_t const *Oldset)
> +{
> +    some_sigmask (SIG_SETMASK, Oldset, 0);
> +}
> +
> +static FILE *
> +tmpfile_classical__ (void)
>  {
>    char buf[FILENAME_MAX];
>    int fd;
>    FILE *f;
> +  sigset_t saved_mask;
> +  int flags = 0;
>  
>    if (__path_search (buf, FILENAME_MAX, NULL, "tmpf", 0))
>      return NULL;
> -  int flags = 0;
> -#ifdef FLAGS
> +
> +# if FLAGS
>    flags = FLAGS;
> -#endif
> +# endif
> +
> +  block_sigs (&saved_mask);
>    fd = __gen_tempname (buf, 0, flags, __GT_FILE);
> +
>    if (fd < 0)
> -    return NULL;
> +      { f=NULL; goto out; }
>  
>    /* Note that this relies on the Unix semantics that
>       a file is not really removed until it is closed.  */
> @@ -55,8 +77,40 @@ tmpfile (void)
>    if ((f = __fdopen (fd, "w+b")) == NULL)
>      __close (fd);
>  
> +out:
> +  unblock_sigs (&saved_mask);
>    return f;
>  }
> +#endif
> +
> +/* This returns a new stream opened on a temporary file (generated
> +   by tmpnam).  The file is opened with mode "w+b" (binary read/write).
> +   If we couldn't generate a unique filename or the file couldn't
> +   be opened, NULL is returned.  */
> +FILE *
> +tmpfile (void)
> +{
> +#if !defined(O_TMPFILE)
> +  return tmpfile_classical__ ();
> +#else
> +  char buf[FILENAME_MAX];
> +  int fd;
> +  FILE *f;
> +
> +  if (__path_search (buf, FILENAME_MAX, NULL, "tmpf", 0))
> +    return NULL;
> +  *(strrchr (buf, '/')) = 0;
> +  fd = open (buf, O_RDWR|O_TMPFILE);
> +
> +  if (fd < 0)
> +      return NULL;
> +
> +  if ((f = __fdopen (fd, "w+b")) == NULL)
> +    __close (fd);
> +
> +  return f;
> +#endif
> +}
>  
>  #if !defined O_LARGEFILE || O_LARGEFILE == 0
>  weak_alias (__new_tmpfile, tmpfile64)
>