Re: cvs commit: httpd-2.0/server/mpm/winnt child.c

[Bill Stoddard <bi...@wstoddard.com>]

I have some questions about this patch (just now looking at porting it to 2.0):

ake@apache.org wrote:
> ake         2004/05/03 15:22:50
> 
>   Modified:    .        CHANGES
>                server/mpm/winnt child.c
>   Log:
>   Prevent Win32 pool corruption at startup
>   
>   
>   1.36      +17 -2     httpd-2.0/server/mpm/winnt/child.c
>   
>   Index: child.c
>   ===================================================================
>   RCS file: /home/cvs/httpd-2.0/server/mpm/winnt/child.c,v
>   retrieving revision 1.35
>   retrieving revision 1.36
>   diff -u -r1.35 -r1.36
>   --- child.c	15 Mar 2004 23:08:41 -0000	1.35
>   +++ child.c	3 May 2004 22:22:50 -0000	1.36
>   @@ -58,7 +58,7 @@
>    static unsigned int g_blocked_threads = 0;
>    static HANDLE max_requests_per_child_event;
>    
>   -
>   +static apr_thread_mutex_t  *child_lock;
>    static apr_thread_mutex_t  *qlock;
>    static PCOMP_CONTEXT qhead = NULL;
>    static PCOMP_CONTEXT qtail = NULL;
>   @@ -145,6 +145,7 @@
>                     */
>                    apr_allocator_t *allocator;
>    
>   +                apr_thread_mutex_lock(child_lock);
>                    context = (PCOMP_CONTEXT) apr_pcalloc(pchild, sizeof(COMP_CONTEXT));
>      
>                    context->Overlapped.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
>   @@ -152,6 +153,8 @@
>                        /* Hopefully this is a temporary condition ... */
>                        ap_log_error(APLOG_MARK,APLOG_WARNING, apr_get_os_error(), ap_server_conf,
>                                     "mpm_get_completion_context: CreateEvent failed.");
>   +
>   +                    apr_thread_mutex_unlock(child_lock);
>                        return NULL;
>                    }
>     
>   @@ -163,6 +166,8 @@
>                        ap_log_error(APLOG_MARK,APLOG_WARNING, rv, ap_server_conf,
>                                     "mpm_get_completion_context: Failed to create the transaction pool.");
>                        CloseHandle(context->Overlapped.hEvent);
>   +
>   +                    apr_thread_mutex_unlock(child_lock);
>                        return NULL;
>                    }
>                    apr_allocator_owner_set(allocator, context->ptrans);
>   @@ -171,6 +176,8 @@
>                    context->accept_socket = INVALID_SOCKET;
>                    context->ba = apr_bucket_alloc_create(pchild);
>                    apr_atomic_inc32(&num_completion_contexts); 
>   +
>   +                apr_thread_mutex_unlock(child_lock);
>                    break;
>                }
>            } else {

Here is the actual code from this section of the patch with my comments:

                 /* Allocate another context.
                  * Note:
                  * Multiple failures in the next two steps will cause the pchild pool
                  * to 'leak' storage. I don't think this is worth fixing...
                  */
                 apr_allocator_t *allocator;

===> lock here  apr_thread_mutex_lock(child_lock);
===> this presumes the calls to apr_pcalloc, createevent, et need protection. I think
===> they don't need protection

                 context = (PCOMP_CONTEXT) apr_pcalloc(pchild, sizeof(COMP_CONTEXT));

                 context->Overlapped.hEvent = CreateEvent(NULL, TRUE, FALSE, NULL);
                 if (context->Overlapped.hEvent == NULL) {
                     /* Hopefully this is a temporary condition ... */
                     ap_log_error(APLOG_MARK,APLOG_WARNING, apr_get_os_error(), ap_server_conf,
                                  "mpm_get_completion_context: CreateEvent failed.");

                     apr_thread_mutex_unlock(child_lock);
                     return NULL;
                 }

                 /* Create the tranaction pool */
==> needs protection?   apr_allocator_create(&allocator);
==> needs protection?   apr_allocator_max_free_set(allocator, ap_max_mem_free);
==> can we move the lock call here???
                 rv = apr_pool_create_ex(&context->ptrans, pchild, NULL, allocator);
==> and add unlock call here?
                 if (rv != APR_SUCCESS) {
                     ap_log_error(APLOG_MARK,APLOG_WARNING, rv, ap_server_conf,
                                  "mpm_get_completion_context: Failed to create the transaction pool.");
                     CloseHandle(context->Overlapped.hEvent);
==> and delete this?
                     apr_thread_mutex_unlock(child_lock);
                     return NULL;
                 }
                 apr_allocator_owner_set(allocator, context->ptrans);
                 apr_pool_tag(context->ptrans, "transaction");

                 context->accept_socket = INVALID_SOCKET;
                 context->ba = apr_bucket_alloc_create(pchild);
                 apr_atomic_inc32(&num_completion_contexts);
==> and delete this?
                 apr_thread_mutex_unlock(child_lock);
                 break;
             }


<snip>
>   +            apr_thread_mutex_lock(child_lock);
>                score_idx = apr_pcalloc(pchild, sizeof(int));
>                *score_idx = i;
>                apr_hash_set(ht, &child_handles[i], sizeof(HANDLE), score_idx);
>   +            apr_thread_mutex_unlock(child_lock);
>            }
This looks okay as child_handles and ht can be access my multiple threads at once, thus needs protection.

Comments?

Bill

Re: cvs commit: httpd-2.0/server/mpm/winnt child.c

[Bill Stoddard <bi...@wstoddard.com>]

Bill Stoddard wrote:

> I have some questions about this patch (just now looking at porting it 
> to 2.0):

Disregard this question, sorry for the noise. apr_pcalloc(pchild) call needs protection in this context 
because multiple threads (accept threads, one per listener) make the call.

Bill