After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 689271 - gnutls: Don't hang trying to handshake, if early close
gnutls: Don't hang trying to handshake, if early close
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-11-29 12:53 UTC by Stef Walter
Modified: 2012-12-21 14:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gnutls: Don't hang trying to handshake, if early close (2.48 KB, patch)
2012-11-29 12:53 UTC, Stef Walter
none Details | Review
gnutls: Don't hang trying to handshake, if early close (2.51 KB, patch)
2012-11-29 13:25 UTC, Stef Walter
reviewed Details | Review
gnutls: Don't hang trying to handshake, if early close (2.49 KB, patch)
2012-11-29 22:08 UTC, Stef Walter
committed Details | Review

Description Stef Walter 2012-11-29 12:53:35 UTC
When trying to close a GTlsConnectionGnutls early before handshakeing,
the code assumed 'need_handshake' and would do an implicit handshake
before closing.

This could lead to hangs on closing a connection, if the server isn't
responsive, and is generally useless.
Comment 1 Stef Walter 2012-11-29 12:53:38 UTC
Created attachment 230179 [details] [review]
gnutls: Don't hang trying to handshake, if early close

When closing a connection before the handshake, the GTlsConnectionGnutls
code would try to do a handshake. Fix this, and test for it.
Comment 2 Stef Walter 2012-11-29 13:25:15 UTC
Created attachment 230184 [details] [review]
gnutls: Don't hang trying to handshake, if early close

Minor fix, don't use the echo test server.
Comment 3 Dan Winship 2012-11-29 15:31:53 UTC
Comment on attachment 230184 [details] [review]
gnutls: Don't hang trying to handshake, if early close

>+  /* If closing, then no need to complete a handshake */
>+  gnutls->priv->need_handshake = FALSE;
>+
>   if (!claim_op (gnutls, G_TLS_CONNECTION_GNUTLS_OP_CLOSE,
> 		 TRUE, cancellable, error))

Hm... should that go after the claim_op?
Comment 4 Stef Walter 2012-11-29 15:41:19 UTC
claim_op() is the one that acts on need_handshake. 

But if you want to solve another way, you could make claim_op not respect need_handshake if G_TLS_CONNECTION_GNUTLS_OP_CLOSE. Up to you.
Comment 5 Dan Winship 2012-11-29 17:11:27 UTC
(In reply to comment #4)
> claim_op() is the one that acts on need_handshake. 

oh, right

> But if you want to solve another way, you could make claim_op not respect
> need_handshake if G_TLS_CONNECTION_GNUTLS_OP_CLOSE. Up to you.

Yeah, there's a race condition in your patch, since another thread might try to start an op in between when you set need_handshake FALSE and when you claim OP_CLOSE.
Comment 6 Stef Walter 2012-11-29 22:08:36 UTC
Created attachment 230242 [details] [review]
gnutls: Don't hang trying to handshake, if early close

When closing a connection before the handshake, the GTlsConnectionGnutls
code would try to do a handshake. Fix this, and test for it.
Comment 7 Stef Walter 2012-11-29 22:30:49 UTC
Attachment 230242 [details] pushed as 58fd3d5 - gnutls: Don't hang trying to handshake, if early close
Comment 8 Debarshi Ray 2012-12-05 17:01:04 UTC
It is still hanging for me with the following backtrace:

  • #0 poll
    at ../sysdeps/unix/syscall-template.S line 81
  • #1 poll
    at /usr/include/bits/poll2.h line 46
  • #2 g_poll
    at gpoll.c line 132
  • #3 claim_op
    at gtlsconnection-gnutls.c line 583
  • #4 g_tls_connection_gnutls_close
    at gtlsconnection-gnutls.c line 1449
  • #5 g_io_stream_close
    at giostream.c line 414
  • #6 disconnect_internal
    at soup-socket.c line 116
  • #7 soup_socket_disconnect
    at soup-socket.c line 1226
  • #8 soup_connection_disconnect
    at soup-connection.c line 831
  • #9 soup_session_abort
    at soup-session.c line 1677
  • #10 _g_closure_invoke_va
    at gclosure.c line 840
  • #11 g_signal_emit_valist
    at gsignal.c line 3226
  • #12 g_signal_emit
    at gsignal.c line 3371
  • #13 g_cancellable_cancel
    at gcancellable.c line 507

Here is a test case: http://rishi.fedorapeople.org/gtls-cancel.tar.gz
Hangs on every 2nd attempt on my system.
Comment 9 Stef Walter 2012-12-21 13:55:12 UTC
I can't duplicate this on my system unfortunately. I guess it's hard to reproduce because it involves a remote server. I did very that cancel_cb() is actually getting called. 

Dan, is soup_session_abort() supposed to try to flush data when closing?
Comment 10 Dan Winship 2012-12-21 14:24:52 UTC
oh, that was a separate bug, which got fixed as bug 688751. Sorry, I didn't notice that it had been mentioned here too