julianfoad@tigris.org writes:
> Author: julianfoad
> Date: Mon Dec 8 14:51:27 2003
> New Revision: 7961
>
> Modified:
> trunk/subversion/libsvn_wc/adm_ops.c
> trunk/subversion/libsvn_wc/lock.c
> trunk/subversion/libsvn_wc/log.c
> Log:
> Avoid using an adm_access baton after it has been closed.
>
> * subversion/libsvn_wc/log.c (svn_wc__run_log):
> * subversion/libsvn_wc/adm_ops.c (svn_wc_process_committed,
> svn_wc_remove_from_revision_control):
> Do not use adm_access after it has been closed.
>
> * subversion/libsvn_wc/lock.c
> (do_close): Invalidate the baton so that mis-use may be caught.
Yuck! I don't like this change.
Using memset to "invalidate" the access baton is a hack, and it
doesn't even work. If we really want to do this, it would be better
to use some sort of positive check rather than relying on memory
poisoning (e.g. closing a write lock sets the lock type to "closed",
perhaps this could be extended to read locks.)
> Modified: trunk/subversion/libsvn_wc/adm_ops.c
> ==============================================================================
> --- trunk/subversion/libsvn_wc/adm_ops.c (original)
> +++ trunk/subversion/libsvn_wc/adm_ops.c Mon Dec 8 14:51:27 2003
> @@ -393,8 +393,9 @@
>
> /* Run the log file we just created. */
> SVN_ERR (svn_wc__run_log (adm_access, NULL, pool));
> -
> - if (recurse)
> +
> + /* Recurse if required, unless we have just removed the entire directory. */
> + if (recurse && svn_wc_adm_locked (adm_access))
This call of svn_wc_adm_locked is using an access baton after it has
been closed, the very thing you are trying to prevent! The memset
doesn't catch it, since the zero it produces looks like an unlocked
access baton.
> {
> apr_hash_t *entries;
> apr_hash_index_t *hi;
> @@ -1721,6 +1722,7 @@
> apr_pool_t *subpool = svn_pool_create (pool);
> apr_hash_index_t *hi;
> svn_wc_entry_t incomplete_entry;
> + const char *adm_access_path = svn_wc_adm_access_path (adm_access);
This is storing a pointer to memory allocated by the access baton...
>
> /* ### sanity check: check 2 places for DELETED flag? */
>
> @@ -1861,8 +1863,7 @@
> *non*-recursive dir_remove should work. If it doesn't,
> no big deal. Just assume there are unversioned items in
> there and set "left_something" */
> - err = svn_io_dir_remove_nonrecursive
> - (svn_wc_adm_access_path (adm_access), subpool);
> + err = svn_io_dir_remove_nonrecursive (adm_access_path, subpool);
...and here that memory is accessed after the baton is closed.
Now the memory associated with the access baton remains allocated
until the pool is cleared. If using the access baton is wrong, why is
using memory allocated by the access baton OK? If using the memory is
OK why should I not be able to use the access baton to get it? Can
you explain the constraints you are attempting to enforce?
Access batons with write locks can not be used to modify the entries
file after they have been closed. Prior to this commit, the validity
of memory associated with an access baton followed the usual rules for
pools. I'm not convinced that adding different memory validity rules
for access baton memory is an improvement, so I think this commit
should be reverted.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Dec 8 23:41:18 2003