Re: svn commit: rev 7978 - trunk/subversion/libsvn_subr

[kf...@collab.net]

dionisos@tigris.org writes:
> Log:
> Fix possible leaking of temporary file.
> 
> * subversion/libsvn_subr/subst.c (svn_subst_copy_and_translate): Eliminate
>   possible leaking of temporary file.

I understand the spirit of this change, but are you sure you did it
correctly?

For example:

> Modified: trunk/subversion/libsvn_subr/subst.c
> ==============================================================================
> --- trunk/subversion/libsvn_subr/subst.c	(original)
> +++ trunk/subversion/libsvn_subr/subst.c	Thu Dec 11 13:16:39 2003
> @@ -735,33 +735,35 @@
>                                      eol_str, repair, keywords, expand);
>  
>    if (err)
> -    {
> -      /* ignore closure errors if we're bailing. */
> -      svn_error_clear (svn_stream_close (src_stream));
> -      svn_error_clear (svn_stream_close (dst_stream));
> -      if (s)
> -        apr_file_close (s);
> -      if (d)
> -        apr_file_close (d);
> -
> -      svn_error_clear (svn_io_remove_file (dst_tmp, pool));
> -      return
> -        svn_error_createf (err->apr_err, err,
> -                           "File translation failed when copying '%s' to '%s'",
> -                           src, dst);
> -    }
> +    goto error;

The specific error message disappears here, but does not reappear
anywhere else.  It has been lost.  

Also, the old code tested s and d before closing them; the new code
does not (I followed the code path down to do_io_file_wrapper_cleanup
in io.c, it just assumes that the file is not null).  Is this safe?

-Karl, beginning to think Greg Stein is right about being stricter on
       all trunk commits starting now...

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 7978 - trunk/subversion/libsvn_subr

[kf...@collab.net]

"Erik Huelsmann" <e....@gmx.net> writes:
> > -Karl, beginning to think Greg Stein is right about being stricter on
> >        all trunk commits starting now...
> 
> I'll start sending my patches to the list. (which I did for this one BTW.)
> And I'll start waiting for votes. (which I didn't do longer than 24h -
> presumably too short as you are all so busy)

Oh, just use your judgement.  Sorry if I came off harshly.

(This discussion won't matter soon, since we're branching 0.35
tomorrow, and that will be what 1.0-stabilization branches from a week
later.  Yay!)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 7978 - trunk/subversion/libsvn_subr

[Erik Huelsmann <e....@gmx.net>]

> I understand the spirit of this change, but are you sure you did it
> correctly?
> 
> For example:
> 
> > Modified: trunk/subversion/libsvn_subr/subst.c
> >
>
==============================================================================
> > --- trunk/subversion/libsvn_subr/subst.c	(original)
> > +++ trunk/subversion/libsvn_subr/subst.c	Thu Dec 11 13:16:39 2003
> > @@ -735,33 +735,35 @@
> >                                      eol_str, repair, keywords, expand);
> >  
> >    if (err)
> > -    {
> > -      /* ignore closure errors if we're bailing. */
> > -      svn_error_clear (svn_stream_close (src_stream));
> > -      svn_error_clear (svn_stream_close (dst_stream));
> > -      if (s)
> > -        apr_file_close (s);
> > -      if (d)
> > -        apr_file_close (d);
> > -
> > -      svn_error_clear (svn_io_remove_file (dst_tmp, pool));
> > -      return
> > -        svn_error_createf (err->apr_err, err,
> > -                           "File translation failed when copying '%s'
> to '%s'",
> > -                           src, dst);
> > -    }
> > +    goto error;
> 
> The specific error message disappears here, but does not reappear
> anywhere else.  It has been lost.  
Yes, but the svn_io wrapper in io.c added an error message which describes
what actually went wrong. If there was not enough disc space to do the
translation, that information would have been lost before this patch.
 
> Also, the old code tested s and d before closing them; the new code
> does not (I followed the code path down to do_io_file_wrapper_cleanup
> in io.c, it just assumes that the file is not null).  Is this safe?
In case there is an error SVN_ERR () prevents the code path from ever
reaching the point where d or s is closed. Even if the pointer were NULL
(hypothetical case!), having a successfull open would necessarily mean that it is
closeable (and close should be able to handle that condition).

Looking at the code in apr_file_open, it cannot complete successfully
without setting d or s to a non-NULL value. (Checked it in case the above does not
ease you.)


> -Karl, beginning to think Greg Stein is right about being stricter on
>        all trunk commits starting now...

I'll start sending my patches to the list. (which I did for this one BTW.)
And I'll start waiting for votes. (which I didn't do longer than 24h -
presumably too short as you are all so busy)

bye,

Erik.

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