Error leak in libsvn_client/revert.c

[Max Bowsher <ma...@ukf.net>]

==========================================================
  err = svn_wc_revert2 (path, adm_access, recursive, use_commit_times,
                        ctx->cancel_func, ctx->cancel_baton,
                        ctx->notify_func2, ctx->notify_baton2,
                        pool);

 /* If no error, or SVN_ERR_UNVERSIONED_RESOURCE error, then we want
     to close up before returning.  For any other kind of error, we
     want to leave things exactly as they were when the error
     occurred. */

  if (err && err->apr_err != SVN_ERR_UNVERSIONED_RESOURCE)
    return err;

  SVN_ERR (svn_wc_adm_close (adm_access));

  return err;
==========================================================

Potential error leak if svn_wc_revert2 errors, and then svn_wc_adm_close 
errors too.

I don't understand that comment about when to close the adm_access and when 
not, so I'm not sure how to fix it correctly.

Max.


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

Re: Error leak in libsvn_client/revert.c

[Philip Martin <ph...@codematters.co.uk>]

"Max Bowsher" <ma...@ukf.net> writes:

> ==========================================================
>   err = svn_wc_revert2 (path, adm_access, recursive, use_commit_times,
>                         ctx->cancel_func, ctx->cancel_baton,
>                         ctx->notify_func2, ctx->notify_baton2,
>                         pool);
>
>  /* If no error, or SVN_ERR_UNVERSIONED_RESOURCE error, then we want
>      to close up before returning.  For any other kind of error, we
>      want to leave things exactly as they were when the error
>      occurred. */
>
>   if (err && err->apr_err != SVN_ERR_UNVERSIONED_RESOURCE)
>     return err;
>
>   SVN_ERR (svn_wc_adm_close (adm_access));
>
>   return err;
> ==========================================================
>
> Potential error leak if svn_wc_revert2 errors, and then
> svn_wc_adm_close errors too.
>
> I don't understand that comment about when to close the adm_access and
> when not, so I'm not sure how to fix it correctly.

I think an SVN_ERR_UNVERSIONED_RESOURCE error is "acceptable" as it
indicates an unversioned item.  Since the error is acceptable it
should not cause the baton to be left open.  So probably something
like:

   if (err && err->apr_err != SVN_ERR_UNVERSIONED_RESOURCE)
     return err;

   err2 = svn_wc_adm_close (adm_access);
   if (err2 && ! err)
     return err2
   svn_error_clear (err2);

   return err;

It might be more elegant to call the notification function in revert
instead of svn_client_revert.

-- 
Philip Martin

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