Re: [PATCH] Do not use %m in common code to print error messages

[Thomas Huth <thuth@redhat.com>]

On 18/10/2019 13.37, Daniel P. Berrangé wrote:
> On Fri, Oct 18, 2019 at 01:31:34PM +0200, Thomas Huth wrote:
>> On 18/10/2019 12.57, Daniel P. Berrangé wrote:
>>> On Fri, Oct 18, 2019 at 12:44:38PM +0200, Thomas Huth wrote:
>>>> The %m format specifier is an extension from glibc - and when compiling
>>>> QEMU for NetBSD, the compiler correctly complains, e.g.:
>>>>
>>>> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c: In function 'sigfd_handler':
>>>> /home/qemu/qemu-test.ELjfrQ/src/util/main-loop.c:64:13: warning: %m is only
>>>>  allowed in syslog(3) like functions [-Wformat=]
>>>>              printf("read from sigfd returned %zd: %m\n", len);
>>>>              ^
>>>> Let's use g_strerror() here instead, which is an easy-to-use wrapper
>>>> around the thread-safe strerror_r() function.
>>>>
>>>> While we're at it, also convert the "printf()" in main-loop.c into
>>>> the preferred "error_report()".
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  hw/misc/tmp421.c | 8 ++++++--
>>>>  util/main-loop.c | 4 +++-
>>>>  util/systemd.c   | 5 +++--
>>>>  3 files changed, 12 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/misc/tmp421.c b/hw/misc/tmp421.c
>>>> index 9f044705fa..f23c46a40a 100644
>>>> --- a/hw/misc/tmp421.c
>>>> +++ b/hw/misc/tmp421.c
>>>> @@ -120,7 +120,9 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name,
>>>>      int tempid;
>>>>  
>>>>      if (sscanf(name, "temperature%d", &tempid) != 1) {
>>>> -        error_setg(errp, "error reading %s: %m", name);
>>>> +        const char *errmsg = g_strerror(errno);
>>>> +        error_setg(errp, "error reading %s: %s", name, errmsg);
>>>> +        g_free((gpointer)errmsg);
>>>
>>> Kaboom crash. This is trying to free a const string that is the caller
>>> doesn't own. It remains under ownership of g_strerror forever.
>>
>> Well, if you look at the implementation of g_strerror(), you can see
>> that glib returns allocated memory here (with g_strdup() for example).
>> So if we don't free, this will leak memory over time.
> 
> This is *not* a leak, it is a cache.
> 
> It is maintaining a static hash table that caches the mapping from
> errno -> string. So many calls all with the same errno value will
> return the same cached string. So by free'ing the string you're
> causing a use-after-free flaw the next time the same errno is
> passed to g_strerror. The memory usage here is finite, bounded
> by the number of possible errnos that we see.
> 
>> But you're right, the "const" and documentation of the function indicate
>> that the caller should rather not free the memory. What a bummer, sounds
>> like g_strerror() is pretty much useless unless you immediately exit()
>> afterwards (and you care about not leaking memory).
>>
>> I guess we have to implement our own wrapper, qemu_strerror() instead?
> 
> No, g_strerror is working fine without free'ing the result.

Ah, well, thanks for the explanation, not sure how I could have missed
that when looking at the source code :-/ Time for weekend, I guess...

So g_strerror should be fine, and I'll respin the patch without g_free.

 Thomas