You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by John Szakmeister <jo...@szakmeister.net> on 2003/11/30 00:44:14 UTC
Problem in copy_versioned_files()?
I was looking through export.c a little more, hoping that I could factor some
stuff together and fix Issue 1361 in the process.
I see the following happen in copy_versioned_files():
SVN_ERR (svn_wc_adm_probe_open (&adm_access, NULL, from, FALSE,
FALSE, pool));
err = svn_wc_entry (&entry, from, adm_access, FALSE, subpool);
SVN_ERR (svn_wc_adm_close (adm_access));
if (err)
{
if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
return err;
else
svn_error_clear (err);
}
and then once we determine there is a file to copy, this occurs:
err = svn_wc_entry (&entry, copy_from, adm_access, FALSE,
subpool);
Is this safe? According to svn_wc_adm_close's docstring, "Any physical locks
will be removed from the working copy. Lock removal is unconditional, there
is no check to determine if cleanup is required." This sounds to me like we
shouldn't be accessing the entry information after we've closed the WC.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by John Szakmeister <jo...@szakmeister.net>.
On Saturday 06 December 2003 16:20, Julian Foad wrote:
[snip]
>
> OK. I might have another look later but I'm doing other things at the
> moment.
>
No problem. I'm still hacking at it, but the rest of my weekend is going to
be shot traveling, so I wanted to get what I had back out to the list.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
> John Szakmeister wrote:
>
>> On Saturday 06 December 2003 21:21, Julian Foad wrote:
>>
> [...]
>
>>> /* 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.)
>>
>> Good idea!
>>
>>> 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"?
>>
>> Oof, good question. I personally like it because it forces us as
>> developers to adhere to the API. I'm not sure it useful outside of
>> that though. I'm in favor of the memset() since if the baton changes
>> for some reason, it's less code to update.
>
> Maybe it (and other debug-only things like the "file" and "line" fields
> of svn_error) should be wrapped in "#ifdef DEBUG" or similar.
>
> I've committed it as it is, in r7961.
... and reverted it in r7966 due to Philip Martin's objections (with which I agree). See thread "svn commit: rev 7961 - trunk/subversion/libsvn_wc".
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by Julian Foad <ju...@btopenworld.com>.
John Szakmeister wrote:
> On Saturday 06 December 2003 21:21, Julian Foad wrote:
>
[...]
>> /* 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.)
>
> Good idea!
>
>>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"?
>
> Oof, good question. I personally like it because it forces us as developers
> to adhere to the API. I'm not sure it useful outside of that though. I'm in
> favor of the memset() since if the baton changes for some reason, it's less
> code to update.
Maybe it (and other debug-only things like the "file" and "line" fields of svn_error) should be wrapped in "#ifdef DEBUG" or similar.
I've committed it as it is, in r7961.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by John Szakmeister <jo...@szakmeister.net>.
On Saturday 06 December 2003 21:21, Julian Foad wrote:
> 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().
The problem was that it was acting on the the committed item first before
acting on it's children. So, I figured we should change the approach and
take care of the children first, then we can act on the item and not have to
worry about whether or not it closes adm_access.
> 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.)
Good idea!
> 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"?
Oof, good question. I personally like it because it forces us as developers
to adhere to the API. I'm not sure it useful outside of that though. I'm in
favor of the memset() since if the baton changes for some reason, it's less
code to update.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by Julian Foad <ju...@btopenworld.com>.
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
Re: Problem in copy_versioned_files()?
Posted by Julian Foad <ju...@btopenworld.com>.
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?
>>
>>- Julian
>
> 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
Ah - perhaps I should have mentioned that only copy test #9 was failing when I got as far as I got. :-)
> 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;
>
> Maybe someone else on the list will see the easy solution to this problem.
> I'll keep looking at it, but the answer isn't apparent to me at the moment.
OK. I might have another look later but I'm doing other things at the moment.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by John Szakmeister <jo...@szakmeister.net>.
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?
>
> - Julian
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;
Maybe someone else on the list will see the easy solution to this problem.
I'll keep looking at it, but the answer isn't apparent to me at the moment.
-John
Re: Problem in copy_versioned_files()?
Posted by John Szakmeister <jo...@szakmeister.net>.
On Tuesday 02 December 2003 22:20, Julian Foad wrote:
> John Szakmeister wrote:
> > On Tuesday 02 December 2003 17:45, Julian Foad wrote:
> >>I am also playing with the following patch:
> >>
> >>Index: subversion/libsvn_wc/lock.c
> >>===================================================================
> >>--- subversion/libsvn_wc/lock.c (revision 7899)
> >>+++ 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;
> >> }
> >>
> >>
> >>It catches the error that is the subject of this message, but makes lots
> >> of other tests fail too. I'm still investigating whether it is finding
> >> real bugs or is a wrong thing to do.
> >
> > Interesting that this caused other errors. I'm curious to know what you
> > find! :-)
>
> 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?
Nice work!
Sure, why not? :-) I started looking at the problem, and actually tried
several different solutions with no success. I was expecting different
errors than what I got. Some of the tests were bailing out, but didn't leave
a trace of what went wrong when I moved the run_log command to be after the
recurse portion. There was no error output, but some tests bailed out right
in the middle of the commit.
If I can't get this over the next couple of days, I'll push it back out to the
list for someone else to try.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by Julian Foad <ju...@btopenworld.com>.
John Szakmeister wrote:
> On Tuesday 02 December 2003 17:45, Julian Foad wrote:
>
>>I am also playing with the following patch:
>>
>>Index: subversion/libsvn_wc/lock.c
>>===================================================================
>>--- subversion/libsvn_wc/lock.c (revision 7899)
>>+++ 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;
>> }
>>
>>
>>It catches the error that is the subject of this message, but makes lots of
>>other tests fail too. I'm still investigating whether it is finding real
>>bugs or is a wrong thing to do.
>
> Interesting that this caused other errors. I'm curious to know what you
> find! :-)
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?
- Julian
Re: Problem in copy_versioned_files()?
Posted by John Szakmeister <jo...@szakmeister.net>.
On Tuesday 02 December 2003 17:45, Julian Foad wrote:
> [snip]
> Is my patch that I have just sent in thread "Sub-pools for recursion" OK
> for this purpose? It also fixes the handling of errors which, in the
> original code above, is wrong in the case where svn_wc_adm_close returns an
> error.
Yeah, I saw that... it was bugging me that the there was a potential for a
memory leak. :-) I looked at your other patch, and it seemed to address the
problems just fine. +1 by me.
> I considered putting an "svn_wc_adm_close" in the error return path ("if
> (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)"), but I don't think that's
> necessary.
That is why I haven't done much with that yet. I was trying to think ahead a
little and was thinking about the fact that maybe semantics would change a
little. Like reads of the entries might actually take out a lock. In that
case, it wouldn't be removed, causing the user to have to do an 'svn cleanup'
if we didn't have svn_wc_adm_close() in the error path. I'm probably just
being paranoid though. :-)
> I am also playing with the following patch:
>
> Index: subversion/libsvn_wc/lock.c
> ===================================================================
> --- subversion/libsvn_wc/lock.c (revision 7899)
> +++ 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;
> }
>
>
> It catches the error that is the subject of this message, but makes lots of
> other tests fail too. I'm still investigating whether it is finding real
> bugs or is a wrong thing to do.
Interesting that this caused other errors. I'm curious to know what you
find! :-)
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by Julian Foad <ju...@btopenworld.com>.
John Szakmeister wrote:
> On Sunday 30 November 2003 12:55, Philip Martin wrote:
>
>>> err = svn_wc_entry (&entry, from, adm_access, FALSE, subpool);
>>> SVN_ERR (svn_wc_adm_close (adm_access));
>>> if (err)
>>> {
>>> if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
>>> return err;
>>> else
>>> svn_error_clear (err);
>>> }
[...]
>>> err = svn_wc_entry (&entry, copy_from, adm_access, FALSE,
>>> subpool);
>>>
>>>Is this safe? According to svn_wc_adm_close's docstring, "Any physical
>>>locks will be removed from the working copy. Lock removal is
>>>unconditional, there is no check to determine if cleanup is required."
>>>This sounds to me like we shouldn't be accessing the entry information
>>>after we've closed the WC.
>>
>>With the current code the entry information will remain until the pool
>>is cleared. I'm not sure that's something we want to guarantee, but I
>>suspect it's not going to change. It probably would be better if we
>>didn't access entry information after a baton is closed.
>
> I figured it would sit in the pool (otherwise I would have seen problems when
> the local export stuff), but it seems that this function is making use of an
> implementation detail that isn't explicitly documented, which is why I had
> cause for concern.
>
> I'll look at getting this all squared away so that we no longer do that in
> this function.
Is my patch that I have just sent in thread "Sub-pools for recursion" OK for this purpose? It also fixes the handling of errors which, in the original code above, is wrong in the case where svn_wc_adm_close returns an error.
I considered putting an "svn_wc_adm_close" in the error return path ("if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)"), but I don't think that's necessary.
I am also playing with the following patch:
Index: subversion/libsvn_wc/lock.c
===================================================================
--- subversion/libsvn_wc/lock.c (revision 7899)
+++ 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;
}
It catches the error that is the subject of this message, but makes lots of other tests fail too. I'm still investigating whether it is finding real bugs or is a wrong thing to do.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by John Szakmeister <jo...@szakmeister.net>.
On Sunday 30 November 2003 12:55, Philip Martin wrote:
> John Szakmeister <jo...@szakmeister.net> writes:
> > I see the following happen in copy_versioned_files():
> >
> > SVN_ERR (svn_wc_adm_probe_open (&adm_access, NULL, from, FALSE,
> > FALSE, pool));
>
> This is a read-only baton so it doesn't create any locks.
Fair enough.
> > err = svn_wc_entry (&entry, from, adm_access, FALSE, subpool);
> > SVN_ERR (svn_wc_adm_close (adm_access));
> > if (err)
> > {
> > if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
> > return err;
> > else
> > svn_error_clear (err);
> > }
> >
> > and then once we determine there is a file to copy, this occurs:
> >
> > err = svn_wc_entry (&entry, copy_from, adm_access, FALSE,
> > subpool);
> >
> > Is this safe? According to svn_wc_adm_close's docstring, "Any physical
> > locks will be removed from the working copy. Lock removal is
> > unconditional, there is no check to determine if cleanup is required."
> > This sounds to me like we shouldn't be accessing the entry information
> > after we've closed the WC.
>
> With the current code the entry information will remain until the pool
> is cleared. I'm not sure that's something we want to guarantee, but I
> suspect it's not going to change. It probably would be better if we
> didn't access entry information after a baton is closed.
I figured it would sit in the pool (otherwise I would have seen problems when
the local export stuff), but it seems that this function is making use of an
implementation detail that isn't explicitly documented, which is why I had
cause for concern.
I'll look at getting this all squared away so that we no longer do that in
this function.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by Philip Martin <ph...@codematters.co.uk>.
John Szakmeister <jo...@szakmeister.net> writes:
> I see the following happen in copy_versioned_files():
>
> SVN_ERR (svn_wc_adm_probe_open (&adm_access, NULL, from, FALSE,
> FALSE, pool));
This is a read-only baton so it doesn't create any locks.
> err = svn_wc_entry (&entry, from, adm_access, FALSE, subpool);
> SVN_ERR (svn_wc_adm_close (adm_access));
> if (err)
> {
> if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
> return err;
> else
> svn_error_clear (err);
> }
>
> and then once we determine there is a file to copy, this occurs:
>
> err = svn_wc_entry (&entry, copy_from, adm_access, FALSE,
> subpool);
>
> Is this safe? According to svn_wc_adm_close's docstring, "Any physical locks
> will be removed from the working copy. Lock removal is unconditional, there
> is no check to determine if cleanup is required." This sounds to me like we
> shouldn't be accessing the entry information after we've closed the WC.
With the current code the entry information will remain until the pool
is cleared. I'm not sure that's something we want to guarantee, but I
suspect it's not going to change. It probably would be better if we
didn't access entry information after a baton is closed.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Problem in copy_versioned_files()?
Posted by Julian Foad <ju...@btopenworld.com>.
John Szakmeister wrote:
>
> SVN_ERR (svn_wc_adm_probe_open (&adm_access, NULL, from, FALSE,
> FALSE, pool));
> err = svn_wc_entry (&entry, from, adm_access, FALSE, subpool);
> SVN_ERR (svn_wc_adm_close (adm_access));
[...]
> err = svn_wc_entry (&entry, copy_from, adm_access, FALSE,
> subpool);
>
> Is this safe? [...]
If using adm_access after closing it is indeed an error, and it looks to me like it is, then can we make the software catch these errors automatically by putting some code into svn_wc_adm_close (and other ..._close functions for other resources) to invalidate the resource baton? This code need only be active in maintainer mode (debug mode).
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org