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 20:28:49 GMT
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
>>
>>
>
>
>
> --
> Born in Roswell... married an alien...
> http://emptyhammock.com/



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

Mime
View raw message