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

[Tim Rühsen <tim dot ruehsen at gmx dot de>]

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 part with the biggest CPU cycle impact is malloc(), whether you call
 xasprintf() or inline the code - it doesn't change much regarding total
used CPU cycles.

But even much more cycles may be required by mktemp() which goes down
into the kernel (several times), goes through file system code and does
file I/O (which may even block).

So I can't really follow your argument of "will run slower".

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


> The existing code has
> been there for 20 years, I doubt it's buggy.  I don't see a benefit to
> making this change.

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

Regards, Tim

Attachment: signature.asc
Description: OpenPGP digital signature