On Wed, Jul 15, 2015 at 08:15:55PM -0700, Kevin J. McCarthy wrote: > Kevin J. McCarthy wrote: > > Kevin J. McCarthy wrote: > > > I wasn't really happy having a loop counter option for GnuTLS. It turns > > > out the GNUTLS_E_AGAIN is translated directly from EAGAIN/EWOULDBLOCK. > > > Since we're not using non-blocking io, this will only be triggered by > > > our timeout. > > > > Sorry, I was wrong. It will return GNUTLS_E_AGAIN for situations other > > than the timeout we set, so my patch is incorrect. I'll have to think > > about this some more. > > After looking some more, I've re-convinced myself the patch approach is > actually okay. > > It looks like GnuTLS will return the GNUTLS_E_AGAIN for a partial > read only if recv returns a EAGAIN or EINTR. Otherwise it will keep > looping until it gets the needed amount. (see _gnutls_stream_read() and > _gnutls_io_read_buffered()) > > The EAGAIN should only occur for a timeout on no data, and the EINTR > should only occur for a signal. Since the timeout is ours and we aren't > setting signals, there is no more reason to loop or handle these errors > than there is with raw_socket_read or raw_socket_write. Therefore I > believe patch should be okay after all. > > I am attaching a somewhat more aggressive patch that removes the looping > from tls_socket_read, and returns tls_socket_write to its behavior of > aborting on any error. > > Feedback and testing is definitely appreciated!
Looks like nice work, Kevin. Cool that this will hopefully get crossed off
the list. But personally I can’t give any feedback or test it until next
week at the earliest.
Grts!
Allen
> WHERE short SendmailWait;
> WHERE short SleepTime INITVAL (1);
> +WHERE short SocketTimeout;
> WHERE short TimeInc;
> WHERE short Timeout;
> WHERE short Wrap;
> WHERE short WrapHeaders;
> WHERE short WriteInc;
>
> WHERE short ScoreThresholdDelete;
> WHERE short ScoreThresholdRead;
> diff --git a/init.h b/init.h
> --- a/init.h
> +++ b/init.h
> @@ -2883,16 +2883,22 @@
> ** smtp[s]://[user[:pass]@]host[:port]
> ** .te
> ** .pp
> ** where ``[...]'' denotes an optional part.
> ** Setting this variable overrides the value of the $$sendmail
> ** variable.
> */
> #endif /* USE_SMTP */
> + { "socket_timeout", DT_NUM, R_NONE, UL &SocketTimeout, 30 },
> + /*
> + ** .pp
> + ** Set socket send and receive timeouts to this many seconds. When
> + ** set to zero, send and receive operations will not time out.
> + */
> { "sort", DT_SORT, R_INDEX|R_RESORT, UL &Sort, SORT_DATE },
> /*
> ** .pp
> ** Specifies how to sort messages in the ``index'' menu. Valid values
> ** are:
> ** .il
> ** .dd date or date-sent
> ** .dd date-received
> diff --git a/mutt_socket.c b/mutt_socket.c
> --- a/mutt_socket.c
> +++ b/mutt_socket.c
> @@ -339,16 +339,17 @@
> return 0;
> }
>
> /* socket_connect: set up to connect to a socket fd. */
> static int socket_connect (int fd, struct sockaddr* sa)
> {
> int sa_size;
> int save_errno;
> + struct timeval socket_timeout_tv = { 0, 0 };
>
> if (sa->sa_family == AF_INET)
> sa_size = sizeof (struct sockaddr_in);
> #ifdef HAVE_GETADDRINFO
> else if (sa->sa_family == AF_INET6)
> sa_size = sizeof (struct sockaddr_in6);
> #endif
> else
> @@ -359,16 +360,35 @@
>
> if (ConnectTimeout > 0)
> alarm (ConnectTimeout);
>
> mutt_allow_interrupt (1);
>
> save_errno = 0;
>
> + if (SocketTimeout > 0)
> + {
> + socket_timeout_tv.tv_sec = SocketTimeout;
> + if (setsockopt (fd, SOL_SOCKET, SO_RCVTIMEO, &socket_timeout_tv,
> + sizeof(struct timeval)))
> + {
> + mutt_perror ("setsockopt");
> + mutt_sleep(3);
> + return -1;
> + }
> + if (setsockopt (fd, SOL_SOCKET, SO_SNDTIMEO, &socket_timeout_tv,
> + sizeof(struct timeval)))
> + {
> + mutt_perror ("setsockopt");
> + mutt_sleep(3);
> + return -1;
> + }
> + }
> +
> if (connect (fd, sa, sa_size) < 0)
> {
> save_errno = errno;
> dprint (2, (debugfile, "Connection failed. errno: %d...\n", errno));
> SigInt = 0; /* reset in case we caught SIGINTR while in connect() */
> }
>
> if (ConnectTimeout > 0)
> diff --git a/mutt_ssl_gnutls.c b/mutt_ssl_gnutls.c
> --- a/mutt_ssl_gnutls.c
> +++ b/mutt_ssl_gnutls.c
> @@ -134,26 +134,23 @@
>
> if (!data)
> {
> mutt_error (_("Error: no TLS socket open"));
> mutt_sleep (2);
> return -1;
> }
>
> - do {
> - ret = gnutls_record_recv (data->state, buf, len);
> - if (ret < 0 && gnutls_error_is_fatal(ret) == 1)
> - {
> - mutt_error ("tls_socket_read (%s)", gnutls_strerror (ret));
> - mutt_sleep (4);
> - return -1;
> - }
> + ret = gnutls_record_recv (data->state, buf, len);
> + if (ret < 0)
> + {
> + mutt_error ("tls_socket_read (%s)", gnutls_strerror (ret));
> + mutt_sleep (4);
> + return -1;
> }
> - while (ret == GNUTLS_E_AGAIN || ret == GNUTLS_E_INTERRUPTED);
>
> return ret;
> }
>
> static int tls_socket_write (CONNECTION* conn, const char* buf, size_t len)
> {
> tlssockdata *data = conn->sockdata;
> int ret;
> @@ -166,25 +163,22 @@
> return -1;
> }
>
> do
> {
> ret = gnutls_record_send (data->state, buf + sent, len - sent);
> if (ret < 0)
> {
> - if (gnutls_error_is_fatal(ret) == 1)
> - {
> - mutt_error ("tls_socket_write (%s)", gnutls_strerror (ret));
> - mutt_sleep (4);
> - return -1;
> - }
> - return ret;
> + mutt_error ("tls_socket_write (%s)", gnutls_strerror (ret));
> + mutt_sleep (4);
> + return -1;
> }
> - sent += ret;
> + else
> + sent += ret;
> } while (sent < len);
>
> return sent;
> }
>
> static int tls_socket_open (CONNECTION* conn)
> {
> if (raw_socket_open (conn) < 0)
signature.asc
Description: PGP signature
