Re: [PATCH] Duplicate close of file handle on WIN32

Well, the only problem with that patch (or rather, my slightly modified
version of it) is that it doesn't work for me on Windows XP. The file
descriptor gets opened just fine, and we no longer have a double-close
bug -- but for some reason I haven't been able to determine yet, Neon
can't read from the descriptor.

I'm pasting my patch here in the hope that somebody can tell me what I'm
doing wrong...

(BTW, we really _should_ be checking the return code from
ne_set_request_body_fd in svn_ra_dav__parsed_request.)

(BTW #2, I think we should introduce two new types into APR:
apr_intptr_t and apr_uintptr_t. That would eliminate one #if in this patch.)

Index: io.c
===================================================================
--- io.c (revision 4676)
+++ io.c (working copy)
@@ -1657,9 +1657,10 @@
 close_file_descriptor (void *baton)
 {
   int fd = (int) baton;
- _close (fd);
- /* Ignore errors from close, because we can't do anything about them. */
- return APR_SUCCESS;
+ if (0 > _close (fd))
+ return APR_EBADF;
+ else
+ return APR_SUCCESS;
 }
 #endif
 
@@ -1674,23 +1675,30 @@
 #ifndef SVN_WIN32
       *fd_p = fd;
 #else
- *fd_p = _open_osfhandle ((long) fd, _O_RDWR);
-
- /* We must close the file descriptor when the apr_file_t is
- closed, otherwise we'll run out of them. What happens if the
- underlyig file handle is closed first is anyone's guess, so
- the pool cleanup just ignores errors from the close. I hope
- the RTL frees the FD slot before closing the handle ... */
- if (*fd_p < 0)
- status = APR_EBADF;
+ /* APR's pool cleanup closes the original file handle. Since we
+ can't meddle with that, and we *do* have to close the file
+ descriptor (which also closes the underlying handle), we'll
+ create our file descriptor from a duplicate of the handle. */
+ HANDLE process = GetCurrentProcess();
+ HANDLE duplicate;
+ if (!DuplicateHandle (process, fd, process, &duplicate,
+ 0, FALSE, DUPLICATE_SAME_ACCESS))
+ status = apr_get_os_error();
       else
         {
- /* FIXME: This bit of code assumes that the first element of
- an apr_file_t on Win32 is a pool. It also assumes an int
- will fit into a void*. Please, let's get rid of this ASAP! */
- apr_pool_t *cntxt = *(apr_pool_t**) file;
- apr_pool_cleanup_register (cntxt, (void*) *fd_p,
- close_file_descriptor, NULL);
+#if APR_SIZEOF_VOIDP > 4
+ apr_int64_t os_handle = (apr_int64_t) duplicate;
+#else
+ long os_handle = (long) duplicate;
+#endif
+ *fd_p = _open_osfhandle (os_handle, _O_RDWR);
+ if (*fd_p < 0)
+ status = APR_EBADF;
+ else
+ apr_pool_cleanup_register (apr_file_pool_get (file),
+ (void*) *fd_p,
+ close_file_descriptor,
+ apr_pool_cleanup_null);
         }
 #endif
     }

-- 
Brane Čibej   <brane_at_xbc.nu>   http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org