Re: asprintf() issue

["Carlos O'Donell" <carlos at redhat dot com>]

On 05/18/2015 06:46 AM, Florian Weimer wrote:
> On 05/14/2015 07:30 AM, Carlos O'Donell wrote:
>> On 05/13/2015 01:24 PM, Joseph Myers wrote:
>>> On Wed, 13 May 2015, Carlos O'Donell wrote:
>>>
>>>> My preference is that we set it to NULL. This will aid in debugging as any
>>>> dereferences to NULL will immediately trap. Leaving the value unchanged
>>>> could result in further manipulation of an invalid memory location and
>>>> program corruption.
>>>
>>> If we do this, do we then want to
>>>
>>> (a) not have a new symbol version; or
>>>
>>> (b) have a new symbol version with the old version being an alias of the 
>>> new (so that new binaries that may rely on it being set to NULL don't run 
>>> with old glibc - similar to the symbol versioning of <fenv.h> functions 
>>> whose return type changed from void to int in C99 TC1, for example); or
>>>
>>> (c) have a new symbol version with the old version not changing *ptr on 
>>> error?
>>
>> IMO we should be conservative and do (c), and document in NEWS, Release wiki
>> page, and hopefully the manual.
> 
> I don't think this is worth the cost.  (Even such little changes add up
> and eventually impact linking time and code size.)  It does not even fix
> a bug, and application code can easily set *ptr to NULL before calling
> asprintf, to get uniform behavior across all known implementations (if
> that simplifies application code).

I disagree that the compat symbol is not worth the cost.

Such a change stands to break binaries that were previously working, and in 
glibc we stand by our community commitment not to break user code.

In this case it's harder to draw a clear line because the API is BSD-based
and our assurances are weaker, but we should not throw away backwards
compatibility without a serious discussion.

Yes, I agree it is easy to fix the code, but that should not be required
if you keep your binary and run it on a newer system.

The impact on linking and code size are minor, and if we want to fix that
we've had at least two patches to make dynamic linking significantly faster
by fixing the O(n^2) symbol sort algorithm in the core code.
 
> So it turns out the asprintf manual pages is correct as-is, because the
> manual pages have a tendency to describe more than just current GNU libc
> behavior.
 
Correctness is not all that relevant. The better question is what behaviour
is better for our users? Pedantically speaking saying it's undefined is always
correct because the implementation can do what it wants.

I think we should do the following:

(a) Rewrite the man page to say we set the pointer to NULL on error.
    Document that this behaviour changed in glibc 2.22.

(b) Add a versioned function to support old binaries, and have the 
    new implementation set the pointer to NULL on error.

Then we're done and move on to the next problem.

Cheers,
Carlos.