| Message ID | 98f750b6ded2dd2e8d0e4ffc9935d3d6e0cd59f4.1678144392.git.quic_mathbern@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io/channel-tls: plug memory leakage on GSource | expand |
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 >
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
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> > }
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,
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(+)