Blame temporary file leak on error

Now that the svn_subst_copy_and_translate issue is all cleared up. I started
looking for other cases where we don't leak handles, but possibly leak
(closed) temporary files.

The only case I could find is in subversion/libsvn_client/blame.c which
leaks a temporary file (on disk) when an error occurs between the start of the
first loop and function completion.

I tried solving this issue the same as Brane solved the issue in
libsvn_subr/subst.c: by using goto's and the correct number of pools. By introducing all
these if (err) goto error's the function got twice as long as it was before.
Brane suggested (before publishing his fix) that pool cleanup can be used to
close and remove files.

So I suggest application of the patch below to solve this issue. I also
changed the allocation of the unique temporary file from the main pool to one of
the sub pools. This means that at each iteration (very) old files get cleaned
up, whereas the one required file from the last iteration is still in
memory. Please I don't want to get on anybody's nerves again, so just tell me to
stop waisting your time until 1.0 has been released if that's applicable.

bye,

Erik.

Log:
[[[
Fix leak of temporary file on error.

* subversion/libsvn_client/blame.c
   (temporary_file_cleanup): New. Pool cleanup function to remove temporary
files.
   (svn_client_blame): Register pool cleanup for removal of unremoved
temporary files and
   introduce second iteration pool for cleaning up old files early.

]]]

Index: subversion/libsvn_client/blame.c
===================================================================
--- subversion/libsvn_client/blame.c (revision 8035)
+++ subversion/libsvn_client/blame.c (working copy)
@@ -317,6 +317,23 @@
                             rev->revision, rev->path);
 }
  
+static apr_status_t
+temporary_file_cleanup (void *f)
+{
+ apr_file_t *file = f;
+ apr_status_t apr_err;
+ const char *fname;
+
+ /* the file may or may not have been closed; try it */
+ apr_file_close (file);
+
+ apr_err = apr_file_name_get (&fname, file);
+ if (! apr_err)
+ (void) apr_file_remove (apr_file_pool_get (file), fname);
+
+ return apr_err;
+}
+
 svn_error_t *
 svn_client_blame (const char *target,
                   const svn_opt_revision_t *start,
@@ -337,6 +354,8 @@
   apr_file_t *file;
   svn_stream_t *stream;
   apr_pool_t *iterpool;
+ apr_pool_t *iterpools[2];
+ int current_iterpool;
   struct rev *rev;
   apr_status_t apr_err;
   svn_node_kind_t kind;
@@ -348,8 +367,12 @@
     return svn_error_create
       (SVN_ERR_CLIENT_BAD_REVISION, NULL, NULL);
  
- iterpool = svn_pool_create (pool);
+ iterpools[0] = svn_pool_create (pool);
+ iterpools[1] = svn_pool_create (pool);
  
+ current_iterpool = 0;
+ iterpool = iterpools[0];
+
   SVN_ERR (svn_client_url_from_path (&url, target, pool));
   if (! url)
     return svn_error_createf (SVN_ERR_ENTRY_MISSING_URL, NULL,
@@ -459,8 +482,15 @@
       apr_pool_clear (iterpool);
       SVN_ERR (svn_io_temp_dir (&temp_dir, pool));
       SVN_ERR (svn_io_open_unique_file (&file, &tmp,
- svn_path_join (temp_dir, "tmp", pool), ".tmp",
- FALSE, pool));
+ svn_path_join (temp_dir, "tmp",
pool),
+ ".tmp", FALSE, iterpool));
+
+ /* Now that we have a valid temp file, make sure it is removed again;
+ assign it to the current iterpool, then it gets cleared *after*
the
+ next iteration */
+ apr_pool_cleanup_register (iterpool, file,
+ temporary_file_cleanup, NULL);
+
       stream = svn_stream_from_aprfile (file, iterpool);
       SVN_ERR (ra_lib->get_file (session, rev->path + 1, rev->revision,
                                  stream, NULL, NULL, iterpool));
@@ -479,6 +509,8 @@
         }
  
       last = tmp;
+ current_iterpool = (current_iterpool) ? 0 : 1;
+ iterpool = iterpools[current_iterpool];
     }
  
   apr_err = apr_file_open (&file, last, APR_READ, APR_OS_DEFAULT, pool);
@@ -512,6 +544,7 @@
   if (apr_err != APR_SUCCESS)
     return svn_error_wrap_apr (apr_err, "Can't remove '%s'", last);
  
- apr_pool_destroy (iterpool);
+ apr_pool_destroy (iterpools[0]);
+ apr_pool_destroy (iterpools[1]);
   return SVN_NO_ERROR;
 }

-- 
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org