With a small typo pointed out by Bjoern, and fixing at least the
prototypes for the socket v.s. file implementations of inherit, here
is the revised patch. It is 'theoretical' - I'll vet it on OS/X in
the morning.
I have one question; should we also be toggling sockets as
FD_CLOEXEC? Common sense tells me, yes - we should
set that flag by default in all the assignments scattered
throughout network_io/unix/sockets.c.
Enjoy the patch... comments greatly appreciated. It's a little
to quiet on this issue... and your *win32* hacker is coding the
Unix solution. Are you scared yet :-?
Bill
A: well, at least you should be...
? consider_accept_retry.patch
? foo
Index: file_io/os2/filedup.c
===================================================================
RCS file: /home/cvs/apr/file_io/os2/filedup.c,v
retrieving revision 1.32
diff -u -r1.32 filedup.c
--- file_io/os2/filedup.c 7 Jan 2003 00:52:51 -0000 1.32
+++ file_io/os2/filedup.c 18 Mar 2003 05:38:40 -0000
@@ -92,7 +92,8 @@
if (*new_file == NULL) {
apr_pool_cleanup_register(dup_file->pool, dup_file, apr_file_cleanup,
- apr_pool_cleanup_null);
+ (dup_file->flags & APR_INHERIT)
+ ? apr_pool_cleanup_null : apr_file_cleanup);
*new_file = dup_file;
}
Index: file_io/os2/open.c
===================================================================
RCS file: /home/cvs/apr/file_io/os2/open.c,v
retrieving revision 1.59
diff -u -r1.59 open.c
--- file_io/os2/open.c 2 Mar 2003 03:11:22 -0000 1.59
+++ file_io/os2/open.c 18 Mar 2003 05:38:40 -0000
@@ -67,7 +67,6 @@
}
-
APR_DECLARE(apr_status_t) apr_file_open(apr_file_t **new, const char *fname,
apr_int32_t flag, apr_fileperms_t perm, apr_pool_t *pool)
{
int oflags = 0;
Index: file_io/unix/filedup.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/filedup.c,v
retrieving revision 1.53
diff -u -r1.53 filedup.c
--- file_io/unix/filedup.c 7 Jan 2003 00:52:53 -0000 1.53
+++ file_io/unix/filedup.c 18 Mar 2003 05:38:40 -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;
@@ -112,10 +113,38 @@
/* 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;
+ }
+
+ /* 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,
+ (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;
}
@@ -123,25 +152,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);
@@ -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;
Index: file_io/unix/mktemp.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/mktemp.c,v
retrieving revision 1.27
diff -u -r1.27 mktemp.c
--- file_io/unix/mktemp.c 7 Jan 2003 00:52:53 -0000 1.27
+++ file_io/unix/mktemp.c 18 Mar 2003 05:38:40 -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);
Index: file_io/unix/open.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/open.c,v
retrieving revision 1.110
diff -u -r1.110 open.c
--- file_io/unix/open.c 6 Mar 2003 09:24:17 -0000 1.110
+++ file_io/unix/open.c 18 Mar 2003 05:38:40 -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;
}
@@ -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_pool_cleanup_null
+ : apr_unix_file_cleanup);
}
return APR_SUCCESS;
}
@@ -292,8 +317,8 @@
return apr_os_file_put(thefile, &fd, 0, pool);
}
-APR_IMPLEMENT_INHERIT_SET(file, flags, pool, apr_unix_file_cleanup)
+APR_IMPLEMENT_INHERIT_SET(file, flags, filedes, pool, apr_unix_file_cleanup)
-APR_IMPLEMENT_INHERIT_UNSET(file, flags, pool, apr_unix_file_cleanup)
+APR_IMPLEMENT_INHERIT_UNSET(file, flags, filedes, pool, apr_unix_file_cleanup)
APR_POOL_IMPLEMENT_ACCESSOR(file)
Index: include/arch/unix/apr_arch_inherit.h
===================================================================
RCS file: /home/cvs/apr/include/arch/unix/apr_arch_inherit.h,v
retrieving revision 1.1
diff -u -r1.1 apr_arch_inherit.h
--- include/arch/unix/apr_arch_inherit.h 6 Jan 2003 23:44:26 -0000
1.1
+++ include/arch/unix/apr_arch_inherit.h 18 Mar 2003 05:38:41 -0000
@@ -59,7 +59,47 @@
#define APR_INHERIT (1 << 24) /* Must not conflict with other bits */
-#define APR_IMPLEMENT_INHERIT_SET(name, flag, pool, cleanup) \
+#ifdef FD_CLOEXEC
+
+#define APR_IMPLEMENT_INHERIT_SET(name, flag, fd, 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->fd, F_GETFD, 0); \
+ if (ffd >= 0) \
+ ffd = fcntl(the##name->fd, 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, fd, 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->fd, F_GETFD, 0); \
+ if (ffd >= 0) \
+ ffd = fcntl(the##name->fd, 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, fd, pool, cleanup) \
apr_status_t apr_##name##_inherit_set(apr_##name##_t *the##name) \
{ \
if (!(the##name->flag & APR_INHERIT)) { \
@@ -69,14 +109,9 @@
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) \
+#define APR_IMPLEMENT_INHERIT_UNSET(name, flag, fd, pool, cleanup) \
apr_status_t apr_##name##_inherit_unset(apr_##name##_t *the##name) \
{ \
if (the##name->flag & APR_INHERIT) { \
@@ -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) \
{ \
Index: network_io/unix/sockets.c
===================================================================
RCS file: /home/cvs/apr/network_io/unix/sockets.c,v
retrieving revision 1.107
diff -u -r1.107 sockets.c
--- network_io/unix/sockets.c 6 Jan 2003 23:44:35 -0000 1.107
+++ network_io/unix/sockets.c 18 Mar 2003 05:38:41 -0000
@@ -391,9 +391,9 @@
return APR_SUCCESS;
}
-APR_IMPLEMENT_INHERIT_SET(socket, inherit, cntxt, socket_cleanup)
+APR_IMPLEMENT_INHERIT_SET(socket, inherit, socketdes, cntxt, socket_cleanup)
-APR_IMPLEMENT_INHERIT_UNSET(socket, inherit, cntxt, socket_cleanup)
+APR_IMPLEMENT_INHERIT_UNSET(socket, inherit, socketdes, cntxt, socket_cleanup)
/* deprecated */
apr_status_t apr_shutdown(apr_socket_t *thesocket, apr_shutdown_how_e how)
