[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Do not use %m in common code to print error messages
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH] Do not use %m in common code to print error messages |
Date: |
Fri, 18 Oct 2019 12:37:03 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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 <address@hidden>
> >> ---
> >> 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.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|