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 Thu, 09 Aug 2012 21:50:27 GMT
On Thu, Aug 9, 2012 at 4:28 PM, Jeff Trawick <trawick@gmail.com> wrote:
> On Thu, Aug 9, 2012 at 3:52 PM, Jeff Trawick <trawick@gmail.com> wrote:
>> On Thu, Aug 9, 2012 at 2:26 PM, Claudio Caldato (MS OPEN TECH)
>> <claudioc@microsoft.com> wrote:
>>> Better patch, fixed minor issue after another code review.
>>
>> I tested and seemed to get good results, but my testing isn't
>> reproducible enough with/without various patches to be conclusive.
>>
>> A couple of comments on the patch itself:
>>
>> * Why is BytesRead initialized to 0 in the other function?  (I didn't
>> include that part of your patch in my test.)
>> * Pass apr_get_netos_error() to ap_log_error instead of rc after a
>> Winsock function like recv() fails.  (apr_get_os_error() otherwise)
>>
>> More comments below:
>>
>>
>>> Thanks
>>>
>>> Claudio
>>
>>> From: Claudio Caldato (MS OPEN TECH) [mailto:claudioc@microsoft.com]
>>> Sent: Thursday, August 9, 2012 11:13 AM
>>> To: dev@httpd.apache.org
>>> Subject: Fix for Windows bug#52476
>>>
>>>
>>>
>>> Please code review the fix and let me know if you find any issue.
>>>
>>>
>>>
>>> Attached is the proposed patch for
>>>
>>> server\mpm\winnt\child.c
>>>
>>>
>>>
>>> Summary for code reviewers:
>>>
>>> If AcceptFilter is ‘connect’ or ‘none’, we read data from socket on worker
>>> thread. We use blocking recv and assign context->overlapped.Pointer to heap
>>> allocated buffer. It is the same procedure as in case of ‘AcceptFilter
>>> data’, but done in worker thread to keep listen thread unblocked.
>>
>> The big picture doesn't seem correct.  The main path of httpd (that
>> stuff called via ap_run_process_connection()) needs to be able to read
>> at will.  It shouldn't help to move a read() to before the call to
>> ap_run_process_connection().
>>
>>
>>> Note:
>>>
>>> It looks like context with overlapped.Pointer == NULL is not processed
>>> correctly in windows version of httpd. It may be related to the fact that
>>> winnt_insert_network_bucket() rejects context records with
>>> overlapped.Pointer == NULL
>>
>> Uhh, maybe I'm confused but that sounds like the issue!  (I'm not
>> familiar with that hook.)
>>
>> What about this patch?
>>
>> Index: server/mpm/winnt/child.c
>> ===================================================================
>> --- server/mpm/winnt/child.c    (revision 1371377)
>> +++ server/mpm/winnt/child.c    (working copy)
>> @@ -780,11 +780,15 @@
>>      apr_bucket *e;
>>      winnt_conn_ctx_t *context = ap_get_module_config(c->conn_config,
>>                                                       &mpm_winnt_module);
>> -    if (context == NULL || (e = context->overlapped.Pointer) == NULL)
>> +    if (context == NULL) {
>>          return AP_DECLINED;
>> +    }
>>
>> -    /* seed the brigade with AcceptEx read heap bucket */
>> -    APR_BRIGADE_INSERT_HEAD(bb, e);
>> +    if ((e = context->overlapped.Pointer) != NULL) {
>> +        /* seed the brigade with AcceptEx read heap bucket */
>> +        APR_BRIGADE_INSERT_HEAD(bb, e);
>> +    }
>> +
>>      /* also seed the brigade with the client socket. */
>>      e = apr_bucket_socket_create(socket, c->bucket_alloc);
>>      APR_BRIGADE_INSERT_TAIL(bb, e);
>
> no, that's wrong
>
> you only need to put the socket bucket there if you need to put data
> in front of it; (when declining, core's insert_network_bucket hook
> will do the right thing with the socket)
>
> AFAICT there's no functional issue with winnt_insert_network_bucket,
> though it could defer to core to handle the socket bucket
>
>>
>>>
>>>
>>>
>>>
>>>
>>> Please advise on what the next step(s) should be.
>>>
>>>
>>>
>>> Thanks
>>>
>>> Claudio

Attached is an alternate version of your patch which uses a different
control over how many bytes should be read.

With TO_READ = 0 (disable your code), it falls over consistently.
With TO_READ = 1 (read just 1 byte) it works reasonably consistently
(1 handshake failure in 100s of attempted connections).

Is there something about the state of the socket that gets reset once
a vanilla recv() is performed?  (a little more later)

wrowe had the suggestion recently that we weren't manufacturing the
apr socket properly and the socket state was wrong.  I think that
creation of the apr socket is as good as it can be, though in some
experiments I did I was left nervous about this code on the listening
socket:

        /* The event needs to be removed from the accepted socket,
         * if not removed from the listen socket prior to accept(),
         */
        rv = WSAEventSelect(nlsd, events[2], FD_ACCEPT);
        if (rv) {
            ap_log_error(APLOG_MARK, APLOG_ERR,
                         apr_get_netos_error(), ap_server_conf, APLOGNO(00333)
                         "WSAEventSelect() failed.");
            CloseHandle(events[2]);
            return 1;
        }

This makes the listening socket non-blocking, and any attempt to set
it blocking results in WSAEINVAL.  I dunno which of this leaks into
the connected child, or if is related to the observation that the
vanilla recv() somehow allows the apr-ized socket to work fine.

We do run this code on the client socket for AcceptFilter None:

            /* Per MSDN, cancel the inherited association of this socket
             * to the WSAEventSelect API, and restore the state corresponding
             * to apr_os_sock_make's default assumptions (really, a flaw within
             * os_sock_make and os_sock_put that it does not query).
             */
            WSAEventSelect(context->accept_socket, 0, 0);

I guess this is all okay, but voodoo is all that comes to mind due to
the first WSAEventSelect() magically making the socket non-blocking.

Mime
View raw message