apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject [Patch] for discussion of FD_CLOEXEC and APR_INHERIT
Date Mon, 17 Mar 2003 23:50:42 GMT
To reassure you all, it looks like unix/pipe.c does things correctly - it's everything
else that's goofy ;-)

Here are the patches to the file schema ... one more change to apr_arch_inherit
is still required for socket fd's, and there are socket fd code changes, but this 
should get the discussion started...

Premise: the default is uninherited handles, backed by FD_CLOEXEC where that
symbol is defined, unless APR_INHERIT is passed to apr_file_open()'s flags or
the user calls apr_file_inherit_set.

Pipes are automatically inherited, and must be apr_file_inherit_unset() when
desired.  Obviously only one side of the handle is changed, usually.

Liberal comments sprinkled below - I'll post the complete patch either tomorrow
morning or perhaps tonight to include socket code patches.  Feedback and comments
would be very greatly appreciated!!!

Bill

--- include/arch/unix/apr_arch_inherit.h	6 Jan 2003 23:44:26 -0000	1.1
+++ include/arch/unix/apr_arch_inherit.h	17 Mar 2003 22:58:31 -0000
@@ -59,6 +59,46 @@
 
 #define APR_INHERIT (1 << 24)    /* Must not conflict with other bits */
 
+#ifdef FD_CLOEXEC
+
+#define APR_IMPLEMENT_INHERIT_SET(name, flag, pool, cleanup)            \
+apr_status_t apr_##name##_inherit_set(apr_##name##_t *the##name)        \
+{                                                                       \
+    if (!(the##name->flag & APR_INHERIT)) {                             \
+        int ffd = fcntl(the##name->filedes, F_GETFD, 0);                \
+        if (ffd >= 0)                                                   \
+            ffd = fcntl(the##name->filedes, F_SETFD, ffd & ~FD_CLOEXEC);\
+    /*  if (ffd < 0)                                                    \
+     *      XXX: What to do in this case?  No good ideas.               \
+     */                                                                 \
+        the##name->flag |= APR_INHERIT;                                 \
+        apr_pool_child_cleanup_set(the##name->pool,                     \
+                                   (void *)the##name,                   \
+                                   cleanup, apr_pool_cleanup_null);     \
+    }                                                                   \
+    return APR_SUCCESS;                                                 \
+}
+
+#define APR_IMPLEMENT_INHERIT_UNSET(name, flag, pool, cleanup)          \
+apr_status_t apr_##name##_inherit_unset(apr_##name##_t *the##name)      \
+{                                                                       \
+    if (the##name->flag & APR_INHERIT) {                                \
+        int ffd = fcntl(the##name->filedes, F_GETFD, 0);                \
+        if (ffd >= 0)                                                   \
+            ffd = fcntl(the##name->filedes, F_SETFD, ffd | FD_CLOEXEC); \
+    /*  if (ffd < 0)                                                    \
+     *      XXX: What to do in this case?  No good ideas.               \
+     */                                                                 \
+        the##name->flag &= ~APR_INHERIT;                                \
+        apr_pool_child_cleanup_set(the##name->pool,                     \
+                                   (void *)the##name,                   \
+                                   cleanup, cleanup);                   \
+    }                                                                   \
+    return APR_SUCCESS;                                                 \
+}
+
+#else
+
 #define APR_IMPLEMENT_INHERIT_SET(name, flag, pool, cleanup)        \
 apr_status_t apr_##name##_inherit_set(apr_##name##_t *the##name)    \
 {                                                                   \

# The patch above provides that we toggle the state of FD_CLOEXEC.  It might
# be overkill, since most platforms support only a single bit in GETFD/SETFD,
# but I was being cautious.

@@ -69,11 +109,6 @@
                                    cleanup, apr_pool_cleanup_null); \
     }                                                               \
     return APR_SUCCESS;                                             \
-}                                                                   \
-/* Deprecated */                                                    \
-void apr_##name##_set_inherit(apr_##name##_t *the##name)            \
-{                                                                   \
-    apr_##name##_inherit_set(the##name);                            \
 }
 
 #define APR_IMPLEMENT_INHERIT_UNSET(name, flag, pool, cleanup)      \

# This macro had to move below to escape from the #ifdef FD_CLOEXEC block...

@@ -86,7 +121,16 @@
                                    cleanup, cleanup);               \
     }                                                               \
     return APR_SUCCESS;                                             \
-}                                                                   \
+}
+
+#endif
+
+/* Deprecated */                                                    \
+void apr_##name##_set_inherit(apr_##name##_t *the##name)            \
+{                                                                   \
+    apr_##name##_inherit_set(the##name);                            \
+}
+
 /* Deprecated */                                                    \
 void apr_##name##_unset_inherit(apr_##name##_t *the##name)          \
 {                                                                   \



--- file_io/unix/open.c	6 Mar 2003 09:24:17 -0000	1.110
+++ file_io/unix/open.c	17 Mar 2003 22:58:30 -0000
@@ -149,11 +149,33 @@
 #endif
 
     if (perm == APR_OS_DEFAULT) {
-        fd = open(fname, oflags, 0666);
+        perm = 0x666;
     }
-    else {
-        fd = open(fname, oflags, apr_unix_perms2mode(perm));
-    } 
+
+    fd = open(fname, oflags, apr_unix_perms2mode(perm));
+
+#ifdef FD_CLOEXEC
+    /* If we are registering a cleanup - we match the FD_CLOEXEC
+     * state to our cleanup state. - meaning that either flags
+     * APR_FILE_NOCLEANUP or APR_INHERIT will inhibit CLOEXEC.
+     */
+    if ((fd >= 0) && !(flag & (APR_FILE_NOCLEANUP | APR_INHERIT))) {
+        /* XXX: in multithreaded applications, there is a race
+         * between open() above and setting the FD_CLOEXEC bit.
+         */
+        int ffd = fcntl(fd, F_GETFD, 0);
+        if (ffd >= 0) {
+            ffd = fcntl(fd, F_SETFD, ffd | FD_CLOEXEC);
+        }
+        if (ffd < 0) {
+            /* XXX: What to do in this case?  No good ideas; one option:
+             * close(fd);
+             * fd = -1;
+             */
+        }
+    }
+#endif
+
     if (fd < 0) {
        return errno;
     }

# The above is rearrangement it becomes simpler to follow the flow
# of open -> fdset, so that the initial state of CLOEXEC is initialized 
# correctly based on the settings of APR_FILE_NOCLEANUP and
# APR_INHERIT.  I don't have a good answer to the failure case though.

@@ -191,7 +213,10 @@
 
     if (!(flag & APR_FILE_NOCLEANUP)) {
         apr_pool_cleanup_register((*new)->pool, (void *)(*new), 
-                                  apr_unix_file_cleanup, apr_unix_file_cleanup);
+                                  apr_unix_file_cleanup, 
+                                  (flag & APR_INHERIT)
+                                      ? apr_unix_file_cleanup
+                                      : apr_pool_cleanup_null);
     }
     return APR_SUCCESS;
 }

# Here we need to respect the APR_INHERIT bit when registering the cleanup.

--- file_io/unix/mktemp.c	7 Jan 2003 00:52:53 -0000	1.27
+++ file_io/unix/mktemp.c	17 Mar 2003 22:58:30 -0000
@@ -225,6 +225,13 @@
     if (fd == -1) {
         return errno;
     }
+    /* XXX: We must reset several flags values as passed-in, since
+     * mkstemp didn't subscribe to our preference flags.
+     *
+     * We either have to unset the flags, or fix up the fd and other
+     * xthread and inherit bits appropriately.  Since gettemp() above
+     * calls apr_file_open, our flags are respected in that code path.
+     */
     apr_os_file_put(fp, &fd, flags, p);
     (*fp)->fname = apr_pstrdup(p, template);
 
# And I plan to add some comments about the sorry state of mktemp/mkstemp().
# I don't have an immediate plan - it looks like we need some heavyweight code
# to deal with all of the flags and permissions issues.

# Now for the uglier patches that fix some existing bugs...

--- file_io/unix/filedup.c	7 Jan 2003 00:52:53 -0000	1.53
+++ file_io/unix/filedup.c	17 Mar 2003 22:58:30 -0000
@@ -64,28 +64,29 @@
 {
     int rv;
     
-    if ((*new_file) == NULL) {
-        if (which_dup == 1) {
-            (*new_file) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
-            if ((*new_file) == NULL) {
-                return APR_ENOMEM;
-            }
-            (*new_file)->pool = p;
-        } else {
-            /* We can't dup2 unless we have a valid new_file */
+    if (which_dup == 2) {
+        if (*new_file == NULL) {
             return APR_EINVAL;
         }
-    }
-
-    if (which_dup == 2) {
+        apr_pool_cleanup_kill((*new_file)->pool, *new_file, 
+                              apr_unix_file_cleanup);
         rv = dup2(old_file->filedes, (*new_file)->filedes);
     } else {
-        rv = ((*new_file)->filedes = dup(old_file->filedes)); 
+        rv = dup(old_file->filedes);
     }
 
     if (rv == -1)
         return errno;
-    
+
+    if ((*new_file) == NULL) {
+        (*new_file) = (apr_file_t *)apr_pcalloc(p, sizeof(apr_file_t));
+        (*new_file)->pool = p;
+    }
+
+    if (which_dup != 2) {
+        (*new_file)->filedes = rv;
+    }
+
     (*new_file)->fname = apr_pstrdup(p, old_file->fname);
     (*new_file)->buffered = old_file->buffered;
 
# The code above is mainly geared to allow the reader to follow the
# code flow.  But it does break out some special cases (with netware)
# that correspond to changes below in the two public entry points below.

@@ -112,10 +113,35 @@
     /* make sure unget behavior is consistent */
     (*new_file)->ungetchar = old_file->ungetchar;
 
-    /* apr_file_dup() clears the inherit attribute, user must call 
-     * apr_file_inherit_set() again on the dupped handle, as necessary.
+    /* apr_file_dup() clears the inherit attribute except for fd's 0..2
+     * so the user must call apr_file_inherit_set() again on the dupped 
+     * handle, when desired.
      */
-    (*new_file)->flags = old_file->flags & ~APR_INHERIT;
+    if ((*new_file)->filedes <= 2) {
+        /* Default the stdin/out/err fd's to inherited */
+        (*new_file)->flags = old_file->flags | APR_INHERIT;
+    }
+    else {
+        (*new_file)->flags = old_file->flags & ~APR_INHERIT;
+    }
+
+    apr_pool_cleanup_register((*new_file)->pool, (void *)(*new_file),
+                              apr_unix_file_cleanup, 
+                              (old_file->flags & APR_INHERIT)
+                                  ? apr_pool_cleanup_null
+                                  : apr_unix_file_cleanup);
+
+#ifdef FD_CLOEXEC
+    int ffd = fcntl((*new_file)->filedes, F_GETFD, 0);
+    if ((ffd >= 0) && (old_file->flags & APR_INHERIT)) {
+        ffd = fcntl((*new_file)->filedes, F_SETFD, ffd | FD_CLOEXEC);
+    }
+    /*  if (ffd < 0)
+     *      XXX: What to do in this case?  No good ideas.
+     *           returning an error implies we have to go
+     *           back and close the original handle.
+     */                                                                 
+#endif
 
     return APR_SUCCESS;
 }

# This code is set up to properly register our cleanups based on the
# the fd 0..2 (stdin/out/err) v.s. other handles.  But the old code was
# simply broken - a closed file handle passed to apr_file_dup (e.g.
# one of the standard handles) would have resulted in no cleanup
# registered at all.

@@ -123,25 +149,19 @@
 APR_DECLARE(apr_status_t) apr_file_dup(apr_file_t **new_file,
                                        apr_file_t *old_file, apr_pool_t *p)
 {
-    apr_status_t rv;
-
-    rv = _file_dup(new_file, old_file, p, 1);
-    if (rv != APR_SUCCESS)
-        return rv;
-
-    /* we do this here as we don't want to double register an existing 
-     * apr_file_t for cleanup
-     */
-    apr_pool_cleanup_register((*new_file)->pool, (void *)(*new_file),
-                              apr_unix_file_cleanup, apr_unix_file_cleanup);
-    return rv;
-
+    return _file_dup(new_file, old_file, p, 1);
 }
 
 APR_DECLARE(apr_status_t) apr_file_dup2(apr_file_t *new_file,
                                         apr_file_t *old_file, apr_pool_t *p)
 {
 #ifdef NETWARE
+    /* XXX Netware must special-case any dup2(stdin/out/err,...)
+     * as provided by apr_file_open_std[in|out|err].
+     * At least we pre-close the target file...
+     */
+    apr_pool_cleanup_run(new_file->pool, new_file, 
+                         apr_unix_file_cleanup);
     return _file_dup(&new_file, old_file, p, 1);
 #else
     return _file_dup(&new_file, old_file, p, 2);

# The changes in the front of _file_dup made all of this code
# simpler to follow, I hope.

@@ -177,7 +200,9 @@
     if (!(old_file->flags & APR_FILE_NOCLEANUP)) {
         apr_pool_cleanup_register(p, (void *)(*new_file), 
                                   apr_unix_file_cleanup,
-                                  apr_unix_file_cleanup);
+                                  ((*new_file)->flags & APR_INHERIT)
+                                      ? apr_pool_cleanup_null
+                                      : apr_unix_file_cleanup);
     }
 
     old_file->filedes = -1;

# and once more, we set up the pool cleanup based on the setting
# of the inherit bit.



Mime
View raw message