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.
