diff mbox series

[PULL,17/51] readline: add a free function

Message ID 1516112253-14480-18-git-send-email-pbonzini@redhat.com
State New
Headers show
Series [PULL,01/51] scsi-generic: Add share-rw option | expand

Commit Message

Paolo Bonzini Jan. 16, 2018, 2:16 p.m. UTC
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Fixes leaks such as:

Direct leak of 2 byte(s) in 1 object(s) allocated from:
    #0 0x7eff58beb850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7eff57942f0c in g_malloc ../glib/gmem.c:94
    #2 0x7eff579431cf in g_malloc_n ../glib/gmem.c:331
    #3 0x7eff5795f6eb in g_strdup ../glib/gstrfuncs.c:363
    #4 0x55db720f1d46 in readline_hist_add /home/elmarco/src/qq/util/readline.c:258
    #5 0x55db720f2d34 in readline_handle_byte /home/elmarco/src/qq/util/readline.c:387
    #6 0x55db71539d00 in monitor_read /home/elmarco/src/qq/monitor.c:3896
    #7 0x55db71f9be35 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167
    #8 0x55db71f9bed3 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179
    #9 0x55db71fa013c in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
    #10 0x55db71fe18a8 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
    #11 0x7eff5793a90b in g_main_dispatch ../glib/gmain.c:3182
    #12 0x7eff5793b7ac in g_main_context_dispatch ../glib/gmain.c:3847
    #13 0x55db720af3bd in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214
    #14 0x55db720af505 in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261
    #15 0x55db720af6d6 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515
    #16 0x55db7184e0de in main_loop /home/elmarco/src/qq/vl.c:1995
    #17 0x55db7185e956 in main /home/elmarco/src/qq/vl.c:4914
    #18 0x7eff4ea17039 in __libc_start_main (/lib64/libc.so.6+0x21039)

(while at it, use g_new0(ReadLineState), it's a bit easier to read)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20180104160523.22995-11-marcandre.lureau@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/readline.h |  1 +
 monitor.c               |  2 +-
 util/readline.c         | 18 +++++++++++++++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

Comments

Alex Williamson Feb. 2, 2018, midnight UTC | #1
On Tue, 16 Jan 2018 15:16:59 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Fixes leaks such as:
> 
> Direct leak of 2 byte(s) in 1 object(s) allocated from:
>     #0 0x7eff58beb850 in malloc (/lib64/libasan.so.4+0xde850)
>     #1 0x7eff57942f0c in g_malloc ../glib/gmem.c:94
>     #2 0x7eff579431cf in g_malloc_n ../glib/gmem.c:331
>     #3 0x7eff5795f6eb in g_strdup ../glib/gstrfuncs.c:363
>     #4 0x55db720f1d46 in readline_hist_add /home/elmarco/src/qq/util/readline.c:258
>     #5 0x55db720f2d34 in readline_handle_byte /home/elmarco/src/qq/util/readline.c:387
>     #6 0x55db71539d00 in monitor_read /home/elmarco/src/qq/monitor.c:3896
>     #7 0x55db71f9be35 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167
>     #8 0x55db71f9bed3 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179
>     #9 0x55db71fa013c in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
>     #10 0x55db71fe18a8 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
>     #11 0x7eff5793a90b in g_main_dispatch ../glib/gmain.c:3182
>     #12 0x7eff5793b7ac in g_main_context_dispatch ../glib/gmain.c:3847
>     #13 0x55db720af3bd in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214
>     #14 0x55db720af505 in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261
>     #15 0x55db720af6d6 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515
>     #16 0x55db7184e0de in main_loop /home/elmarco/src/qq/vl.c:1995
>     #17 0x55db7185e956 in main /home/elmarco/src/qq/vl.c:4914
>     #18 0x7eff4ea17039 in __libc_start_main (/lib64/libc.so.6+0x21039)
> 
> (while at it, use g_new0(ReadLineState), it's a bit easier to read)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Message-Id: <20180104160523.22995-11-marcandre.lureau@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---

I'm having some trouble with this patch, using b05631954d6d:

# /usr/local/bin/qemu-system-x86_64 -m 1G -nodefaults -net none -monitor stdio -serial none -parallel none -nographic 
QEMU 2.11.50 monitor - type 'help' for more information
(qemu) sys<tab>
system_powerdown  system_reset      system_wakeup     
(qemu) system_p<tab>    # resulting in system_powerdown
(qemu) quit
Segmentation fault (core dumped)

gdb shows:

Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
0x00007f7d64d82927 in malloc () from /lib64/libc.so.6
(gdb) bt
#0  0x00007f7d64d82927 in malloc () at /lib64/libc.so.6
#1  0x00007f7d6ef68359 in g_malloc () at /lib64/libglib-2.0.so.0
#2  0x00007f7d6ef83004 in g_strsplit () at /lib64/libglib-2.0.so.0
#3  0x000055e5ac0d549d in container_get (root=0x55e5ad570ee0, path=path@entry=0x55e5ac2fa0f8 "/chardevs") at qom/container.c:34
#4  0x000055e5ac14d102 in get_chardevs_root () at chardev/char.c:43
#5  0x000055e5ac14ec4d in qemu_chr_cleanup () at chardev/char.c:1107
#6  0x000055e5abeff1c4 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4780

Reverting this patch, commit e5dc1a6c6c435, I don't see the issue.
Thanks,

Alex
Paolo Bonzini Feb. 2, 2018, 12:10 a.m. UTC | #2
On 01/02/2018 19:00, Alex Williamson wrote:
> On Tue, 16 Jan 2018 15:16:59 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Fixes leaks such as:
>>
>> Direct leak of 2 byte(s) in 1 object(s) allocated from:
>>     #0 0x7eff58beb850 in malloc (/lib64/libasan.so.4+0xde850)
>>     #1 0x7eff57942f0c in g_malloc ../glib/gmem.c:94
>>     #2 0x7eff579431cf in g_malloc_n ../glib/gmem.c:331
>>     #3 0x7eff5795f6eb in g_strdup ../glib/gstrfuncs.c:363
>>     #4 0x55db720f1d46 in readline_hist_add /home/elmarco/src/qq/util/readline.c:258
>>     #5 0x55db720f2d34 in readline_handle_byte /home/elmarco/src/qq/util/readline.c:387
>>     #6 0x55db71539d00 in monitor_read /home/elmarco/src/qq/monitor.c:3896
>>     #7 0x55db71f9be35 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167
>>     #8 0x55db71f9bed3 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179
>>     #9 0x55db71fa013c in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
>>     #10 0x55db71fe18a8 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
>>     #11 0x7eff5793a90b in g_main_dispatch ../glib/gmain.c:3182
>>     #12 0x7eff5793b7ac in g_main_context_dispatch ../glib/gmain.c:3847
>>     #13 0x55db720af3bd in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214
>>     #14 0x55db720af505 in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261
>>     #15 0x55db720af6d6 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515
>>     #16 0x55db7184e0de in main_loop /home/elmarco/src/qq/vl.c:1995
>>     #17 0x55db7185e956 in main /home/elmarco/src/qq/vl.c:4914
>>     #18 0x7eff4ea17039 in __libc_start_main (/lib64/libc.so.6+0x21039)
>>
>> (while at it, use g_new0(ReadLineState), it's a bit easier to read)
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Message-Id: <20180104160523.22995-11-marcandre.lureau@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
> 
> I'm having some trouble with this patch, using b05631954d6d:
> 
> # /usr/local/bin/qemu-system-x86_64 -m 1G -nodefaults -net none -monitor stdio -serial none -parallel none -nographic 
> QEMU 2.11.50 monitor - type 'help' for more information
> (qemu) sys<tab>
> system_powerdown  system_reset      system_wakeup     
> (qemu) system_p<tab>    # resulting in system_powerdown
> (qemu) quit
> Segmentation fault (core dumped)
> 
> gdb shows:
> 
> Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> 0x00007f7d64d82927 in malloc () from /lib64/libc.so.6
> (gdb) bt
> #0  0x00007f7d64d82927 in malloc () at /lib64/libc.so.6
> #1  0x00007f7d6ef68359 in g_malloc () at /lib64/libglib-2.0.so.0
> #2  0x00007f7d6ef83004 in g_strsplit () at /lib64/libglib-2.0.so.0
> #3  0x000055e5ac0d549d in container_get (root=0x55e5ad570ee0, path=path@entry=0x55e5ac2fa0f8 "/chardevs") at qom/container.c:34
> #4  0x000055e5ac14d102 in get_chardevs_root () at chardev/char.c:43
> #5  0x000055e5ac14ec4d in qemu_chr_cleanup () at chardev/char.c:1107
> #6  0x000055e5abeff1c4 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4780
> 
> Reverting this patch, commit e5dc1a6c6c435, I don't see the issue.
> Thanks,

Yeah, I have a fix queued.  Unfortunately, I don't have the usual setup
to do pre-pull-request sets here so it will have to wait for next Monday.

Paolo
Greg Kurz Feb. 2, 2018, 9:05 a.m. UTC | #3
On Thu, 1 Feb 2018 19:10:06 -0500
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 01/02/2018 19:00, Alex Williamson wrote:
> > On Tue, 16 Jan 2018 15:16:59 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >   
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> Fixes leaks such as:
> >>
> >> Direct leak of 2 byte(s) in 1 object(s) allocated from:
> >>     #0 0x7eff58beb850 in malloc (/lib64/libasan.so.4+0xde850)
> >>     #1 0x7eff57942f0c in g_malloc ../glib/gmem.c:94
> >>     #2 0x7eff579431cf in g_malloc_n ../glib/gmem.c:331
> >>     #3 0x7eff5795f6eb in g_strdup ../glib/gstrfuncs.c:363
> >>     #4 0x55db720f1d46 in readline_hist_add /home/elmarco/src/qq/util/readline.c:258
> >>     #5 0x55db720f2d34 in readline_handle_byte /home/elmarco/src/qq/util/readline.c:387
> >>     #6 0x55db71539d00 in monitor_read /home/elmarco/src/qq/monitor.c:3896
> >>     #7 0x55db71f9be35 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167
> >>     #8 0x55db71f9bed3 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179
> >>     #9 0x55db71fa013c in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
> >>     #10 0x55db71fe18a8 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
> >>     #11 0x7eff5793a90b in g_main_dispatch ../glib/gmain.c:3182
> >>     #12 0x7eff5793b7ac in g_main_context_dispatch ../glib/gmain.c:3847
> >>     #13 0x55db720af3bd in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214
> >>     #14 0x55db720af505 in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261
> >>     #15 0x55db720af6d6 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515
> >>     #16 0x55db7184e0de in main_loop /home/elmarco/src/qq/vl.c:1995
> >>     #17 0x55db7185e956 in main /home/elmarco/src/qq/vl.c:4914
> >>     #18 0x7eff4ea17039 in __libc_start_main (/lib64/libc.so.6+0x21039)
> >>
> >> (while at it, use g_new0(ReadLineState), it's a bit easier to read)
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> Message-Id: <20180104160523.22995-11-marcandre.lureau@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---  
> > 
> > I'm having some trouble with this patch, using b05631954d6d:
> > 
> > # /usr/local/bin/qemu-system-x86_64 -m 1G -nodefaults -net none -monitor stdio -serial none -parallel none -nographic 
> > QEMU 2.11.50 monitor - type 'help' for more information
> > (qemu) sys<tab>
> > system_powerdown  system_reset      system_wakeup     
> > (qemu) system_p<tab>    # resulting in system_powerdown
> > (qemu) quit
> > Segmentation fault (core dumped)
> > 
> > gdb shows:
> > 
> > Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
> > 0x00007f7d64d82927 in malloc () from /lib64/libc.so.6
> > (gdb) bt
> > #0  0x00007f7d64d82927 in malloc () at /lib64/libc.so.6
> > #1  0x00007f7d6ef68359 in g_malloc () at /lib64/libglib-2.0.so.0
> > #2  0x00007f7d6ef83004 in g_strsplit () at /lib64/libglib-2.0.so.0
> > #3  0x000055e5ac0d549d in container_get (root=0x55e5ad570ee0, path=path@entry=0x55e5ac2fa0f8 "/chardevs") at qom/container.c:34
> > #4  0x000055e5ac14d102 in get_chardevs_root () at chardev/char.c:43
> > #5  0x000055e5ac14ec4d in qemu_chr_cleanup () at chardev/char.c:1107
> > #6  0x000055e5abeff1c4 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at vl.c:4780
> > 
> > Reverting this patch, commit e5dc1a6c6c435, I don't see the issue.
> > Thanks,  
> 
> Yeah, I have a fix queued.  Unfortunately, I don't have the usual setup
> to do pre-pull-request sets here so it will have to wait for next Monday.
> 
> Paolo
> 

The queued fix is:

http://patchwork.ozlabs.org/patch/862816/
diff mbox series

Patch

diff --git a/include/qemu/readline.h b/include/qemu/readline.h
index c08cf74..e812583 100644
--- a/include/qemu/readline.h
+++ b/include/qemu/readline.h
@@ -59,5 +59,6 @@  ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
                              ReadLineFlushFunc *flush_func,
                              void *opaque,
                              ReadLineCompletionFunc *completion_finder);
+void readline_free(ReadLineState *rs);
 
 #endif /* READLINE_H */
diff --git a/monitor.c b/monitor.c
index d682eee..b9da5e2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -583,7 +583,7 @@  static void monitor_data_destroy(Monitor *mon)
     if (monitor_is_qmp(mon)) {
         json_message_parser_destroy(&mon->qmp.parser);
     }
-    g_free(mon->rs);
+    readline_free(mon->rs);
     QDECREF(mon->outbuf);
     qemu_mutex_destroy(&mon->out_lock);
 }
diff --git a/util/readline.c b/util/readline.c
index bbdee79..24ec839 100644
--- a/util/readline.c
+++ b/util/readline.c
@@ -500,12 +500,28 @@  const char *readline_get_history(ReadLineState *rs, unsigned int index)
     return rs->history[index];
 }
 
+void readline_free(ReadLineState *rs)
+{
+    int i;
+
+    if (!rs) {
+        return;
+    }
+    for (i = 0; i < READLINE_MAX_CMDS; i++) {
+        g_free(rs->history[i]);
+    }
+    for (i = 0; i < READLINE_MAX_COMPLETIONS; i++) {
+        g_free(rs->completions[i]);
+    }
+    g_free(rs);
+}
+
 ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
                              ReadLineFlushFunc *flush_func,
                              void *opaque,
                              ReadLineCompletionFunc *completion_finder)
 {
-    ReadLineState *rs = g_malloc0(sizeof(*rs));
+    ReadLineState *rs = g_new0(ReadLineState, 1);
 
     rs->hist_entry = -1;
     rs->opaque = opaque;