Re: [PATCH v3 1/3] qemu-keymap: Free xkb allocations

[Akihiko Odaki]

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