Re: Fix Win32 file handle leak in translate-test.exe?

kfogel@collab.net wrote:

>Branko Čibej <brane@xbc.nu> writes:
>
>
>>Good point. In the meantime I found (and fixed) a subtle memory usage
>>bug in my patch, *sigh*. I'll commit to trunk as soon as I've run some
>>tests.
>>
>>
>
>Might as well post here for more review, while waiting?...
>
>
>
Sure.

[[[
Fix a handle leak that caused translate-test to fail on Windows because
an open file cannot be removed there.

* subversion/libsvn_subr/subst.c (svn_subst_copy_and_translate): Use a
  subpool to open all files and streams and destroy it to make sure
  that all file handles are closed before returning.
]]]

Index: subversion/libsvn_subr/subst.c
===================================================================
--- subversion/libsvn_subr/subst.c (revision 8044)
+++ subversion/libsvn_subr/subst.c (working copy)
@@ -41,6 +41,7 @@
 #include "svn_io.h"
 #include "svn_hash.h"
 #include "svn_subst.h"
+#include "svn_pools.h"
 
 void
 svn_subst_eol_style_from_value (svn_subst_eol_style_t *style,
@@ -707,33 +708,44 @@
                               svn_boolean_t expand,
                               apr_pool_t *pool)
 {
- const char *dst_tmp;
+ const char *dst_tmp = NULL;
   svn_stream_t *src_stream, *dst_stream;
   apr_file_t *s = NULL, *d = NULL; /* init to null important for APR */
   svn_error_t *err;
+ apr_pool_t *subpool;
 
   /* The easy way out: no translation needed, just copy. */
   if (! (eol_str || keywords))
     return svn_io_copy_file (src, dst, FALSE, pool);
 
+ subpool = svn_pool_create (pool);
+
   /* Open source file. */
- SVN_ERR (svn_io_file_open (&s, src, APR_READ | APR_BUFFERED,
- APR_OS_DEFAULT, pool));
+ err = svn_io_file_open (&s, src, APR_READ | APR_BUFFERED,
+ APR_OS_DEFAULT, subpool);
+ if (err)
+ goto error;
 
   /* For atomicity, we translate to a tmp file and
      then rename the tmp file over the real destination. */
 
- SVN_ERR (svn_io_open_unique_file (&d, &dst_tmp, dst,
- ".tmp", FALSE, pool));
+ err = svn_io_open_unique_file (&d, &dst_tmp, dst,
+ ".tmp", FALSE, subpool);
 
+ /* Move the file name to a more permanent pool. */
+ if (dst_tmp)
+ dst_tmp = apr_pstrdup(pool, dst_tmp);
+
+ if (err)
+ goto error;
+
   /* Now convert our two open files into streams. */
- src_stream = svn_stream_from_aprfile (s, pool);
- dst_stream = svn_stream_from_aprfile (d, pool);
+ src_stream = svn_stream_from_aprfile (s, subpool);
+ dst_stream = svn_stream_from_aprfile (d, subpool);
 
   /* Translate src stream into dst stream. */
   err = svn_subst_translate_stream (src_stream, dst_stream,
                                     eol_str, repair, keywords, expand);
-
   if (err)
     goto error;
 
@@ -746,23 +758,26 @@
   if (err)
     goto error;
 
- err = svn_io_file_close (s, pool);
+ err = svn_io_file_close (s, subpool);
   if (err)
     goto error;
 
- err = svn_io_file_close (d, pool);
+ err = svn_io_file_close (d, subpool);
   if (err)
- goto error;
+ goto error;
 
   /* Now that dst_tmp contains the translated data, do the atomic rename. */
- err = svn_io_file_rename (dst_tmp, dst, pool);
+ err = svn_io_file_rename (dst_tmp, dst, subpool);
   if (err)
     goto error;
 
+ svn_pool_destroy (subpool);
   return SVN_NO_ERROR;
+
  error:
- svn_error_clear (svn_io_remove_file (dst_tmp, pool));
-
+ svn_pool_destroy (subpool); /* Make sure all files are closed first. */
+ if (dst_tmp)
+ svn_error_clear (svn_io_remove_file (dst_tmp, pool));
   return err;
 }
 

-- 
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