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 22:35:21 GMT
On Thu, Aug 9, 2012 at 5:50 PM, Jeff Trawick <trawick@gmail.com> wrote:
> 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.

Woohoo!  I get consistent success with this bit of code in lieu of the
patch being discussed/tweaked in this thread.  I think this means that
simply calling recv() to change some aspect of the socket state is
what is needed, and the problem isn't related to the queuing of any
pre-read data.  (That's not to say that this is the desired way to
change that state, but it is better than nothing.)

#define TO_PEEK 1
        if (TO_PEEK > 0 &&
            (context->overlapped.Pointer == NULL) &&
            (context->accept_socket != INVALID_SOCKET)) {
            char buffer[TO_PEEK];

            rc = recv(context->accept_socket, buffer, TO_PEEK, MSG_PEEK);
            if (rc == SOCKET_ERROR) {
                ap_log_error(APLOG_MARK, APLOG_ERR, apr_get_netos_error(),
                             ap_server_conf, APLOGNO(00365)
                             "worker_main: recv(MSG_PEEK) error");
            }
        }

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

Mime
View raw message