This fixes LeakSanitizer complaints with xkbcommon 1.6.0.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
qemu-keymap.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/qemu-keymap.c b/qemu-keymap.c
index 8c80f7a4ed65..7a9f38cf9863 100644
--- a/qemu-keymap.c
+++ b/qemu-keymap.c
@@ -237,6 +237,9 @@ int main(int argc, char *argv[])
xkb_state_unref(state);
state = NULL;
+ xkb_keymap_unref(map);
+ xkb_context_unref(ctx);
+
/* add quirks */
fprintf(outfile,
"\n"
--
2.45.1On Wed, 22 May 2024 at 11:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > This fixes LeakSanitizer complaints with xkbcommon 1.6.0. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > qemu-keymap.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/qemu-keymap.c b/qemu-keymap.c > index 8c80f7a4ed65..7a9f38cf9863 100644 > --- a/qemu-keymap.c > +++ b/qemu-keymap.c > @@ -237,6 +237,9 @@ int main(int argc, char *argv[]) > xkb_state_unref(state); > state = NULL; > > + xkb_keymap_unref(map); > + xkb_context_unref(ctx); > + > /* add quirks */ > fprintf(outfile, > "\n" This is surely a sanitizer bug. We're unconditionally about to exit() the program here, where everything is freed, so nothing is leaked. thanks -- PMM
On Wed, May 22, 2024 at 12:35:23PM +0100, Peter Maydell wrote: > On Wed, 22 May 2024 at 11:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > > > This fixes LeakSanitizer complaints with xkbcommon 1.6.0. > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > --- > > qemu-keymap.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/qemu-keymap.c b/qemu-keymap.c > > index 8c80f7a4ed65..7a9f38cf9863 100644 > > --- a/qemu-keymap.c > > +++ b/qemu-keymap.c > > @@ -237,6 +237,9 @@ int main(int argc, char *argv[]) > > xkb_state_unref(state); > > state = NULL; > > > > + xkb_keymap_unref(map); > > + xkb_context_unref(ctx); > > + > > /* add quirks */ > > fprintf(outfile, > > "\n" > > This is surely a sanitizer bug. We're unconditionally about > to exit() the program here, where everything is freed, so nothing > is leaked. I'm not sure I'd call it a sanitizer bug, rather its expected behaviour of sanitizers. Even if you're about to exit, its important to see info about all memory that is not freed by that time, since it can reveal leaks that were ongoing in the process that are valid things to fix. To make the sanitizers usable you need to get rid of the noise. IOW, either have to provide a file to supress reports of memory that is expected to remain allocated, or have to free it despite being about to exit. Free'ing is the more maintainable strategy, as IME, supression files get outdated over time. So as long as the free'ing action is not unreasonably expensive, we should just do its, so from my POV I'd Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With 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 :|
On Wed, 22 May 2024 at 12:47, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, May 22, 2024 at 12:35:23PM +0100, Peter Maydell wrote: > > On Wed, 22 May 2024 at 11:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > > > > > This fixes LeakSanitizer complaints with xkbcommon 1.6.0. > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > --- > > > qemu-keymap.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/qemu-keymap.c b/qemu-keymap.c > > > index 8c80f7a4ed65..7a9f38cf9863 100644 > > > --- a/qemu-keymap.c > > > +++ b/qemu-keymap.c > > > @@ -237,6 +237,9 @@ int main(int argc, char *argv[]) > > > xkb_state_unref(state); > > > state = NULL; > > > > > > + xkb_keymap_unref(map); > > > + xkb_context_unref(ctx); > > > + > > > /* add quirks */ > > > fprintf(outfile, > > > "\n" > > > > This is surely a sanitizer bug. We're unconditionally about > > to exit() the program here, where everything is freed, so nothing > > is leaked. > > I'm not sure I'd call it a sanitizer bug, rather its expected behaviour > of sanitizers. Even if you're about to exit, its important to see info > about all memory that is not freed by that time, since it can reveal > leaks that were ongoing in the process that are valid things to fix. > To make the sanitizers usable you need to get rid of the noise. IOW, > either have to provide a file to supress reports of memory that is > expected to remain allocated, or have to free it despite being about > to exit. Free'ing is the more maintainable strategy, as IME, supression > files get outdated over time. I think if there's still a live variable pointing to the unfreed memory at point of exit the compiler/sanitizer should be able to deduce that that's not a real leak. And if you believe that these really are leaks then you also need to be fixing them on the early exit paths, like the one where we exit(1) if xkb_keymap_new_from_names() fails. I don't object to this change, but I think that if the sanitizer complains about this kind of thing it's a bug, because it obscures real leaks. -- PMM
On 2024/05/22 23:36, Peter Maydell wrote: > On Wed, 22 May 2024 at 12:47, Daniel P. Berrangé <berrange@redhat.com> wrote: >> >> On Wed, May 22, 2024 at 12:35:23PM +0100, Peter Maydell wrote: >>> On Wed, 22 May 2024 at 11:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>> >>>> This fixes LeakSanitizer complaints with xkbcommon 1.6.0. >>>> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> --- >>>> qemu-keymap.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/qemu-keymap.c b/qemu-keymap.c >>>> index 8c80f7a4ed65..7a9f38cf9863 100644 >>>> --- a/qemu-keymap.c >>>> +++ b/qemu-keymap.c >>>> @@ -237,6 +237,9 @@ int main(int argc, char *argv[]) >>>> xkb_state_unref(state); >>>> state = NULL; >>>> >>>> + xkb_keymap_unref(map); >>>> + xkb_context_unref(ctx); >>>> + >>>> /* add quirks */ >>>> fprintf(outfile, >>>> "\n" >>> >>> This is surely a sanitizer bug. We're unconditionally about >>> to exit() the program here, where everything is freed, so nothing >>> is leaked. >> >> I'm not sure I'd call it a sanitizer bug, rather its expected behaviour >> of sanitizers. Even if you're about to exit, its important to see info >> about all memory that is not freed by that time, since it can reveal >> leaks that were ongoing in the process that are valid things to fix. >> To make the sanitizers usable you need to get rid of the noise. IOW, >> either have to provide a file to supress reports of memory that is >> expected to remain allocated, or have to free it despite being about >> to exit. Free'ing is the more maintainable strategy, as IME, supression >> files get outdated over time. > > I think if there's still a live variable pointing to the unfreed > memory at point of exit the compiler/sanitizer should be able to > deduce that that's not a real leak. And if you believe that these > really are leaks then you also need to be fixing them on the early > exit paths, like the one where we exit(1) if xkb_keymap_new_from_names() > fails. > > I don't object to this change, but I think that if the sanitizer > complains about this kind of thing it's a bug, because it obscures > real leaks. The sanitizer can certainly be improved to keep the automatic variables alive when there is exit(), but I'm a bit sympathetic with the sanitizer. Covering such a case requires the sanitizer to know that exit() terminates the process. Perhaps the sanitizer can look for __attribute__((noreturn)) and __builtin_unreachable(), but they may not be present and not reliable. I think it is a legitimate design decision not to try to deal with this kind of situation instead of partially handling it with attributes and builtin calls. Regards, Akihiko Odaki
22.05.2024 14:35, Peter Maydell wrote: ... > This is surely a sanitizer bug. We're unconditionally about > to exit() the program here, where everything is freed, so nothing > is leaked. https://lists.nongnu.org/archive/html/qemu-devel/2024-05/msg00658.html fwiw. /mjt -- GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24. New key: rsa4096/61AD3D98ECDF2C8E 9D8B E14E 3F2A 9DD7 9199 28F1 61AD 3D98 ECDF 2C8E Old key: rsa2048/457CE0A0804465C5 6EE1 95D1 886E 8FFB 810D 4324 457C E0A0 8044 65C5 Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt
© 2016 - 2024 Red Hat, Inc.