PATCH: small compilation cleanup for utils.c

PATCH: small compilation cleanup for utils.c

[John T. Guthrie]

Hello all,

This is a very small patch to clean up compilation on RedHat Linux.  (And,
I believe on most anything that uses GLIBC >= 2.0, but I'm not certain of
that.)  When compiling on RedHat Linux 8.0, I get the compilation complaint:

utils.o: In function `gettempname':
utils.o(.text+0x1b1a): the use of `mktemp' is dangerous, better use `mkstemp'

This patch fixes that for systems that have mktemp(), but not _mktemp().  I
saw that configure was testing for both mkstemp() and _mktemp(), but the code
only used _mktemp().  I don't know whether the test for HAVE_MKSTEMP should
come before or after the test for HAVE__MKTEMP, so let me know if there is a
different preferred order.

John Guthrie
guthrie@counterexample.org

--------------------------BEGIN PATCH--------------------------------

*** utils.c.orig        2002-10-15 14:00:00.000000000 -0400
--- utils.c     2003-02-18 02:51:59.000000000 -0500
***************
*** 1108,1119 ****
--- 1108,1123 ----
      if (!(s = getsparam("TMPPREFIX")))
        s = DEFAULT_TMPPREFIX;
   
+ #ifdef HAVE_MKSTEMP
+     ret = ((char *) mkstemp(dyncat(unmeta(s), "XXXXXX")));
+ #else
  #ifdef HAVE__MKTEMP
      /* Zsh uses mktemp() safely, so silence the warnings */
      ret = ((char *) _mktemp(dyncat(unmeta(s), "XXXXXX")));
  #else
      ret = ((char *) mktemp(dyncat(unmeta(s), "XXXXXX")));
  #endif
+ #endif
      unqueue_signals();
  
      return ret;

Re: PATCH: small compilation cleanup for utils.c

[Zefram]

John T. Guthrie wrote:
>+     ret = ((char *) mkstemp(dyncat(unmeta(s), "XXXXXX")));

That's not how to use mkstemp().  It returns a file descriptor, not
the pathname.

-zefram

Re: PATCH: small compilation cleanup for utils.c

[John T. Guthrie]

Zefram wrote:
> John T. Guthrie wrote:
> >+     ret = ((char *) mkstemp(dyncat(unmeta(s), "XXXXXX")));
> 
> That's not how to use mkstemp().  It returns a file descriptor, not
> the pathname.
> 
> -zefram

Mea Culpa!  I guess that is what I get for doing this at 3:30 in the morning,
and then not actually checking that it worked.  (Which 3:30AM may also have
something to do with.)  So anyway, here is a patch that actually uses
mkstemp() correctly, and I've actually verified that it works by using
the =(list) construct.

That said, I have to wonder if this patch is even
worth the trouble.  :-(  I say that because mkstemp actually creates and opens
the file for you.  In every place in the code where gettempname() is
called, the shell subsequently opens the file.  Since dangling file
descriptors is normally considered a bad thing(TM), we want to close the
file descriptor that we are given.  Normally, we could just leave things
this way, and let the shell re-open the file after gettempname() returns.
However, in namedpipe() in exec.c, there is a call to gettempname() that
is immediately followed by either a mknod() or a mkfifo().  These will
fail if the file already exists as a plain file.  Thus, we either have to
unlink() the temporary file that we just created, or add in some more 
intelligence to gettempname() so that it knows something about who is
calling it.  For the time being, I have chosen the simpler path of just
deleting the file, and letting the shell re-create the file later on, but
doesn't this really eliminate alot of the benefit of using mkstemp() in
the first place.

That said, here is a correct version of the patch.

John Guthrie
guthrie@counterexample.org

-----------------------------CUT HERE-------------------------------

*** utils.c.orig        2002-10-15 14:00:00.000000000 -0400
--- utils.c     2003-02-18 23:38:41.000000000 -0500
***************
*** 1103,1119 ****
--- 1103,1130 ----
  gettempname(void)
  {
      char *s, *ret;
+ #ifdef HAVE_MKSTEMP
+     int fd;
+     char *t;
+ #endif

      queue_signals();
      if (!(s = getsparam("TMPPREFIX")))
        s = DEFAULT_TMPPREFIX;

+ #ifdef HAVE_MKSTEMP
+     fd = mkstemp(t = dyncat(unmeta(s), "XXXXXX"));
+     close(fd);
+     unlink(t);
+     ret = t;
+ #else
  #ifdef HAVE__MKTEMP
      /* Zsh uses mktemp() safely, so silence the warnings */
      ret = ((char *) _mktemp(dyncat(unmeta(s), "XXXXXX")));
  #else
      ret = ((char *) mktemp(dyncat(unmeta(s), "XXXXXX")));
  #endif
+ #endif
      unqueue_signals();

      return ret;