On Fri, 2007-05-11 at 08:14 -0500, William A. Rowe, Jr. wrote:
> Trouble with the submitted patch is that it isn't thread safe, I see all
> sorts of subtle races in this code :(
Yes, I was afraid someone may notice :-(
I'm attaching a patch that features a mutex to make things safer.
Hopefully a bit better. Also, I dropped OS2 and Win32 stuff - no need.
> But maybe I'm mis-reading things - does this only get toggled within the
> child process after fork()?
I think so.
--
Bojan
Index: memory/unix/apr_pools.c
===================================================================
--- memory/unix/apr_pools.c (revision 537319)
+++ memory/unix/apr_pools.c (working copy)
@@ -515,6 +515,11 @@
static apr_file_t *file_stderr = NULL;
#endif /* (APR_POOL_DEBUG & APR_POOL_DEBUG_VERBOSE_ALL) */
+static int cleanup_for_exec = 0;
+#if APR_HAS_THREADS
+static apr_thread_mutex_t *cleanup_for_exec_mutex = NULL;
+#endif
+
/*
* Local functions
*/
@@ -577,6 +582,14 @@
apr_allocator_owner_set(global_allocator, global_pool);
+#if APR_HAS_THREADS
+ if ((rv = apr_thread_mutex_create(&cleanup_for_exec_mutex,
+ APR_THREAD_MUTEX_DEFAULT,
+ global_pool)) != APR_SUCCESS) {
+ return rv;
+ }
+#endif /* APR_HAS_THREADS */
+
return APR_SUCCESS;
}
@@ -2100,6 +2113,11 @@
cleanup_pool_for_exec(p);
}
+int apr_cleanup_is_for_exec()
+{
+ return cleanup_for_exec;
+}
+
APR_DECLARE(void) apr_pool_cleanup_for_exec(void)
{
#if !defined(WIN32) && !defined(OS2)
@@ -2110,9 +2128,21 @@
* be automajically closed. The only problem is with
* file handles that are open, but there isn't much
* I can do about that (except if the child decides
- * to go out and close them
+ * to go out and close them).
*/
+
+#if APR_HAS_THREADS
+ apr_thread_mutex_lock(cleanup_for_exec_mutex);
+#endif
+
+ cleanup_for_exec = 1;
cleanup_pool_for_exec(global_pool);
+ cleanup_for_exec = 0;
+
+#if APR_HAS_THREADS
+ apr_thread_mutex_unlock(cleanup_for_exec_mutex);
+#endif
+
#endif /* !defined(WIN32) && !defined(OS2) */
}
Index: include/arch/apr_private_common.h
===================================================================
--- include/arch/apr_private_common.h (revision 537319)
+++ include/arch/apr_private_common.h (working copy)
@@ -39,4 +39,7 @@
#define APR_UINT32_TRUNC_CAST apr_uint32_t
#define APR_UINT32_MAX 0xFFFFFFFFUL
+/* cleanup_for_exec */
+int apr_cleanup_is_for_exec();
+
#endif /*APR_PRIVATE_COMMON_H*/
Index: file_io/unix/readwrite.c
===================================================================
--- file_io/unix/readwrite.c (revision 537319)
+++ file_io/unix/readwrite.c (working copy)
@@ -319,6 +319,10 @@
APR_DECLARE(apr_status_t) apr_file_flush(apr_file_t *thefile)
{
+ if (apr_cleanup_is_for_exec()) {
+ return APR_SUCCESS;
+ }
+
if (thefile->buffered) {
if (thefile->direction == 1 && thefile->bufpos) {
apr_ssize_t written;
