John Szakmeister wrote:
> On Tuesday 02 December 2003 22:20, Julian Foad wrote:
>
>>It does catch a few other instances of using an adm_access baton after it
>>was closed. The attached patch fixes all but one tricky case, marked with
>>"###". Do you fancy trying to see how to deal with that tricky case?
>
> Well, I spent some time trying to fix the tricky case in
> svn_wc_process_committed(). I've managed to get nearly everything working
> with the attached patch. Only copy test #9 and svnlook test #2 fail and I
> think for the same reason. It appears that this line is no longer filtering
> out the already committed file "A/B2/E/alpha" in svn_client_commit(), and
> tries to run svn_wc_process_committed() on it again, causing it to fail.
>
> if (! entry
> && have_processed_parent (commit_items, i, item->path, subpool))
> /* This happens when the item is a file that is deleted, and it
> has been processed as a child of an earlier item. */
> continue;
Hmm, I haven't seen a problem with this bit of code. I will look for the effect you describe later, but in the meantime I've just concentrated on the body of svn_wc_process_committed().
I see in your version of svn_wc_process_committed that you swap the order of processing and recursing. I'm afraid I am not competent to comment on that. I have had another look and I have a simple idea that works: don't recurse if the adm_access baton has been closed.
/* Recurse if required, unless we have just removed the entire directory. */
if (recurse && svn_wc_adm_locked (adm_access))
This works because if the directory was removed, svn_wc_remove_from_revision_control->svn_wc__adm_destroy->svn_wc_adm_close will have marked the access baton as not having a write lock. (Actually, the status is explicitly set to "closed" but my zeroing it out interferes with that and changes the status to "unlocked". Either way is sufficient for this to work, but it is unclean.)
I think this patch now fixes the used-after-closed errors: all tests pass on Linux over RA-local and RA-svn. The only question I have is whether we want the baton-invalidation step to be checked in. If so, should it set various fields individually instead of doing a "memset"?
(I have removed the couple of "assert" statements from the end of my patch. They were just randomly placed debugging aids, not carefully chosen.)
- Julian
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.
Index: subversion/libsvn_wc/log.c
===================================================================
--- subversion/libsvn_wc/log.c (revision 7945)
+++ subversion/libsvn_wc/log.c (working copy)
@@ -1279,6 +1279,16 @@
{
const svn_wc_entry_t *thisdir_entry, *parent_entry;
svn_wc_entry_t tmp_entry;
+ const char *parent, *bname;
+ svn_wc_adm_access_t *parent_access;
+
+ svn_path_split (svn_wc_adm_access_path (adm_access), &parent,
+ &bname, pool);
+ SVN_ERR (svn_wc_adm_retrieve (&parent_access, adm_access, parent,
+ pool));
+ SVN_ERR (svn_wc_entry (&parent_entry, parent, parent_access, FALSE,
+ pool));
+
SVN_ERR (svn_wc_entry (&thisdir_entry,
svn_wc_adm_access_path (adm_access), adm_access,
FALSE, pool));
@@ -1294,29 +1304,17 @@
/* If revnum of this dir is greater than parent's revnum, then
recreate 'deleted' entry in parent. */
- {
- const char *parent, *bname;
- svn_wc_adm_access_t *parent_access;
-
- svn_path_split (svn_wc_adm_access_path (adm_access), &parent,
- &bname, pool);
- SVN_ERR (svn_wc_adm_retrieve (&parent_access, adm_access, parent,
- pool));
- SVN_ERR (svn_wc_entry (&parent_entry, parent, parent_access, FALSE,
- pool));
-
- if (thisdir_entry->revision > parent_entry->revision)
- {
- tmp_entry.kind = svn_node_dir;
- tmp_entry.deleted = TRUE;
- tmp_entry.revision = thisdir_entry->revision;
- SVN_ERR (svn_wc__entry_modify (parent_access, bname, &tmp_entry,
- SVN_WC__ENTRY_MODIFY_REVISION
- | SVN_WC__ENTRY_MODIFY_KIND
- | SVN_WC__ENTRY_MODIFY_DELETED,
- TRUE, pool));
- }
- }
+ if (thisdir_entry->revision > parent_entry->revision)
+ {
+ tmp_entry.kind = svn_node_dir;
+ tmp_entry.deleted = TRUE;
+ tmp_entry.revision = thisdir_entry->revision;
+ SVN_ERR (svn_wc__entry_modify (parent_access, bname, &tmp_entry,
+ SVN_WC__ENTRY_MODIFY_REVISION
+ | SVN_WC__ENTRY_MODIFY_KIND
+ | SVN_WC__ENTRY_MODIFY_DELETED,
+ TRUE, pool));
+ }
}
else
{
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 7945)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -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))
{
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);
/* ### 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);
if (err)
{
left_something = TRUE;
Index: subversion/libsvn_wc/lock.c
===================================================================
--- subversion/libsvn_wc/lock.c (revision 7945)
+++ subversion/libsvn_wc/lock.c (working copy)
@@ -738,6 +738,9 @@
assert (! adm_access->set_owner || apr_hash_count (adm_access->set) == 0);
}
+ /* Invalidate the baton so that attempts to use it will be caught. */
+ memset (adm_access, 0, sizeof(*adm_access));
+
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Dec 7 03:16:21 2003