Philip Martin wrote:
> 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,
Agreed it's a hack. It is a valid technique when used in the "free" or "close" path of an object manager that does not know the structure of the objects that it manages but in this case we're at a higher level where we do know the structure. I should have done something better, or just used that for experimentation but not committed it.
> and it doesn't even work.
In what way does it not work? In that it doesn't catch _all_ attempts to access it? I know that; the idea was to catch _most_ attempts to use it, or even _some_ attempts.
> 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.)
OK. I'll revert this part right away and then think about a better solution (if we want one at all) for later.
>>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.
Oh, darn. That was stupid of me. Half of my brain analysed the way that the "memset" was affecting it, but I didn't notice the conceptual contradiction. Thanks for catching that ... and the stuff below.
>>@@ -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...
>>@@ -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?
Using that memory was not OK in the absence of any interface promises regarding it. I should have made a duplicate of that string so that it (the duplicate) is definitely not associated with the adm_access baton.
> Can you explain the constraints you are attempting to enforce?
The principle was:
Do not use an adm_access baton after it has been closed.
"Use" means, for example, dereference or pass to another function. I think I would now include the path string and any resource that was owned by the adm_access baton and not explicitly disowned by it.
The reason for trying to enforce that principle (both by trying to fix some current violations and by trying to add automatic detection) is, of course, that use of any resource after it is closed is hard to detect in other ways and is often a source of subtle bugs.
> 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.
OK. You have convinced me that this change was pretty poor and should be reverted. I will try to create a better version that addresses the concerns, as I believe the principle is sound. (Of course you may dispute the principle, too, if you wish.)
Reverted in r7966.
Apologies for the badness.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 9 02:53:51 2003