You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ak...@apache.org on 2004/05/04 00:22:50 UTC
cvs commit: httpd-2.0/server/mpm/winnt child.c
ake 2004/05/03 15:22:50
Modified: . CHANGES
server/mpm/winnt child.c
Log:
Prevent Win32 pool corruption at startup
Revision Changes Path
1.1475 +2 -0 httpd-2.0/CHANGES
Index: CHANGES
===================================================================
RCS file: /home/cvs/httpd-2.0/CHANGES,v
retrieving revision 1.1474
retrieving revision 1.1475
diff -u -r1.1474 -r1.1475
--- CHANGES 26 Apr 2004 22:05:44 -0000 1.1474
+++ CHANGES 3 May 2004 22:22:50 -0000 1.1475
@@ -2,6 +2,8 @@
[Remove entries to the current 2.0 section below, when backported]
+ *) Prevent Win32 pool corruption at startup [Allan Edwards]
+
*) Removed old and unmaintained ap_add_named_module API and changed
the following APIs to return an error instead of hard exiting:
ap_add_module, ap_add_loaded_module, ap_setup_prelinked_modules,
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 {
@@ -419,6 +426,7 @@
if (context == NULL) {
/* allocate the completion context and the transaction pool */
apr_allocator_t *allocator;
+ apr_thread_mutex_lock(child_lock);
context = apr_pcalloc(pchild, sizeof(COMP_CONTEXT));
apr_allocator_create(&allocator);
apr_allocator_max_free_set(allocator, ap_max_mem_free);
@@ -426,6 +434,7 @@
apr_allocator_owner_set(allocator, context->ptrans);
apr_pool_tag(context->ptrans, "transaction");
context->ba = apr_bucket_alloc_create(pchild);
+ apr_thread_mutex_unlock(child_lock);
}
while (1) {
@@ -920,6 +929,8 @@
ap_log_error(APLOG_MARK,APLOG_NOTICE, APR_SUCCESS, ap_server_conf,
"Child %d: Starting %d worker threads.", my_pid, ap_threads_per_child);
child_handles = (HANDLE) apr_pcalloc(pchild, ap_threads_per_child * sizeof(int));
+ apr_thread_mutex_create(&child_lock, APR_THREAD_MUTEX_DEFAULT, pchild);
+
while (1) {
for (i = 0; i < ap_threads_per_child; i++) {
int *score_idx;
@@ -943,9 +954,11 @@
/* Save the score board index in ht keyed to the thread handle. We need this
* when cleaning up threads down below...
*/
+ 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);
}
/* Start the listener only when workers are available */
if (!listener_started && threads_created) {
@@ -1128,8 +1141,10 @@
CloseHandle(allowed_globals.jobsemaphore);
apr_thread_mutex_destroy(allowed_globals.jobmutex);
+ apr_thread_mutex_destroy(child_lock);
+
if (use_acceptex) {
- apr_thread_mutex_destroy(qlock);
+ apr_thread_mutex_destroy(qlock);
CloseHandle(qwait_event);
}
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c
Posted by 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
Re: cvs commit: httpd-2.0/server/mpm/winnt child.c
Posted by 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