"dangerous" warning question

"dangerous" warning question

[NightStrike]

When compiling binutils, I receive this warning:

../../src/libiberty/choose-temp.c:68: warning: the use of `mktemp' is
dangerous, better use `mkstemp'

Is this something I can do anything about, such as via a configure
option?  What exactly does this mean?

Re: "dangerous" warning question

[Brian Dessent]

NightStrike wrote:

> When compiling binutils, I receive this warning:
> 
> ../../src/libiberty/choose-temp.c:68: warning: the use of `mktemp' is
> dangerous, better use `mkstemp'
> 
> Is this something I can do anything about, such as via a configure
> option?  What exactly does this mean?

This is a warning from glibc.  It's telling you that you're building
code that uses the mktemp function which is considered dangerous by
design due to the race condition inherent between when the filename is
checked for existance and its subsequent open().  libiberty acknowledges
this deficiency:

> @deftypefn Extension char* choose_temp_base (void)
> 
> Return a prefix for temporary file names or @code{NULL} if unable to
> find one.  The current directory is chosen if all else fails so the
> program is exited if a temporary directory can't be found (@code{mktemp}
> fails).  The buffer for the result is obtained with @code{xmalloc}.
> 
> This function is provided for backwards compatibility only.  Its use is
> not recommended.

It's not likely that choose_temp_base() could ever be removed from
libiberty since this library is shared by lots of projects, but at least
as a first step it would be a good idea I suppose if all consumers of it
in binutils were removed.  At the moment that seems to be just
dlltool.c, dllwrap.c, and resrc.c, which is not surprising as these tend
to suffer from a lot of bitrot anyway.  But that's not going to do
anything to remove the warning, as you will always get that when
building libiberty on a glibc host even if you removed all
choose_temp_base callers.

As far as I know there is no way to disable the warning in glibc, and
the glibc developers are not interested in hearing any complaints about
it.  The fact that you don't get the warning on other platforms doesn't
mean mktemp() isn't inherently broken/insecure there too, it just means
that glibc is particularly militant about spreading their ideas on these
matters.

Brian

Re: "dangerous" warning question

[NightStrike]

On 7/14/07, Brian Dessent <brian@dessent.net> wrote:
> NightStrike wrote:
>
> > When compiling binutils, I receive this warning:
> >
> > ../../src/libiberty/choose-temp.c:68: warning: the use of `mktemp' is
> > dangerous, better use `mkstemp'
> >
> > Is this something I can do anything about, such as via a configure
> > option?  What exactly does this mean?
>
> This is a warning from glibc.  It's telling you that you're building
> code that uses the mktemp function which is considered dangerous by
> design due to the race condition inherent between when the filename is
> checked for existance and its subsequent open().  libiberty acknowledges
> this deficiency:
>
> > @deftypefn Extension char* choose_temp_base (void)
> >
> > Return a prefix for temporary file names or @code{NULL} if unable to
> > find one.  The current directory is chosen if all else fails so the
> > program is exited if a temporary directory can't be found (@code{mktemp}
> > fails).  The buffer for the result is obtained with @code{xmalloc}.
> >
> > This function is provided for backwards compatibility only.  Its use is
> > not recommended.
>
> It's not likely that choose_temp_base() could ever be removed from
> libiberty since this library is shared by lots of projects, but at least
> as a first step it would be a good idea I suppose if all consumers of it
> in binutils were removed.  At the moment that seems to be just
> dlltool.c, dllwrap.c, and resrc.c, which is not surprising as these tend
> to suffer from a lot of bitrot anyway.  But that's not going to do
> anything to remove the warning, as you will always get that when
> building libiberty on a glibc host even if you removed all
> choose_temp_base callers.
>
> As far as I know there is no way to disable the warning in glibc, and
> the glibc developers are not interested in hearing any complaints about
> it.  The fact that you don't get the warning on other platforms doesn't
> mean mktemp() isn't inherently broken/insecure there too, it just means
> that glibc is particularly militant about spreading their ideas on these
> matters.


Would the best course of action be to remove calls to choose_temp_base
from those aforementioned files, and to further change the way
choose_temp_base works to call mkstemp instead of mktemp?  Or would
that break other things?

I'm obviously a fledgling in this regard -- just trying to learn, so I
appreciate your patience and your verbosity.  The indepth answers that
you, and everyone on this list, provides is immensely helpful.

Re: "dangerous" warning question

[Brian Dessent]

NightStrike wrote:

> Would the best course of action be to remove calls to choose_temp_base
> from those aforementioned files, and to further change the way
> choose_temp_base works to call mkstemp instead of mktemp?  Or would
> that break other things?

No, the problem is that both choose_temp_base and mktemp represent the
same broken semantics, namely "give me the name of a unique temporary
file that does not exist but do not open it yet."  mkstemp is not a drop
in replacement for mktemp because it solves the race condition by doing
the uniqueness check and the opening of the file as an atomic step, and
returns the integer fd of this newly opened file.

You could implement choose_temp_base in terms of mkstemp but that would
be kind of silly in that the temporary file would be created, opened,
and closed, only to later be opened again by the caller.  And there
would still be a possibility of a race condition, although it would
probably be much harder to exploit.

And in fact the above already exists in libiberty as make_temp_file, so
it looks like the best short term solution would be to replace users of
choose_temp_base with that.  (Although there is also make_tempname in
bucomm.c that might be usable but this has the strange logic that if the
target doesn't have mkstemp it falls back to mktemp instead of using the
mkstemps replacement that's in libiberty...)

> I'm obviously a fledgling in this regard -- just trying to learn, so I
> appreciate your patience and your verbosity.  The indepth answers that
> you, and everyone on this list, provides is immensely helpful.

And I was wrong about how the glibc warning works.  Since it's
implemented as a link-time thing you should only get the warning if
there is a call to mktemp in the final link, meaning that if all callers
to choose_temp_base are cleaned up it shouldn't matter that there is
still a call to mktemp in libiberty.a as that's dead code.

Brian

Re: "dangerous" warning question

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

On Sunday 15 July 2007, Brian Dessent wrote:
> And I was wrong about how the glibc warning works.  Since it's
> implemented as a link-time thing you should only get the warning if
> there is a call to mktemp in the final link, meaning that if all callers
> to choose_temp_base are cleaned up it shouldn't matter that there is
> still a call to mktemp in libiberty.a as that's dead code.

i'd say make sure you check out the code first and how it's used before 
spending time on fixing something that merely makes a warning go away.  if 
you look at the mktemp() warning from binutils/bucomm.c, you'll see that it 
is pointless to fix the warning in that file considering the usage model.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: "dangerous" warning question

[NightStrike]

On 7/15/07, Brian Dessent <brian@dessent.net> wrote:
> NightStrike wrote:
>
> And in fact the above already exists in libiberty as make_temp_file, so
> it looks like the best short term solution would be to replace users of
> choose_temp_base with that.  (Although there is also make_tempname in
> bucomm.c that might be usable but this has the strange logic that if the
> target doesn't have mkstemp it falls back to mktemp instead of using the
> mkstemps replacement that's in libiberty...)


Would this patch be acceptable?


Index: binutils/dlltool.c
===================================================================
RCS file: /cvs/src/src/binutils/dlltool.c,v
retrieving revision 1.78
diff -u -r1.78 dlltool.c
--- binutils/dlltool.c  5 Jul 2007 16:54:45 -0000       1.78
+++ binutils/dlltool.c  27 Aug 2007 03:34:51 -0000
@@ -1174,7 +1174,7 @@
   int i;
   const char **argv;
   char *errmsg_fmt, *errmsg_arg;
-  char *temp_base = choose_temp_base ();
+  char *temp_base = make_temp_file (NULL);

   inform ("run: %s %s", what, args);

Index: binutils/dllwrap.c
===================================================================
RCS file: /cvs/src/src/binutils/dllwrap.c,v
retrieving revision 1.23
diff -u -r1.23 dllwrap.c
--- binutils/dllwrap.c  5 Jul 2007 16:54:45 -0000       1.23
+++ binutils/dllwrap.c  27 Aug 2007 03:34:51 -0000
@@ -346,7 +346,7 @@
   int i;
   const char **argv;
   char *errmsg_fmt, *errmsg_arg;
-  char *temp_base = choose_temp_base ();
+  char *temp_base = make_temp_file (NULL);
   int in_quote;
   char sep;

@@ -799,7 +799,7 @@

   if (! def_file_seen)
     {
-      char *fileprefix = choose_temp_base ();
+      char *fileprefix = make_temp_file (NULL);

       def_file_name = (char *) xmalloc (strlen (fileprefix) + 5);
       sprintf (def_file_name, "%s.def",
@@ -982,7 +982,7 @@

   if (! base_file_name)
     {
-      char *fileprefix = choose_temp_base ();
+      char *fileprefix = make_temp_file (NULL);
       base_file_name = (char *) xmalloc (strlen (fileprefix) + 6);
       sprintf (base_file_name, "%s.base",
               (dontdeltemps) ? mybasename (fileprefix) : fileprefix);
Index: binutils/resrc.c
===================================================================
RCS file: /cvs/src/src/binutils/resrc.c,v
retrieving revision 1.32
diff -u -r1.32 resrc.c
--- binutils/resrc.c    5 Jul 2007 16:54:45 -0000       1.32
+++ binutils/resrc.c    27 Aug 2007 03:34:51 -0000
@@ -207,7 +207,7 @@
   int i;
   const char **argv;
   char *errmsg_fmt, *errmsg_arg;
-  char *temp_base = choose_temp_base ();
+  char *temp_base = make_temp_file (NULL);
   int in_quote;
   char sep;
   int redir_handle = -1;
@@ -318,9 +318,8 @@
 {
   if (istream_type == ISTREAM_FILE)
     {
-      char *fileprefix;
+      char *fileprefix = make_temp_file (NULL);

-      fileprefix = choose_temp_base ();
       cpp_temp_file = (char *) xmalloc (strlen (fileprefix) + 5);
       sprintf (cpp_temp_file, "%s.irc", fileprefix);
       free (fileprefix);

Re: "dangerous" warning question

[Ian Lance Taylor]

NightStrike <nightstrike@gmail.com> writes:

> On 7/15/07, Brian Dessent <brian@dessent.net> wrote:
> > NightStrike wrote:
> >
> > And in fact the above already exists in libiberty as make_temp_file, so
> > it looks like the best short term solution would be to replace users of
> > choose_temp_base with that.  (Although there is also make_tempname in
> > bucomm.c that might be usable but this has the strange logic that if the
> > target doesn't have mkstemp it falls back to mktemp instead of using the
> > mkstemps replacement that's in libiberty...)
> 
> 
> Would this patch be acceptable?

Since that patch would not work correctly, I don't think it would be
acceptable.  choose_temp_base and make_temp_file do not do the same
thing.

Ian