Return-Path: Delivered-To: apmail-apr-dev-archive@apr.apache.org Received: (qmail 40196 invoked by uid 500); 18 Mar 2003 05:47:17 -0000 Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Delivered-To: mailing list dev@apr.apache.org Received: (qmail 40182 invoked from network); 18 Mar 2003 05:47:16 -0000 Errors-To: Message-Id: <5.2.0.9.2.20030317234345.03651828@pop3.rowe-clan.net> X-Sender: wrowe%rowe-clan.net@pop3.rowe-clan.net X-Mailer: QUALCOMM Windows Eudora Version 5.2.0.9 Date: Mon, 17 Mar 2003 23:47:44 -0600 To: dev@apr.apache.org From: "William A. Rowe, Jr." Subject: [Patch] redux; discussion of FD_CLOEXEC and APR_INHERIT In-Reply-To: <5.2.0.9.2.20030317170204.0342bd78@pop3.rowe-clan.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=====================_693480234==_" X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N --=====================_693480234==_ Content-Type: text/plain; charset="us-ascii" 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... --=====================_693480234==_ Content-Type: text/plain; charset="us-ascii" Content-Disposition: attachment; filename="apr_inherit.patch" ? 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) --=====================_693480234==_ Content-Type: text/plain; charset="us-ascii" --=====================_693480234==_--