tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: SSL errors with tc-native
Date Wed, 22 Jun 2016 13:06:40 GMT
On 22/06/2016 14:01, Rainer Jung wrote:
> 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] ?

Yes.

>> 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]?

That is much better.

> 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)".

Makes sense.

Thanks for the review. I'll adjust the patch accordingly and commit.

Mark


> 
>> [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
> 


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


Mime
View raw message