httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: Fix for Windows bug#52476
Date Fri, 10 Aug 2012 17:43:12 GMT
On Fri, Aug 10, 2012 at 1:31 PM, Jeff Trawick <trawick@gmail.com> wrote:
> On Fri, Aug 10, 2012 at 12:41 PM, Jeff Trawick <trawick@gmail.com> wrote:
>> On Thu, Aug 9, 2012 at 7:35 PM, William A. Rowe Jr. <wrowe@rowe-clan.net> wrote:
>>> On 8/9/2012 11:26 AM, Claudio Caldato (MS OPEN TECH) wrote:
>>>> Better patch, fixed minor issue after another code review.
>>>
>>> Sadly, it won't fix the defect.
>>>
>>> Yes, you are successfullly performing a blocking initial read.
>>>
>>> And the pipe remains unblocked for the rest of the connection, so any
>>> further blocking reads will be ignored (just like the initial read by
>>> mod_ssl failed to block).  Any module expecting blocking behavior on
>>> subsequent reads will be disappointed.
>>
>> The SSL case with the chitchat to set up the connection is a bit
>> different than the simple case where as soon as you read you have
>> everything needed, so it does seem moderately interesting that the
>> read|peek makes the simple SSL testcase work.
>>
>> Also in the FWLIW department:
>>
>> a. once we get to the process_connection hook, APR's limited knowledge
>> of the socket state (timeout, non-blocking) is the same on Linux and
>> Windows
>>
>> (That was expected all along.)
>>
>> I had hoped to query state directly from the OS socket too, but
>> apparently the Microsoft folks don't think you need to be able to do
>> that, and I don't see any external sysinternals|other tools to tell
>> you anything interesting about the socket other than the normal
>> netstat stuff.
>>
>> b. Using WSAEnumNetworkEvents to clear any existing events didn't seem to help
>>
>>         /* start new */
>>         rv = WSAEnumNetworkEvents(context->accept_socket, 0, &lpNetworkEvents);
>>         if (rv == SOCKET_ERROR) {
>>             ap_log_error(APLOG_MARK, APLOG_WARNING, 0, ap_server_conf,
>>                          "failed to clear network events on connected socket");
>>         }
>>         /* end new */
>>         apr_os_sock_make(&context->sock, &sockinfo, context->ptrans);
>>
>> c. switching out the WSAEvent stuff for a straight
>> select(is-listener-readable) doesn't work around the problem either
>
> The APR socket gets set to non-blocking prior to the first call to
> apr_socket_recv():
>
> #0  apr_socket_opt_set (sock=0x2933890, opt=8, on=1)
>     at network_io/win32/sockopt.c:104
> #1  0x00423daa in ap_core_output_filter (f=0xc2bb310, new_bb=0x2933d20)
>     at core_filters.c:394
> #2  0x0042197c in ap_pass_brigade (next=0xc2bb310, bb=0x2933d20)
>     at util_filter.c:533
> #3  0x0045be1e in bio_filter_out_pass (bio=<value optimized out>)
>     at ssl_engine_io.c:134
> #4  bio_filter_out_flush (bio=<value optimized out>) at ssl_engine_io.c:155
> #5  0x0045bff9 in bio_filter_in_read (bio=0x28aa9a8,
>     in=0x2899348 "0╪ \f\003\001", inlen=11) at ssl_engine_io.c:458
> #6  0x004b30f7 in BIO_read ()
> #7  0x00c1b798 in ?? ()
>
> We picked up that apr_socket_opt_set() from the async-dev branch with
> r327872, though the timeout calls in there were changed subsequently.
> I wonder if that call is stray and it doesn't get along with the
> timeout handling on Windows because of the SO_RCVTIMEO/SO_SENDTIMEO
> use on Windows, in lieu of non-blocking socket + poll like on Unix.
>
> At the time it was added, the new code was
>
> apr_socket_timeout_set(client_socket, 0)
> apr_socket_opt_set(client_socket, APR_SO_NONBLOCK, 1)
>
> (redundant unless there was some APR glitch at the time)
>
> The apr_socket_timeout_set() was subsequently removed.
>
> I'll proceed with the obvious test.  (I'm pretending that someone is
> out there reading this and will chime in with any hints or other
> comments based on my stream of observations :) )

This patch is testing great so far with the AcceptFilter-none+SSL
scenario on Windows.

Index: server/core_filters.c
===================================================================
--- server/core_filters.c       (revision 1371377)
+++ server/core_filters.c       (working copy)
@@ -391,10 +391,6 @@
     if (ctx == NULL) {
         ctx = apr_pcalloc(c->pool, sizeof(*ctx));
         net->out_ctx = (core_output_filter_ctx_t *)ctx;
-        rv = apr_socket_opt_set(net->client_socket, APR_SO_NONBLOCK, 1);
-        if (rv != APR_SUCCESS) {
-            return rv;
-        }
         /*
          * Need to create tmp brigade with correct lifetime. Passing
          * NULL to apr_brigade_split_ex would result in a brigade

I'll run it through the test framework on Linux before committing.

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Mime
View raw message