Here are two patches that address the current bug, for comparison.
apr_dupfix.patch keeps the cleanup_kill -> dup2 -> register_cleanup
logic that protects us from dup2'ing into an apr_file_close()ed fd.
apr_duprevert.patch keeps the reorganization of the code, but drops
the cleanup_kill and register_cleanup in the dup2 case, in favor of
trusting the current cleanups (and that they correpond to ->flags.)
I honestly like the second patch better, only if we agree to not support
apr_file_dup2()ing into any apr_file_t that's already been apr_file_close()d.
Supporting that behavior will be a PITA on Win32.
In *BOTH* patches we revert apr_file_dup() assumption that fd 0..2 are
inherited, to reflect Jeff's concern with that change.
Please express a preference and I'll commit the favored patch and
test on OS/X prior to T&R of 0.9.2.
Bill
Index: file_io/unix/filedup.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/filedup.c,v
retrieving revision 1.60
diff -u -r1.60 filedup.c
--- file_io/unix/filedup.c 19 Mar 2003 10:17:26 -0000 1.60
+++ file_io/unix/filedup.c 21 Mar 2003 17:31:58 -0000
@@ -117,16 +117,23 @@
/* make sure unget behavior is consistent */
(*new_file)->ungetchar = old_file->ungetchar;
- /* apr_file_dup() clears the inherit attribute for normal files,
- * but sets the inherit attribute for std[out,in,err] fd's.
- * The user must call apr_file_inherit_[un]set() on the dupped
- * apr_file_t when desired.
- */
- if ((*new_file)->filedes <= 2) {
- (*new_file)->flags = old_file->flags | APR_INHERIT;
+ if (which_dup == 1) {
+ /* apr_file_dup() retains all old_file flags with the exceptions
+ * of APR_INHERIT and APR_FILE_NOCLEANUP.
+ * The user must call apr_file_inherit_set() on the dupped
+ * apr_file_t when desired.
+ */
+ (*new_file)->flags = old_file->flags
+ & ~(APR_INHERIT | APR_FILE_NOCLEANUP);
}
- else {
- (*new_file)->flags = old_file->flags & ~APR_INHERIT;
+ else /* which_dup == 2 */ {
+ /* apr_file_dup2() must respect the original settings for the
+ * new_file->flags inheritence. If there is no cleanup, we
+ * are finished already
+ */
+ if ((*new_file)->flags & APR_FILE_NOCLEANUP) {
+ return APR_SUCCESS;
+ }
}
apr_pool_cleanup_register((*new_file)->pool, (void *)(*new_file),
Index: file_io/unix/filedup.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/filedup.c,v
retrieving revision 1.60
diff -u -r1.60 filedup.c
--- file_io/unix/filedup.c 19 Mar 2003 10:17:26 -0000 1.60
+++ file_io/unix/filedup.c 21 Mar 2003 22:31:05 -0000
@@ -117,23 +117,27 @@
/* make sure unget behavior is consistent */
(*new_file)->ungetchar = old_file->ungetchar;
- /* apr_file_dup() clears the inherit attribute for normal files,
- * but sets the inherit attribute for std[out,in,err] fd's.
- * The user must call apr_file_inherit_[un]set() on the dupped
- * apr_file_t when desired.
+ /* apr_file_dup2() retains the original cleanup, reflecting
+ * the correct inherit and nocleanup flags. However,
+ * apr_file_dup2() cannot be called against an apr_file_t
+ * closed with apr_file_close, since the cleanup has been
+ * destroyed already.
*/
- if ((*new_file)->filedes <= 2) {
- (*new_file)->flags = old_file->flags | APR_INHERIT;
- }
- else {
- (*new_file)->flags = old_file->flags & ~APR_INHERIT;
+ if (which_dup == 2) {
+ return APR_SUCCESS;
}
+ /* apr_file_dup() retains all old_file flags with the exceptions
+ * of APR_INHERIT and APR_FILE_NOCLEANUP.
+ * The user must call apr_file_inherit_set() on the dupped
+ * apr_file_t when desired.
+ */
+ (*new_file)->flags = old_file->flags
+ & ~(APR_INHERIT | APR_FILE_NOCLEANUP);
+
apr_pool_cleanup_register((*new_file)->pool, (void *)(*new_file),
apr_unix_file_cleanup,
- ((*new_file)->flags & APR_INHERIT)
- ? apr_pool_cleanup_null
- : apr_unix_file_cleanup);
+ apr_unix_file_cleanup);
return APR_SUCCESS;
}
@@ -147,14 +151,6 @@
APR_DECLARE(apr_status_t) apr_file_dup2(apr_file_t *new_file,
apr_file_t *old_file, apr_pool_t *p)
{
- /* an existing apr_file_t may already be closed, and therefore
- * have no cleanup remaining; but we don't want to double-register
- * the same cleanup in _file_dup. Kill the existing cleanup before
- * invoking _file_dup to the existing new_file.
- */
- apr_pool_cleanup_kill(new_file->pool, (void *)(new_file),
- apr_unix_file_cleanup);
-
return _file_dup(&new_file, old_file, p, 2);
}
