Re: [PATCH] libiberty: Fix and cleanup choose_temp_base()

["Ian Lance Taylor via binutils" <binutils at sourceware dot org>]

On Fri, Nov 8, 2019 at 3:25 AM Tim Rühsen <tim.ruehsen@gmx.de> wrote:
>
> On 11/7/19 6:43 PM, Ian Lance Taylor wrote:
> > On Thu, Nov 7, 2019 at 9:09 AM Tim Rühsen <tim.ruehsen@gmx.de> wrote:
> >>
> >> reduced code size by using xasprintf().
> >>
> >> Please review the check of mktemp(), which was formerly checking against
> >> 0 (regarding glibc man pages that was wrong). I left that check intact
> >> (just in case I miss something) and added the check for an empty string,
> >> as documented in the glibc man pages.
> >>
> >> Does it make sense to further replace strlen/malloc/strcpy/strcat
> >> sequences by [x]asprintf in order to reduce source lines and library
> >> (binary) size ? (In the means of "is it appreciated")
> >>
> >> A side effect is calming down static analyzers that dislike unbounded
> >> memory accesses and thus warn about strcpy() and strcat().
> >
> > The new code is simpler but it will run slower.
>
> The commit does two things (I can split these if you want me to do).
>
> 1) Code deduplication
>
> The new code eliminates code duplication. xasprintf("%s%s", ...) is
> basically 2xstrlen, 1xmalloc, 2xstrcpy. The old code just does this
> *manually* inlined (with a small optimization of dropping strlen for an
> 8 byte string).

The xasprintf function has to parse the format string, twice.  There
is no comparable work in the current function.


> While 20 years ago we all where keen at manually inlining and unrolling
> everything to spare even a few CPU cycles, the coding paradigm today is
> bit different (IMO). It's more shifted towards readability,
> maintainability, runtime stability.
>
> But if the project's policy is to keep code just because it's old, I
> accept that (but not very happily).

Sure, we might write it differently today.  But we probably still
wouldn't use xasprintf.

That said, avoiding code churn is a benefit.  If you have working
code, the only reason to change it is to make it faster or remove
bugs.  If you don't do that, the only meaningful effect of changing
existing working code would be to possibly break it.  If code works,
leave it alone.  This code base is not intended as an example for how
to write good code.  It's intended to work.


> 2) Fixing the check of the return value of mktemp()
>
> The current code calls abort() if mktemp() returns NULL. Now read the
> glibc man page of mktemp():
>
> RETURN VALUE
>        The  mktemp()  function always returns template.  If a unique
> name was created, the last six bytes of template will have been modi‐
>        fied in such a way that the resulting name is unique (i.e., does
> not exist already) If a unique name could not be created, template
>        is made an empty string, and errno is set to indicate the error.
>
> a) mktemp() never returns NULL, so the check is wrong.
>
> b) choose_temp_base() either returns a temporary file name OR an empty
> string
>
> c) the documentation of choose_temp_base() does not reflect this
> behavior (it says "Return a prefix for temporary file names or NULL if
> unable to find one").
>
> d) callers of choose_temp_base() rely on the return value being a
> temporary file name (not NULL nor an empty string), e.g. see
> ./libiberty/pex-msdos.c:185
> ./binutils/dlltool.c:1309
> ./binutils/dllwrap.c:354
> ./binutils/dllwrap.c:820
> ./binutils/dllwrap.c:1033
> ./binutils/resrc.c:204
> ./binutils/resrc.c:318
>
>
> If you think that the man page (3) of mktemp() if wrong about the return
> value, let me know and I dig deeper (into glibc sources).
>
> The documentation of choose_temp_base() is definitely incorrect. If you
> agree I extend the commit with a fix (or make a second commit - what you
> think is more appropriate).

The mktemp function behaves differently on different systems.  See,
for example, https://man.openbsd.org/NetBSD-7.0.1/mktemp.3 .  It would
be fine to fix this code to use mktemp in a better way, but it needs
to be portable.  Perhaps these days we can safely call something other
than mktemp; after all, both man pages recommend not using it.
Thanks.

Ian