[PATCH] Pipes on Win32 (fix resource leaks)

["Costello, Tim" <ti...@zcmgroup.com.au>]

Hi,

I hope this is the right list/forum for this message - apologies in advance
if it isn't. I'm writing to submit two patches against the current CVS tree
for APR to fix a couple of resource leaks. 

The first issue is that the pipe creation code used by mod_cgi in Apache 2.0
doesn't register cleanups for either the read or the write ends of the pipe
- so file handles (and event handles for pipes with asynchronous I/O mode
set) are never closed. The function in question is called
apr_create_nt_pipe, and the following patch fixes this (see also
apr_file_pipe_create). 

===== BEGIN PATCH =====

--- pipe.c	Mon Jul 23 17:47:27 2001
+++ apr/file_io/win32/pipe.c	Fri Oct 12 01:42:55 2001
@@ -230,5 +230,9 @@
         }
     }
 
+    apr_pool_cleanup_register((*in)->cntxt, (void *)(*in), file_cleanup,
+                        apr_pool_cleanup_null);
+    apr_pool_cleanup_register((*out)->cntxt, (void *)(*out), file_cleanup,
+                        apr_pool_cleanup_null);
     return APR_SUCCESS;
 }

====== END PATCH ======

The second issue is that read_with_timeout in apr/file_io/win32/readwrite.c
incorrectly returns APR_SUCCESS instead of APR_EOF when PeekNamedPipe fails
and the result from GetLastError() is ERROR_BROKEN_PIPE. Because of this,
the pipe isn't closed as soon as it could be. Combined with the lack of
cleanup code above, the server fails to close one end of the pipe (and quite
often, an event in the OVERLAPPED structure) and leaks resources over time. 

The relevant quote from MSDN (from the ReadFile doc): "If the anonymous
write pipe handle has been closed and ReadFile attempts to read using the
corresponding anonymous read pipe handle, the function returns FALSE and
GetLastError returns ERROR_BROKEN_PIPE. " PeekNamedPipe operates similarly,
and it applies to both named and anonymous pipes. 

===== BEGIN PATCH =====

--- readwrite.c	Tue Aug 28 01:56:09 2001
+++ apr/file_io/win32/readwrite.c	Fri Oct 12 01:43:17 2001
@@ -89,7 +89,7 @@
                                &dwBytesLeftThisMsg)) { // pointer to unread
bytes in this message
                 rv = apr_get_os_error();
                 if (rv == APR_FROM_OS_ERROR(ERROR_BROKEN_PIPE))
-                    return APR_SUCCESS;
+                    return APR_EOF;
             }
             else {
                 if (dwBytesRead == 0) {

====== END PATCH ======

I've tested these patches and they eliminate the resource leaks that I'd
noticed with Apache 2.0.25. 

Tim

Re: [PATCH] Pipes on Win32 (fix resource leaks)

[Jeff Trawick <tr...@attglobal.net>]

"Costello, Tim" <ti...@zcmgroup.com.au> writes:

> Hi,
> 
> I hope this is the right list/forum for this message - apologies in advance
> if it isn't. I'm writing to submit two patches against the current CVS tree
> for APR to fix a couple of resource leaks. 

This is definitely the right list.  I'll commit these patches Real
Soon Now.  Thanks!

-- 
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
       http://www.geocities.com/SiliconValley/Park/9289/
             Born in Roswell... married an alien...