[Qemu-devel] [PATCH v5 0/5] Memory leak fixes

[Qemu-devel] [PATCH v5 0/5] Memory leak fixes

[Marc-André Lureau]

Hi,

A new series of leaks spotted by ASAN. Mostly after introducing of the
test-hmp. Would it be useful having a configure --enable-asan, and
enabled by default with --enable-debug?

Marc-André Lureau (5):
  test-keyval: fix leaks
  audio: fix capture buffer leaks
  audio: fix WAVState leak
  slirp: fix leak
  dump: fix memory_mapping_filter leak

 audio/audio.c       | 2 ++
 audio/wavcapture.c  | 1 +
 memory_mapping.c    | 1 +
 slirp/socket.c      | 3 +++
 tests/test-keyval.c | 4 ++++
 5 files changed, 11 insertions(+)

-- 
2.12.0.191.gc5d8de91d

[Qemu-devel] [PATCH v5 1/5] test-keyval: fix leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-keyval.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index ba19560a22..c556b1b117 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -628,6 +628,7 @@ static void test_keyval_visit_alternate(void)
     visit_type_AltNumStr(v, "a", &ans, &error_abort);
     g_assert_cmpint(ans->type, ==, QTYPE_QSTRING);
     g_assert_cmpstr(ans->u.s, ==, "1");
+    qapi_free_AltNumStr(ans);
     visit_type_AltNumInt(v, "a", &ani, &err);
     error_free_or_abort(&err);
     visit_end_struct(v, NULL);
@@ -651,9 +652,12 @@ static void test_keyval_visit_any(void)
     g_assert(qlist);
     qstr = qobject_to_qstring(qlist_pop(qlist));
     g_assert_cmpstr(qstring_get_str(qstr), ==, "null");
+    QDECREF(qstr);
     qstr = qobject_to_qstring(qlist_pop(qlist));
     g_assert_cmpstr(qstring_get_str(qstr), ==, "1");
     g_assert(qlist_empty(qlist));
+    QDECREF(qstr);
+    qobject_decref(any);
     visit_check_struct(v, &error_abort);
     visit_end_struct(v, NULL);
     visit_free(v);
-- 
2.12.0.191.gc5d8de91d

Re: [Qemu-devel] [PATCH v5 1/5] test-keyval: fix leaks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On 05/03/2017 05:38 PM, Marc-André Lureau wrote:
> Spotted by ASAN.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-keyval.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Eric blake <eblake@redhat.com>

> 
> diff --git a/tests/test-keyval.c b/tests/test-keyval.c
> index ba19560a22..c556b1b117 100644
> --- a/tests/test-keyval.c
> +++ b/tests/test-keyval.c
> @@ -628,6 +628,7 @@ static void test_keyval_visit_alternate(void)
>      visit_type_AltNumStr(v, "a", &ans, &error_abort);
>      g_assert_cmpint(ans->type, ==, QTYPE_QSTRING);
>      g_assert_cmpstr(ans->u.s, ==, "1");
> +    qapi_free_AltNumStr(ans);
>      visit_type_AltNumInt(v, "a", &ani, &err);
>      error_free_or_abort(&err);
>      visit_end_struct(v, NULL);
> @@ -651,9 +652,12 @@ static void test_keyval_visit_any(void)
>      g_assert(qlist);
>      qstr = qobject_to_qstring(qlist_pop(qlist));
>      g_assert_cmpstr(qstring_get_str(qstr), ==, "null");
> +    QDECREF(qstr);
>      qstr = qobject_to_qstring(qlist_pop(qlist));
>      g_assert_cmpstr(qstring_get_str(qstr), ==, "1");
>      g_assert(qlist_empty(qlist));
> +    QDECREF(qstr);
> +    qobject_decref(any);
>      visit_check_struct(v, &error_abort);
>      visit_end_struct(v, NULL);
>      visit_free(v);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v5 1/5] test-keyval: fix leaks

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 05/03/2017 05:38 PM, Marc-André Lureau wrote:
>> Spotted by ASAN.
>> 
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  tests/test-keyval.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>
> Reviewed-by: Eric blake <eblake@redhat.com>

This is in qapi-next already.  I'm adding the R-by now.

Re: [Qemu-devel] [PATCH v5 1/5] test-keyval: fix leaks

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 05/03/2017 05:38 PM, Marc-André Lureau wrote:
>>> Spotted by ASAN.
>>> 
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  tests/test-keyval.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>
>> Reviewed-by: Eric blake <eblake@redhat.com>
>
> This is in qapi-next already.  I'm adding the R-by now.

Actually, I'm replacing the commit in my tree by this one, because that
results in a simpler, cleaner commit message: I don't have to note the
tweaks I applied in my tree, and you in your v5.  Thanks!

[Qemu-devel] [PATCH v5 2/5] audio: fix capture buffer leaks

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 audio/audio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/audio/audio.c b/audio/audio.c
index c8898d8422..beafed209b 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -2028,6 +2028,8 @@ void AUD_del_capture (CaptureVoiceOut *cap, void *cb_opaque)
                     sw = sw1;
                 }
                 QLIST_REMOVE (cap, entries);
+                g_free (cap->hw.mix_buf);
+                g_free (cap->buf);
                 g_free (cap);
             }
             return;
-- 
2.12.0.191.gc5d8de91d

Re: [Qemu-devel] [PATCH v5 2/5] audio: fix capture buffer leaks

[Philippe Mathieu-Daudé]

Gerd already sent this one few days ago ;)

http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg05587.html

On 05/03/2017 07:38 PM, Marc-André Lureau wrote:
> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  audio/audio.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index c8898d8422..beafed209b 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -2028,6 +2028,8 @@ void AUD_del_capture (CaptureVoiceOut *cap, void *cb_opaque)
>                      sw = sw1;
>                  }
>                  QLIST_REMOVE (cap, entries);
> +                g_free (cap->hw.mix_buf);
> +                g_free (cap->buf);
>                  g_free (cap);
>              }
>              return;
>

[Qemu-devel] [PATCH v5 3/5] audio: fix WAVState leak

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 audio/wavcapture.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/audio/wavcapture.c b/audio/wavcapture.c
index 8bfb9e7654..5863803584 100644
--- a/audio/wavcapture.c
+++ b/audio/wavcapture.c
@@ -88,6 +88,7 @@ static void wav_capture_destroy (void *opaque)
     WAVState *wav = opaque;
 
     AUD_del_capture (wav->cap, wav);
+    g_free (wav);
 }
 
 static void wav_capture_info (void *opaque)
-- 
2.12.0.191.gc5d8de91d

Re: [Qemu-devel] [PATCH v5 3/5] audio: fix WAVState leak

[Philippe Mathieu-Daudé]

On 05/03/2017 07:38 PM, Marc-André Lureau wrote:
> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  audio/wavcapture.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/audio/wavcapture.c b/audio/wavcapture.c
> index 8bfb9e7654..5863803584 100644
> --- a/audio/wavcapture.c
> +++ b/audio/wavcapture.c
> @@ -88,6 +88,7 @@ static void wav_capture_destroy (void *opaque)
>      WAVState *wav = opaque;
>
>      AUD_del_capture (wav->cap, wav);
> +    g_free (wav);
>  }
>
>  static void wav_capture_info (void *opaque)
>

[Qemu-devel] [PATCH v5 4/5] slirp: fix leak

[Marc-André Lureau]

Spotted by ASAN:

/x86_64/hmp/pc-0.12:
=================================================================
==22538==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 224 byte(s) in 1 object(s) allocated from:
    #0 0x7f0f63cdee60 in malloc (/lib64/libasan.so.3+0xc6e60)
    #1 0x556f11ff32d7 in tcp_newtcpcb /home/elmarco/src/qemu/slirp/tcp_subr.c:250
    #2 0x556f11fdb1d1 in tcp_listen /home/elmarco/src/qemu/slirp/socket.c:688
    #3 0x556f11fca9d5 in slirp_add_hostfwd /home/elmarco/src/qemu/slirp/slirp.c:1052
    #4 0x556f11f8db41 in slirp_hostfwd /home/elmarco/src/qemu/net/slirp.c:506
    #5 0x556f11f8dd83 in hmp_hostfwd_add /home/elmarco/src/qemu/net/slirp.c:535

There might be a better way to fix this, but calling slirp tcp_close()
doesn't work.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 slirp/socket.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/slirp/socket.c b/slirp/socket.c
index 86927722e1..3b49a69a93 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -100,6 +100,9 @@ sofree(struct socket *so)
   if(so->so_next && so->so_prev)
     remque(so);  /* crashes if so is not in a queue */
 
+  if (so->so_tcpcb) {
+      free(so->so_tcpcb);
+  }
   free(so);
 }
 
-- 
2.12.0.191.gc5d8de91d

Re: [Qemu-devel] [PATCH v5 4/5] slirp: fix leak

[Samuel Thibault]

Applied to my tree, thanks!

Re: [Qemu-devel] [PATCH v5 4/5] slirp: fix leak

[Marc-André Lureau]

On Sat, May 6, 2017 at 3:56 AM Samuel Thibault <samuel.thibault@gnu.org>
wrote:

> Applied to my tree, thanks!
>
>
still not in tree, please remove the unnecessary 'if' before merging

thanks!
-- 
Marc-André Lureau

[Qemu-devel] [PATCH v5 5/5] dump: fix memory_mapping_filter leak

[Marc-André Lureau]

Spotted by ASAN.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 memory_mapping.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/memory_mapping.c b/memory_mapping.c
index 6a39d71da2..a5d38552a6 100644
--- a/memory_mapping.c
+++ b/memory_mapping.c
@@ -337,6 +337,7 @@ void memory_mapping_filter(MemoryMappingList *list, int64_t begin,
         if (cur->phys_addr >= begin + length ||
             cur->phys_addr + cur->length <= begin) {
             QTAILQ_REMOVE(&list->head, cur, next);
+            g_free(cur);
             list->num--;
             continue;
         }
-- 
2.12.0.191.gc5d8de91d

Re: [Qemu-devel] [PATCH v5 5/5] dump: fix memory_mapping_filter leak

[Marc-André Lureau]

Hi

On Thu, May 4, 2017 at 2:42 AM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> Spotted by ASAN.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  memory_mapping.c | 1 +
>

No maintainer for this file, Wen, Paolo?



>  1 file changed, 1 insertion(+)
>
> diff --git a/memory_mapping.c b/memory_mapping.c
> index 6a39d71da2..a5d38552a6 100644
> --- a/memory_mapping.c
> +++ b/memory_mapping.c
> @@ -337,6 +337,7 @@ void memory_mapping_filter(MemoryMappingList *list,
> int64_t begin,
>          if (cur->phys_addr >= begin + length ||
>              cur->phys_addr + cur->length <= begin) {
>              QTAILQ_REMOVE(&list->head, cur, next);
> +            g_free(cur);
>              list->num--;
>              continue;
>          }
> --
> 2.12.0.191.gc5d8de91d
>
>
> --
Marc-André Lureau

Re: [Qemu-devel] [PATCH v5 5/5] dump: fix memory_mapping_filter leak

[Marc-André Lureau]

On Fri, May 26, 2017 at 3:03 PM Marc-André Lureau <
marcandre.lureau@gmail.com> wrote:

> Hi
>
> On Thu, May 4, 2017 at 2:42 AM Marc-André Lureau <
> marcandre.lureau@redhat.com> wrote:
>
>> Spotted by ASAN.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  memory_mapping.c | 1 +
>>
>
> No maintainer for this file, Wen, Paolo?
>

CC qemu-trivial

thanks


>
>
>
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/memory_mapping.c b/memory_mapping.c
>> index 6a39d71da2..a5d38552a6 100644
>> --- a/memory_mapping.c
>> +++ b/memory_mapping.c
>> @@ -337,6 +337,7 @@ void memory_mapping_filter(MemoryMappingList *list,
>> int64_t begin,
>>          if (cur->phys_addr >= begin + length ||
>>              cur->phys_addr + cur->length <= begin) {
>>              QTAILQ_REMOVE(&list->head, cur, next);
>> +            g_free(cur);
>>              list->num--;
>>              continue;
>>          }
>> --
>> 2.12.0.191.gc5d8de91d
>>
>>
>> --
> Marc-André Lureau
>
-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v5 5/5] dump: fix memory_mapping_filter leak

[Michael Tokarev]

04.05.2017 01:38, Marc-André Lureau wrote:
> Spotted by ASAN.

Applied to -trivial. Thanks!

/mjt

Re: [Qemu-devel] [PATCH v5 0/5] Memory leak fixes

[Marc-André Lureau]

This is actually v1 (confused with git-publish)

----- Original Message -----
> Hi,
> 
> A new series of leaks spotted by ASAN. Mostly after introducing of the
> test-hmp. Would it be useful having a configure --enable-asan, and
> enabled by default with --enable-debug?
> 
> Marc-André Lureau (5):
>   test-keyval: fix leaks
>   audio: fix capture buffer leaks
>   audio: fix WAVState leak
>   slirp: fix leak
>   dump: fix memory_mapping_filter leak
> 
>  audio/audio.c       | 2 ++
>  audio/wavcapture.c  | 1 +
>  memory_mapping.c    | 1 +
>  slirp/socket.c      | 3 +++
>  tests/test-keyval.c | 4 ++++
>  5 files changed, 11 insertions(+)
> 
> --
> 2.12.0.191.gc5d8de91d
> 
> 

Re: [Qemu-devel] [PATCH v5 0/5] Memory leak fixes

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH v5 0/5] Memory leak fixes
Message-id: 20170503223846.6559-1-marcandre.lureau@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
84762e2 dump: fix memory_mapping_filter leak
345a5d8 slirp: fix leak
3f1ae28 audio: fix WAVState leak
565e071 audio: fix capture buffer leaks
2ce5984 test-keyval: fix leaks

=== OUTPUT BEGIN ===
Checking PATCH 1/5: test-keyval: fix leaks...
Checking PATCH 2/5: audio: fix capture buffer leaks...
ERROR: space prohibited between function name and open parenthesis '('
#22: FILE: audio/audio.c:2031:
+                g_free (cap->hw.mix_buf);

ERROR: space prohibited between function name and open parenthesis '('
#23: FILE: audio/audio.c:2032:
+                g_free (cap->buf);

total: 2 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/5: audio: fix WAVState leak...
ERROR: space prohibited between function name and open parenthesis '('
#22: FILE: audio/wavcapture.c:91:
+    g_free (wav);

total: 1 errors, 0 warnings, 7 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/5: slirp: fix leak...
ERROR: suspect code indent for conditional statements (2, 6)
#37: FILE: slirp/socket.c:103:
+  if (so->so_tcpcb) {
+      free(so->so_tcpcb);

total: 1 errors, 0 warnings, 9 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 5/5: dump: fix memory_mapping_filter leak...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v5 0/5] Memory leak fixes

[Alex Bennée]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi,
>
> A new series of leaks spotted by ASAN. Mostly after introducing of the
> test-hmp. Would it be useful having a configure --enable-asan, and

Yes.

> enabled by default with --enable-debug?

Not sure - isn't --enable-debug just for debug symbols and we enable
extra stuff like --enable-tcg-debug for additional checking? Given the
overhead of asan I still think we need a configure setup for reasonably
debugable but still performant.

>
> Marc-André Lureau (5):
>   test-keyval: fix leaks
>   audio: fix capture buffer leaks
>   audio: fix WAVState leak
>   slirp: fix leak
>   dump: fix memory_mapping_filter leak
>
>  audio/audio.c       | 2 ++
>  audio/wavcapture.c  | 1 +
>  memory_mapping.c    | 1 +
>  slirp/socket.c      | 3 +++
>  tests/test-keyval.c | 4 ++++
>  5 files changed, 11 insertions(+)


--
Alex Bennée

Re: [Qemu-devel] [PATCH v5 0/5] Memory leak fixes

[Paolo Bonzini]

On 04/05/2017 00:38, Marc-André Lureau wrote:
> Hi,
> 
> A new series of leaks spotted by ASAN. Mostly after introducing of the
> test-hmp. Would it be useful having a configure --enable-asan, and
> enabled by default with --enable-debug?

Yes, --enable-asan would be a nice idea, and we can also add a build
target for Docker tests.

Paolo