GNOME Bugzilla – Bug 736809
gnutls: Fix leaking GTask on TLS close after implicit handshake
Last modified: 2016-01-11 15:08:30 UTC
Fix a subtle leak where a leaked GTask will cause the entire GTlsConnectionGnutls to be leaked (due to the inherent reference loop between them) in the situation where: • TLS connection is created • Read/Write operation is started on the connection, and triggers an implicit handshake. • TLS connection is closed before the handshake is completed, and before any other operations are invoked on the connection. • implicit_handshake GTask is then leaked. There are a few other trivial cleanup patches here, just to squash some compiler warnings.
Created attachment 286391 [details] [review] gnutls: Reorder ‘static’ and ‘const’ attributes gcc prefers them this way round — the other way round is old-style.
Created attachment 286392 [details] [review] gnutls: Fix a signed/unsigned integer comparison
Created attachment 286393 [details] [review] gnutls: Add a missing enum value to a switch statement And a missing default case. This introduces no functional changes.
Created attachment 286394 [details] [review] gnutls: Ensure all private members of GTlsConnectionGnutls are freed Just to be on the safe side, ensure every private member of GTlsConnectionGnutls is freed when the object itself is finalised. implicit_handshake must always be NULL when finalize() is reached, as it holds a reference to the GTlsConnectionGnutls as its source object. However, it’s better to clear the member anyway, just in case this behaviour changes in future.
Created attachment 286395 [details] [review] gnutls: Fix leaking GTask on TLS close after implicit handshake If a GTlsConnectionGnutls is created and an implicit handshake operation is started on it (e.g. via a read), if the handshake doesn’t complete and close() is called on the connection without any other operations being called first, the GTlsConnectionGnutls.implicit_handshake GTask will be leaked. Since it holds a reference to the GTlsConnectionGnutls connection as a whole, that object will be leaked too in a reference cycle. Fix this by explicitly clearing implicit_handshake when claiming OP_CLOSE, instead of ignoring it. Any errors from the handshake process are ignored as before, however.
Comment on attachment 286391 [details] [review] gnutls: Reorder ‘static’ and ‘const’ attributes can't imagine why gcc cares, but ok
Comment on attachment 286394 [details] [review] gnutls: Ensure all private members of GTlsConnectionGnutls are freed >implicit_handshake must always be NULL when finalize() is reached, as it >holds a reference to the GTlsConnectionGnutls as its source object. >However, itâs better to clear the member anyway, just in case this >behaviour changes in future. Can you put that in a comment there too?
Comment on attachment 286395 [details] [review] gnutls: Fix leaking GTask on TLS close after implicit handshake I agree with your analysis of the bug, but for the fix, I think it would be clearer to leave the existing code as-is and just add something like: if (op == G_TLS_CONNECTION_GNUTLS_OP_CLOSE && gnutls->priv->implicit_handshake && priv->need_finish_handshake) { gnutls->priv->need_finish_handshake = FALSE; g_clear_object (&gnutls->priv->implicit_handshake); } It would also be great to add a test case for this.
Created attachment 286793 [details] [review] gnutls: Ensure all private members of GTlsConnectionGnutls are freed Just to be on the safe side, ensure every private member of GTlsConnectionGnutls is freed when the object itself is finalised. implicit_handshake must always be NULL when finalize() is reached, as it holds a reference to the GTlsConnectionGnutls as its source object. However, it’s better to clear the member anyway, just in case this behaviour changes in future.
Created attachment 286794 [details] [review] gnutls: Fix leaking GTask on TLS close after implicit handshake If a GTlsConnectionGnutls is created and an implicit handshake operation is started on it (e.g. via a read), if the handshake doesn’t complete and close() is called on the connection without any other operations being called first, the GTlsConnectionGnutls.implicit_handshake GTask will be leaked. Since it holds a reference to the GTlsConnectionGnutls connection as a whole, that object will be leaked too in a reference cycle. Fix this by explicitly clearing implicit_handshake when claiming OP_CLOSE, instead of ignoring it. Any errors from the handshake process are ignored as before, however. This updates one of the connection unit tests to check for the bug too.
First three commits pushed. commit f3b252c4981de66a887e4cec9ec4bfa96e651dbb Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Sep 17 13:06:30 2014 +0100 gnutls: Add a missing enum value to a switch statement And a missing default case. This introduces no functional changes. https://bugzilla.gnome.org/show_bug.cgi?id=736809 tls/gnutls/gtlsconnection-gnutls.c | 1 + tls/gnutls/gtlsserverconnection-gnutls.c | 1 + 2 files changed, 2 insertions(+) commit 082740f1ef4338b159a7b473e1d23f7c56224dd1 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Sep 17 13:06:15 2014 +0100 gnutls: Fix a signed/unsigned integer comparison https://bugzilla.gnome.org/show_bug.cgi?id=736809 tls/gnutls/gtlsfiledatabase-gnutls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) commit c3e65925c18fe4c3b616372c44f67c6716759353 Author: Philip Withnall <philip.withnall@collabora.co.uk> Date: Wed Sep 17 13:05:29 2014 +0100 gnutls: Reorder ‘static’ and ‘const’ attributes gcc prefers them this way round — the other way round is old-style. https://bugzilla.gnome.org/show_bug.cgi?id=736809 tls/gnutls/gtlsdatabase-gnutls-pkcs11.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Comment on attachment 286794 [details] [review] gnutls: Fix leaking GTask on TLS close after implicit handshake >@@ -1363,13 +1364,12 @@ test_async_implicit_handshake (TestConnection *test, gconstpointer data) > test, NULL); > > g_source_attach (input_source, NULL); >+ g_source_unref (input_source); > > g_main_loop_run (test->loop); > > g_io_stream_close (G_IO_STREAM (test->client_connection), NULL, &error); > g_assert_no_error (error); >- g_object_unref (test->client_connection); >- test->client_connection = NULL; > } how does this trigger that case? It looks like it just (a) unleaks input_source, and (b) leaks test->client_connection. To hit the new case, you'd have to do a non-blocking read, assert that it returned G_IO_ERROR_WOULD_BLOCK, and then close the connection (without ever running a main loop).
(In reply to comment #12) > how does this trigger that case? It looks like it just (a) unleaks > input_source, and (b) leaks test->client_connection. b) test->client_connection is freed in teardown_connection(), which additionally adds a weak pointer to it and blocks until that’s nullified, aborting after 10s of it not being nullified, thereby catching the case where the connection is leaked in another thread.
Comment on attachment 286793 [details] [review] gnutls: Ensure all private members of GTlsConnectionGnutls are freed Attachment 286793 [details] pushed as abd1a62 - gnutls: Ensure all private members of GTlsConnectionGnutls are freed
Pushed, and this bug can be marked as fixed, thanks! Attachment 286794 [details] pushed as 8609fff - gnutls: Fix leaking GTask on TLS close after implicit handshake