Re: [PATCH] vasnprintf: fix potential use after free

[Pádraig Brady]

On 06/12/14 16:06, Pádraig Brady wrote:
> On 06/12/14 02:46, Eric Blake wrote:
>> On 12/05/2014 06:23 PM, Pádraig Brady wrote:
>>> * lib/vasnprintf.c (VASNPRINTF): Fix free-memory read,
>>> flagged by clang-analyzer 3.4.2.
>>> ---
>>>  ChangeLog        | 6 ++++++
>>>  lib/vasnprintf.c | 2 +-
>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>
>>> +++ b/lib/vasnprintf.c
>>> @@ -5184,13 +5184,13 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
>>>                            free (result);
>>>                          if (buf_malloced != NULL)
>>>                            free (buf_malloced);
>>> -                        CLEANUP ();
>>>                          errno =
>>>                            (saved_errno != 0
>>>                             ? saved_errno
>>>                             : (dp->conversion == 'c' || dp->conversion == 
>>> 's'
>>>                                ? EILSEQ
>>>                                : EINVAL));
>>> +                        CLEANUP ();
>>
>> Ouch.  This is a bug.  The whole point of assigning to errno after
>> CLEANUP() is that CLEANUP() may invalidate the value stored in errno.
>>
>> I suggest doing something like:
>>
>> if (saved_errno == 0)
>>   saved_errno = dp->conversion == 'c' || dp->conversion == 's'
>>     ? EILSEQ : EINVAL;
>> CLEANUP();
>> errno = saved_errno;
> 
> Oh right, I did think about it and looked at:
> http://austingroupbugs.net/view.php?id=385
> but jumped to the wrong conclusion that the updated standard
> was just documenting existing practise.

BTW if free() may reset errno on some platforms then it's
probably worth augmenting the gnulib free() wrapper
to restore errno if needed, rather than worrying about
this sort of stuff everywhere.

cheers,
Pádraig.