tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rainer Jung <rainer.j...@kippdata.de>
Subject Re: SSL errors with tc-native
Date Wed, 22 Jun 2016 13:01:38 GMT
Hi Mark,

Am 22.06.2016 um 13:20 schrieb Mark Thomas:
> A while ago I observed unexpected APR_EGENERAL errors being returned
> when performing SSL reads. I was unable to identify the root cause but I
> did discover that if those errors were treated as EAGAIN, processing
> continued normally. As a result, I committed [1].
>
> A report on the users list [2] has highlighted that, in some
> circumstances at least, [1] is the wrong thing to do. I have therefore
> investigated the circumstances that led to [1] further. The relevant
> code is [3].
>
> With some local debug logging I have discovered that in the case [1] was
> trying to address the result of the SSL_read call at [3] is as follows:
> s == -1
> i == 5 (SSL_ERROR_SYSCALL)
> rv = 730035
>
> Subtracting the 720000 offset from rv gives the OS error as 10035 which
> [4] gives as WSAEWOULDBLOCK which looks exactly like EAGAIN to me.
>
> Based on the above, my conclusion is that [2] was caused by some other
> windows error which was incorrectly handled as EAGAIN.
>
> Therefore, I'd like to propose something along the following lines for
> tc-native:
> Index: src/sslnetwork.c
> ===================================================================
> --- src/sslnetwork.c    (revision 1749592)
> +++ src/sslnetwork.c    (working copy)
> @@ -427,7 +427,11 @@
>                          con->shutdown_type = SSL_SHUTDOWN_TYPE_STANDARD;
>                          return APR_EOF;
>                      }
> -#if !defined(_WIN32)
> +#if defined(_WIN32)
> +                    else if (rv == 730035 && timeout == 0) {
> +                        return APR_EAGAIN;
> +                    }
> +#else
>                      else if (APR_STATUS_IS_EINTR(rv)) {
>                          /* Interrupted by signal
>                           */

... and reverting [1] ?

> I'd appreciate some review of this change as I know C is not my strong
> point. The hard-coded value for the test of rv looks wrong to me. Is
> there a better way to do this? Any other review comments?
>
> Obviously some changes will be required on the Tomcat side as well. I'm
> still looking at those as I think I have discovered another issue that
> was masked by [1].

File apr_errno.h contains a macro

   APR_STATUS_IS_EAGAIN(s)

which in the WIN32 case is defined as:

   ((s) == APR_EAGAIN \
   || (s) == APR_OS_START_SYSERR + ERROR_NO_DATA \
   || (s) == APR_OS_START_SYSERR + ERROR_NO_PROC_SLOTS \
   || (s) == APR_OS_START_SYSERR + ERROR_NESTING_NOT_ALLOWED \
   || (s) == APR_OS_START_SYSERR + ERROR_MAX_THRDS_REACHED \
   || (s) == APR_OS_START_SYSERR + ERROR_LOCK_VIOLATION \
   || (s) == APR_OS_START_SYSERR + WSAEWOULDBLOCK)

so one could use "APR_STATUS_IS_EAGAIN(rv)" instead of "rv == 730035" as 
a condition. This would be broader than your suggestion. Whether it 
still separates the case the user observed from the one you want to 
handle with could maybe be tested by the user in [2]?

Finally if using the macro, one could also likely just drop the "#if 
defined(_WIN32)" before the EAGAIN test, because the macro is also 
defined for other platforms. For Unix/Linux either as "((s) == 
APR_EAGAIN)" or "((s) == APR_EAGAIN || (s) == EWOULDBLOCK)".

> [1] http://svn.apache.org/r1534619
> [2]
> https://lists.apache.org/thread.html/170386a4bd6ec3d66bf81ca1e43838d11a403569993cb5bd2bddbdb2@%3Cusers.tomcat.apache.org%3E
> [3]
> http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslnetwork.c?view=annotate#l407
> [4]
> https://msdn.microsoft.com/en-gb/library/windows/desktop/ms740668(v=vs.85).aspx

Regards,

Rainer

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message