You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Aaron Bannert <aa...@clove.org> on 2001/12/27 02:36:38 UTC
[PATCH] update apr_thread_exit's prototype and fix usage in worker
On Wed, Dec 26, 2001 at 04:54:00AM -0800, Brian Pane wrote:
> I'm seeing a segfault during child process shutdown with the
> worker MPM on Solaris. It appears to happen because the worker
> threads do this:
>
> static void *worker_thread(apr_thread_t *thd, void * dummy)
> {
> proc_info * ti = dummy;
> int process_slot = ti->pid;
> int thread_slot = ti->tid;
> apr_socket_t *csd = NULL;
> apr_pool_t *ptrans; /* Pool for per-transaction stuff */
> apr_status_t rv;
> ...
> rv = APR_SUCCESS;
> apr_thread_exit(thd, &rv);
> return NULL;
> }
>
> Note that "rv" is a local var...
> When apr_thread_join tries to dereference the saved apr_status_t*
> to get the return status, it's accessing memory on the stack of
> a now-defunct thread. This yields random data at best, or a segfault
> at worst.
>
> I was about to apply the simple fix: make the worker thread malloc
> an apr_status_t and pass the address of the malloc'ed block to
> apr_thread_exit. But that won't quite work, because apr_thread_join
> makes a copy of the return status, rather than passing the pointer
> itself back to the thread that does the join. So there would be
> no way to free the malloc'ed space.
>
> I think the only clean way to fix this is to change apr_thread_exit
> and apr_thread_join.
> Either:
> * malloc space for a copy of the return status in apr_thread_exit
> and free it in apr_thread join
> or:
> * add an apr_status_t to the apr_thread_t struct, and store the
> exit status there
The following patch does the trick. I've actually attached two patches,
the first is the APR change and the second is the update to the worker MPM
to use the new function signature. What's changed in APR: apr_thread_exit
no longer takes a pointer to an apr_status_t, which was very ackward. It
now takes a simple pass-by-copy apr_status_t parameter which is then
stored in the thread structure on all platforms and pulled back out with
a call to apr_thread_join.
* I did my best to get this to work on non-unix platforms, but I
will need help verifying my changes (especially netware).
** Will: Please verify my win32 changes. I don't recall if this patch
contains the changes we talked about for win32 from my original posting
of this patch.
p.s. I liked the name "exitval" that David put in Beos better than
what I had before "rv". Other than that this patch is pretty much
the same thing (minus what David already implemented on Beos) I
posted a couple months ago.
-aaron
Index: include/apr_thread_proc.h
===================================================================
RCS file: /home/cvs/apr/include/apr_thread_proc.h,v
retrieving revision 1.77
diff -u -u -r1.77 apr_thread_proc.h
--- include/apr_thread_proc.h 2001/11/27 22:10:31 1.77
+++ include/apr_thread_proc.h 2001/12/27 01:04:01
@@ -227,7 +227,7 @@
* @param retval The return value to pass back to any thread that cares
*/
APR_DECLARE(apr_status_t) apr_thread_exit(apr_thread_t *thd,
- apr_status_t *retval);
+ apr_status_t retval);
/**
* block until the desired thread stops executing.
Index: include/arch/netware/threadproc.h
===================================================================
RCS file: /home/cvs/apr/include/arch/netware/threadproc.h,v
retrieving revision 1.3
diff -u -u -r1.3 threadproc.h
--- include/arch/netware/threadproc.h 2001/10/05 16:56:21 1.3
+++ include/arch/netware/threadproc.h 2001/12/27 01:04:01
@@ -72,6 +72,7 @@
apr_int32_t cancel_how;
void *data;
apr_thread_start_t func;
+ apr_status_t exitval;
};
struct apr_threadattr_t {
Index: include/arch/os2/threadproc.h
===================================================================
RCS file: /home/cvs/apr/include/arch/os2/threadproc.h,v
retrieving revision 1.12
diff -u -u -r1.12 threadproc.h
--- include/arch/os2/threadproc.h 2001/09/02 05:44:43 1.12
+++ include/arch/os2/threadproc.h 2001/12/27 01:04:01
@@ -74,7 +74,7 @@
unsigned long tid;
apr_thread_start_t func;
void *data;
- apr_status_t rv;
+ apr_status_t exitval;
};
struct apr_threadkey_t {
Index: include/arch/unix/threadproc.h
===================================================================
RCS file: /home/cvs/apr/include/arch/unix/threadproc.h,v
retrieving revision 1.20
diff -u -u -r1.20 threadproc.h
--- include/arch/unix/threadproc.h 2001/08/27 03:17:15 1.20
+++ include/arch/unix/threadproc.h 2001/12/27 01:04:01
@@ -92,6 +92,7 @@
pthread_t *td;
void *data;
apr_thread_start_t func;
+ apr_status_t exitval;
};
struct apr_threadattr_t {
Index: include/arch/win32/threadproc.h
===================================================================
RCS file: /home/cvs/apr/include/arch/win32/threadproc.h,v
retrieving revision 1.16
diff -u -u -r1.16 threadproc.h
--- include/arch/win32/threadproc.h 2001/09/09 06:03:05 1.16
+++ include/arch/win32/threadproc.h 2001/12/27 01:04:02
@@ -68,6 +68,7 @@
apr_int32_t cancel_how;
void *data;
apr_thread_start_t func;
+ apr_status_t exitval;
};
struct apr_threadattr_t {
Index: threadproc/beos/thread.c
===================================================================
RCS file: /home/cvs/apr/threadproc/beos/thread.c,v
retrieving revision 1.29
diff -u -u -r1.29 thread.c
--- threadproc/beos/thread.c 2001/10/29 17:29:32 1.29
+++ threadproc/beos/thread.c 2001/12/27 01:04:02
@@ -142,10 +142,10 @@
return tid1 == tid2;
}
-APR_DECLARE(apr_status_t) apr_thread_exit(apr_thread_t *thd, apr_status_t *retval)
+APR_DECLARE(apr_status_t) apr_thread_exit(apr_thread_t *thd, apr_status_t retval)
{
apr_pool_destroy(thd->cntxt);
- thd->exitval = *retval;
+ thd->exitval = retval;
exit_thread ((status_t)(*retval));
/* This will never be reached... */
return APR_SUCCESS;
Index: threadproc/netware/thread.c
===================================================================
RCS file: /home/cvs/apr/threadproc/netware/thread.c,v
retrieving revision 1.8
diff -u -u -r1.8 thread.c
--- threadproc/netware/thread.c 2001/12/11 19:40:50 1.8
+++ threadproc/netware/thread.c 2001/12/27 01:04:03
@@ -182,10 +182,11 @@
}
apr_status_t apr_thread_exit(apr_thread_t *thd,
- apr_status_t *retval)
+ apr_status_t retval)
{
+ thd->exitval = retval;
apr_pool_destroy(thd->cntxt);
- NXThreadExit((void *)*retval);
+ NXThreadExit(NULL);
return APR_SUCCESS;
}
@@ -195,7 +196,8 @@
apr_status_t stat;
NXThreadId_t dthr;
- if ((stat = NXThreadJoin(thd->td, &dthr, (void *)&retval)) == 0) {
+ if ((stat = NXThreadJoin(thd->td, &dthr, NULL)) == 0) {
+ *retval = thd->exitval;
return APR_SUCCESS;
}
else {
Index: threadproc/os2/thread.c
===================================================================
RCS file: /home/cvs/apr/threadproc/os2/thread.c,v
retrieving revision 1.29
diff -u -u -r1.29 thread.c
--- threadproc/os2/thread.c 2001/09/02 05:34:55 1.29
+++ threadproc/os2/thread.c 2001/12/27 01:04:04
@@ -154,9 +154,9 @@
-APR_DECLARE(apr_status_t) apr_thread_exit(apr_thread_t *thd, apr_status_t *retval)
+APR_DECLARE(apr_status_t) apr_thread_exit(apr_thread_t *thd, apr_status_t retval)
{
- thd->rv = *retval;
+ thd->exitval = retval;
_endthread();
return -1; /* If we get here something's wrong */
}
@@ -176,7 +176,7 @@
if (rc == ERROR_INVALID_THREADID)
rc = 0; /* Thread had already terminated */
- *retval = (apr_status_t)thd->rv;
+ *retval = thd->exitval;
return APR_OS2_STATUS(rc);
}
Index: threadproc/unix/thread.c
===================================================================
RCS file: /home/cvs/apr/threadproc/unix/thread.c,v
retrieving revision 1.48
diff -u -u -r1.48 thread.c
--- threadproc/unix/thread.c 2001/09/02 03:44:37 1.48
+++ threadproc/unix/thread.c 2001/12/27 01:04:04
@@ -176,10 +176,11 @@
return pthread_equal(tid1, tid2);
}
-APR_DECLARE(apr_status_t) apr_thread_exit(apr_thread_t *thd, apr_status_t *retval)
+APR_DECLARE(apr_status_t) apr_thread_exit(apr_thread_t *thd, apr_status_t retval)
{
+ thd->exitval = retval;
apr_pool_destroy(thd->cntxt);
- pthread_exit(retval);
+ pthread_exit(NULL);
return APR_SUCCESS;
}
@@ -189,7 +190,7 @@
apr_status_t *thread_stat;
if ((stat = pthread_join(*thd->td,(void *)&thread_stat)) == 0) {
- *retval = thread_stat ? *thread_stat : APR_SUCCESS;
+ *retval = thd->exitval;
return APR_SUCCESS;
}
else {
Index: threadproc/win32/thread.c
===================================================================
RCS file: /home/cvs/apr/threadproc/win32/thread.c,v
retrieving revision 1.40
diff -u -u -r1.40 thread.c
--- threadproc/win32/thread.c 2001/11/27 02:32:37 1.40
+++ threadproc/win32/thread.c 2001/12/27 01:04:04
@@ -140,10 +140,11 @@
}
APR_DECLARE(apr_status_t) apr_thread_exit(apr_thread_t *thd,
- apr_status_t *retval)
+ apr_status_t retval)
{
+ thd->exitval = retval;
apr_pool_destroy(thd->cntxt);
- _endthreadex(*retval);
+ _endthreadex(0);
return APR_SUCCESS;
}
@@ -153,10 +154,8 @@
apr_status_t stat;
if ((stat = WaitForSingleObject(thd->td, INFINITE)) == WAIT_OBJECT_0) {
- if (GetExitCodeThread(thd->td, retval) == 0) {
- return APR_SUCCESS;
- }
- return apr_get_os_error();
+ *retval = thd->exitval;
+ return APR_SUCCESS;
}
else {
return stat;
Index: server/mpm/worker/worker.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/worker/worker.c,v
retrieving revision 1.54
diff -u -u -r1.54 worker.c
--- server/mpm/worker/worker.c 2001/12/25 12:24:14 1.54
+++ server/mpm/worker/worker.c 2001/12/27 01:28:12
@@ -750,9 +750,7 @@
ap_scoreboard_image->parent[process_slot].quiescing = 1;
kill(ap_my_pid, SIGTERM);
-/* Unsure if this can be safely uncommented. -aaron
apr_thread_exit(thd, APR_SUCCESS);
-*/
return NULL;
}
@@ -789,8 +787,7 @@
worker_thread_count--;
apr_thread_mutex_unlock(worker_thread_count_mutex);
- rv = APR_SUCCESS;
- apr_thread_exit(thd, &rv);
+ apr_thread_exit(thd, APR_SUCCESS);
return NULL;
}
@@ -879,8 +876,7 @@
* "life_status" is almost right, but it's in the worker's structure, and
* the name could be clearer. gla
*/
- rv = APR_SUCCESS;
- apr_thread_exit(thd, &rv);
+ apr_thread_exit(thd, APR_SUCCESS);
return NULL;
}
Re: [PATCH] update apr_thread_exit's prototype and fix usage in worker
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: <dr...@jetnet.co.uk>
Sent: Thursday, December 27, 2001 9:31 AM
> +1 from me.
>
> david
>
>
> > ** Will: Please verify my win32 changes. I don't recall if this patch
> > contains the changes we talked about for win32 from my original posting
> > of this patch.
I'll test, but presume it does and go ahead and commit [sometimes it's simpler
just to 'mop-up' :]
Re: [PATCH] update apr_thread_exit's prototype and fix usage in worker
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: <dr...@jetnet.co.uk>
Sent: Thursday, December 27, 2001 9:31 AM
> +1 from me.
>
> david
>
>
> > ** Will: Please verify my win32 changes. I don't recall if this patch
> > contains the changes we talked about for win32 from my original posting
> > of this patch.
I'll test, but presume it does and go ahead and commit [sometimes it's simpler
just to 'mop-up' :]
Re: [PATCH] update apr_thread_exit's prototype and fix usage in worker
Posted by dr...@jetnet.co.uk.
+1 from me.
david
> The following patch does the trick. I've actually attached two patches,
> the first is the APR change and the second is the update to the worker
> MPM to use the new function signature. What's changed in APR:
> apr_thread_exit no longer takes a pointer to an apr_status_t, which was
> very ackward. It now takes a simple pass-by-copy apr_status_t parameter
> which is then stored in the thread structure on all platforms and
> pulled back out with a call to apr_thread_join.
>
> * I did my best to get this to work on non-unix platforms, but I
> will need help verifying my changes (especially netware).
>
> ** Will: Please verify my win32 changes. I don't recall if this patch
> contains the changes we talked about for win32 from my original posting
> of this patch.
>
> p.s. I liked the name "exitval" that David put in Beos better than what
> I had before "rv". Other than that this patch is pretty much
> the same thing (minus what David already implemented on Beos) I
> posted a couple months ago.
>
> -aaron
Re: [PATCH] update apr_thread_exit's prototype and fix usage in worker
Posted by dr...@jetnet.co.uk.
+1 from me.
david
> The following patch does the trick. I've actually attached two patches,
> the first is the APR change and the second is the update to the worker
> MPM to use the new function signature. What's changed in APR:
> apr_thread_exit no longer takes a pointer to an apr_status_t, which was
> very ackward. It now takes a simple pass-by-copy apr_status_t parameter
> which is then stored in the thread structure on all platforms and
> pulled back out with a call to apr_thread_join.
>
> * I did my best to get this to work on non-unix platforms, but I
> will need help verifying my changes (especially netware).
>
> ** Will: Please verify my win32 changes. I don't recall if this patch
> contains the changes we talked about for win32 from my original posting
> of this patch.
>
> p.s. I liked the name "exitval" that David put in Beos better than what
> I had before "rv". Other than that this patch is pretty much
> the same thing (minus what David already implemented on Beos) I
> posted a couple months ago.
>
> -aaron