1. Patchew
  2. QEMU
  3. View series

[PATCH] io/channel-tls: plug memory leakage on GSource

Matheus Tavares Bernardino posted 1 patch 1 year, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/98f750b6ded2dd2e8d0e4ffc9935d3d6e0cd59f4.1678144392.git.quic._5Fmathbern@quicinc.com
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>
io/channel-tls.c | 1 +
1 file changed, 1 insertion(+)
[PATCH] io/channel-tls: plug memory leakage on GSource
Posted by Matheus Tavares Bernardino 1 year, 1 month ago 
This leakage can be seen through test-io-channel-tls:

$ ../configure --target-list=aarch64-softmmu --enable-sanitizers
$ make ./tests/unit/test-io-channel-tls
$ ./tests/unit/test-io-channel-tls

Indirect leak of 104 byte(s) in 1 object(s) allocated from:
    #0 0x7f81d1725808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
    #1 0x7f81d135ae98 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
    #2 0x55616c5d4c1b in object_new_with_propv ../qom/object.c:795
    #3 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768
    #4 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70
    #5 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158
    #6 0x7f81d137d58d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)

Indirect leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f81d1725a06 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
    #1 0x7f81d1472a20 in gnutls_dh_params_init (/lib/x86_64-linux-gnu/libgnutls.so.30+0x46a20)
    #2 0x55616c6485ff in qcrypto_tls_creds_x509_load ../crypto/tlscredsx509.c:634
    #3 0x55616c648ba2 in qcrypto_tls_creds_x509_complete ../crypto/tlscredsx509.c:694
    #4 0x55616c5e1fea in user_creatable_complete ../qom/object_interfaces.c:28
    #5 0x55616c5d4c8c in object_new_with_propv ../qom/object.c:807
    #6 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768
    #7 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70
    #8 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158
    #9 0x7f81d137d58d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)

...

SUMMARY: AddressSanitizer: 49143 byte(s) leaked in 184 allocation(s).

The docs for `g_source_add_child_source(source, child_source)` says
"source will hold a reference on child_source while child_source is
attached to it." Therefore, we should unreference the child source at
`qio_channel_tls_read_watch()` after attaching it to `source`. With this
change, ./tests/unit/test-io-channel-tls shows no leakages.

Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
---
 io/channel-tls.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 8052945ba0..5a7a3d48d6 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -446,6 +446,7 @@ qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source)
     object_ref(OBJECT(tioc));
 
     g_source_add_child_source(source, child);
+    g_source_unref(child);
 }
 
 static GSource *qio_channel_tls_create_watch(QIOChannel *ioc,
-- 
2.37.2
Re: [PATCH] io/channel-tls: plug memory leakage on GSource
Posted by Philippe Mathieu-Daudé 1 year, 1 month ago 
On 7/3/23 00:15, Matheus Tavares Bernardino wrote:
> This leakage can be seen through test-io-channel-tls:
> 
> $ ../configure --target-list=aarch64-softmmu --enable-sanitizers
> $ make ./tests/unit/test-io-channel-tls
> $ ./tests/unit/test-io-channel-tls
> 
> Indirect leak of 104 byte(s) in 1 object(s) allocated from:
>      #0 0x7f81d1725808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
>      #1 0x7f81d135ae98 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
>      #2 0x55616c5d4c1b in object_new_with_propv ../qom/object.c:795
>      #3 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768
>      #4 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70
>      #5 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158
>      #6 0x7f81d137d58d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
> 
> Indirect leak of 32 byte(s) in 1 object(s) allocated from:
>      #0 0x7f81d1725a06 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
>      #1 0x7f81d1472a20 in gnutls_dh_params_init (/lib/x86_64-linux-gnu/libgnutls.so.30+0x46a20)
>      #2 0x55616c6485ff in qcrypto_tls_creds_x509_load ../crypto/tlscredsx509.c:634
>      #3 0x55616c648ba2 in qcrypto_tls_creds_x509_complete ../crypto/tlscredsx509.c:694
>      #4 0x55616c5e1fea in user_creatable_complete ../qom/object_interfaces.c:28
>      #5 0x55616c5d4c8c in object_new_with_propv ../qom/object.c:807
>      #6 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768
>      #7 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70
>      #8 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158
>      #9 0x7f81d137d58d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
> 
> ...
> 
> SUMMARY: AddressSanitizer: 49143 byte(s) leaked in 184 allocation(s).
> 
> The docs for `g_source_add_child_source(source, child_source)` says
> "source will hold a reference on child_source while child_source is
> attached to it." Therefore, we should unreference the child source at
> `qio_channel_tls_read_watch()` after attaching it to `source`. With this
> change, ./tests/unit/test-io-channel-tls shows no leakages.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>   io/channel-tls.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 8052945ba0..5a7a3d48d6 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -446,6 +446,7 @@ qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source)
>       object_ref(OBJECT(tioc));
>   
>       g_source_add_child_source(source, child);
> +    g_source_unref(child);

Or declare child with 'g_autoptr(GSource)'. The difference is
only a matter of style, so I'll let Daniel to decide what is
preferred. Regardless:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   }
Re: [PATCH] io/channel-tls: plug memory leakage on GSource
Posted by Daniel P. Berrangé 1 year, 1 month ago 
On Mon, Mar 06, 2023 at 08:15:21PM -0300, Matheus Tavares Bernardino wrote:
> This leakage can be seen through test-io-channel-tls:
> 
> $ ../configure --target-list=aarch64-softmmu --enable-sanitizers
> $ make ./tests/unit/test-io-channel-tls
> $ ./tests/unit/test-io-channel-tls
> 
> Indirect leak of 104 byte(s) in 1 object(s) allocated from:
>     #0 0x7f81d1725808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
>     #1 0x7f81d135ae98 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
>     #2 0x55616c5d4c1b in object_new_with_propv ../qom/object.c:795
>     #3 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768
>     #4 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70
>     #5 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158
>     #6 0x7f81d137d58d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
> 
> Indirect leak of 32 byte(s) in 1 object(s) allocated from:
>     #0 0x7f81d1725a06 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
>     #1 0x7f81d1472a20 in gnutls_dh_params_init (/lib/x86_64-linux-gnu/libgnutls.so.30+0x46a20)
>     #2 0x55616c6485ff in qcrypto_tls_creds_x509_load ../crypto/tlscredsx509.c:634
>     #3 0x55616c648ba2 in qcrypto_tls_creds_x509_complete ../crypto/tlscredsx509.c:694
>     #4 0x55616c5e1fea in user_creatable_complete ../qom/object_interfaces.c:28
>     #5 0x55616c5d4c8c in object_new_with_propv ../qom/object.c:807
>     #6 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768
>     #7 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70
>     #8 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158
>     #9 0x7f81d137d58d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
> 
> ...
> 
> SUMMARY: AddressSanitizer: 49143 byte(s) leaked in 184 allocation(s).
> 
> The docs for `g_source_add_child_source(source, child_source)` says
> "source will hold a reference on child_source while child_source is
> attached to it." Therefore, we should unreference the child source at
> `qio_channel_tls_read_watch()` after attaching it to `source`. With this
> change, ./tests/unit/test-io-channel-tls shows no leakages.

Yes, the other places in QEMU that call g_source_add_child_source also
unref the source after adding it.

> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>
> ---
>  io/channel-tls.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

and queued.

> 
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 8052945ba0..5a7a3d48d6 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -446,6 +446,7 @@ qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source)
>      object_ref(OBJECT(tioc));
>  
>      g_source_add_child_source(source, child);
> +    g_source_unref(child);
>  }
>  
>  static GSource *qio_channel_tls_create_watch(QIOChannel *ioc,
> -- 
> 2.37.2
> 

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 :|
Re: [PATCH] io/channel-tls: plug memory leakage on GSource
Posted by Antoine Damhet 1 year, 1 month ago 
Nice catch !

On Mon, Mar 06, 2023 at 08:15:21PM -0300, Matheus Tavares Bernardino wrote:
> This leakage can be seen through test-io-channel-tls:
> 
> $ ../configure --target-list=aarch64-softmmu --enable-sanitizers
> $ make ./tests/unit/test-io-channel-tls
> $ ./tests/unit/test-io-channel-tls
> 
> Indirect leak of 104 byte(s) in 1 object(s) allocated from:
>     #0 0x7f81d1725808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
>     #1 0x7f81d135ae98 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
>     #2 0x55616c5d4c1b in object_new_with_propv ../qom/object.c:795
>     #3 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768
>     #4 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70
>     #5 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158
>     #6 0x7f81d137d58d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
> 
> Indirect leak of 32 byte(s) in 1 object(s) allocated from:
>     #0 0x7f81d1725a06 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
>     #1 0x7f81d1472a20 in gnutls_dh_params_init (/lib/x86_64-linux-gnu/libgnutls.so.30+0x46a20)
>     #2 0x55616c6485ff in qcrypto_tls_creds_x509_load ../crypto/tlscredsx509.c:634
>     #3 0x55616c648ba2 in qcrypto_tls_creds_x509_complete ../crypto/tlscredsx509.c:694
>     #4 0x55616c5e1fea in user_creatable_complete ../qom/object_interfaces.c:28
>     #5 0x55616c5d4c8c in object_new_with_propv ../qom/object.c:807
>     #6 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768
>     #7 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70
>     #8 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158
>     #9 0x7f81d137d58d  (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
> 
> ...
> 
> SUMMARY: AddressSanitizer: 49143 byte(s) leaked in 184 allocation(s).
> 
> The docs for `g_source_add_child_source(source, child_source)` says
> "source will hold a reference on child_source while child_source is
> attached to it." Therefore, we should unreference the child source at
> `qio_channel_tls_read_watch()` after attaching it to `source`. With this
> change, ./tests/unit/test-io-channel-tls shows no leakages.
> 
> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com>

Reviewed-by: Antoine Damhet <antoine.damhet@shadow.tech>

> ---
>  io/channel-tls.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 8052945ba0..5a7a3d48d6 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -446,6 +446,7 @@ qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source)
>      object_ref(OBJECT(tioc));
>  
>      g_source_add_child_source(source, child);
> +    g_source_unref(child);
>  }
>  
>  static GSource *qio_channel_tls_create_watch(QIOChannel *ioc,
> -- 
> 2.37.2
> 

-- 
Antoine 'xdbob' Damhet

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.