You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Collins-Sussman <su...@collab.net> on 2005/05/10 13:39:23 UTC
dav autoversioning bug... not good.
I think there's a latent bug in DAV autoversioning, one I hope to
isolate and fix today. But I thought I'd alert more people to it...
more eyes to see it, etc.
Refresher:
In the DAV universe, locks have a <DAV:owner> field, which actually
has nothing to do with authn or authz; it's just a comment that the
client expects the server to preserve. In fact, mod_dav_svn maps
<DAV:owner> to svn_lock_t->comment.
The problem with this is that if you do a lock with an svn client
over ra_dav, a bunch of XML junk ends up being stored in svn_lock_t-
>comment (due to mod_dav's design), which violates our principle of
any RA layer leaving a nasty footprint behind.
So our previous solution was to make mod_dav_svn notice whether the
incoming LOCK request is coming from svn, or from a generic DAV
client. In the former case, we strip the XML junk off of the comment
and store the lock. In the latter case, we leave the XML junk in
place, but set a flag reminding us it's there.
When retrieving locks, we look at the flag. If not present, we re-
add proper XML wrapping so as to create a compliant response. If the
flag is set, we do nothing, because the XML wrapping is already there.
All of this code is in mod_dav_svn/lock.c, functions
svn_lock_to_dav_lock() and dav_lock_to_svn_lock().
Bug:
I'm looking at a Dreamweaver http session, and it's the first DAV
client I've seen which sets the <DAV:owner> field. It's setting the
value to "<DAV:href>blah</DAV:href>". Later on, when it does a
PROPFIND to retrieve the lock, the value isn't coming back identical
-- it's been accidentally xml-unescaped. (i.e.
"<DAV:href>blah...") Dreameweaver then chokes and refuses to
interoperate with mod_dav_svn.
Impact of Bug:
I think that any generic DAV client which sets the <DAV:owner> field
is likely to fail against mod_dav_svn. This is serious enough that I
might argue it's worth rolling an rc4 tarball for, delaying the final
release by a day or two. After all, autoversioning is supposed to be
one of the big new important features in 1.2.
Reproducing:
Does anyone know a free DAV client that allows me to set <DAV:owner>
in a LOCK request? Cadaver won't let me do it, and I don't think
Windows Webfolders or OSX does it either. Maybe I'll just do a
'netcat' simulation.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: dav autoversioning bug... not good.
Posted by Ben Collins-Sussman <su...@collab.net>.
On May 10, 2005, at 12:49 PM, C. Michael Pilato wrote:
> So, Ben, Fitz and I dove into this, and saw that the is_xml_comment
> flag in the lock was being clobbered.
Bug is now fixed. I can confirm this via hand-tests, by pasting LOCK
and subsequent PROPFIND requests into netcat, simulating what a
generic dav client (like dreamweaver) does.
The fix is very simple: propogate the svn_lock_t boolean field into
the public svn_fs_lock() API. Of course the patch *looks* huge,
touching 16 files due to the API change... but the actual amount of
logical code-churn is close to zero. It's really easy to review.
As soon as 'make check' passes over ra_dav, I'll commit the change
and nominate for 1.2.x. As I said before, I think this bug is a
showstopper for the 1.2 autoversioning feature, and warrants an rc4
release. (Maybe breser can roll rc4 later this evening?)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: dav autoversioning bug... not good.
Posted by "C. Michael Pilato" <cm...@collab.net>.
So, Ben, Fitz and I dove into this, and saw that the is_xml_comment
flag in the lock was being clobbered. Then a light bulb went on in my
head. When I dropped svn_fs_attach_lock() in favor of a single
locking API (svn_fs_lock()), I failed to notice that not all the lock
fields (which would have made it into the repository by virtue of
being present in the lock passed in svn_fs_attach_lock()) were
represented as parameters to svn_fs_lock(). Specifically, there is no
'is_xml_comment' parameter to match the relevant lock field. The
result is that is_xml_comment is just lost in the ether between
mod_dav_svn and libsvn_repos/libsvn_fs. And that means that we're
doing extra escaping on a value that shouldn't be re-escaped.
That would be ... uh ... my fault. :-(
Ben Collins-Sussman <su...@collab.net> writes:
> I think there's a latent bug in DAV autoversioning, one I hope to
> isolate and fix today. But I thought I'd alert more people to it...
> more eyes to see it, etc.
>
> Refresher:
>
> In the DAV universe, locks have a <DAV:owner> field, which actually
> has nothing to do with authn or authz; it's just a comment that the
> client expects the server to preserve. In fact, mod_dav_svn maps
> <DAV:owner> to svn_lock_t->comment.
>
> The problem with this is that if you do a lock with an svn client
> over ra_dav, a bunch of XML junk ends up being stored in svn_lock_t-
> >comment (due to mod_dav's design), which violates our principle of
> any RA layer leaving a nasty footprint behind.
>
> So our previous solution was to make mod_dav_svn notice whether the
> incoming LOCK request is coming from svn, or from a generic DAV
> client. In the former case, we strip the XML junk off of the comment
> and store the lock. In the latter case, we leave the XML junk in
> place, but set a flag reminding us it's there.
>
> When retrieving locks, we look at the flag. If not present, we re-
> add proper XML wrapping so as to create a compliant response. If the
> flag is set, we do nothing, because the XML wrapping is already there.
>
> All of this code is in mod_dav_svn/lock.c, functions
> svn_lock_to_dav_lock() and dav_lock_to_svn_lock().
>
>
> Bug:
>
> I'm looking at a Dreamweaver http session, and it's the first DAV
> client I've seen which sets the <DAV:owner> field. It's setting the
> value to "<DAV:href>blah</DAV:href>". Later on, when it does a
> PROPFIND to retrieve the lock, the value isn't coming back identical
> -- it's been accidentally xml-unescaped. (i.e.
> "<DAV:href>blah...") Dreameweaver then chokes and refuses to
> interoperate with mod_dav_svn.
>
>
> Impact of Bug:
>
> I think that any generic DAV client which sets the <DAV:owner> field
> is likely to fail against mod_dav_svn. This is serious enough that I
> might argue it's worth rolling an rc4 tarball for, delaying the final
> release by a day or two. After all, autoversioning is supposed to be
> one of the big new important features in 1.2.
>
>
> Reproducing:
>
> Does anyone know a free DAV client that allows me to set <DAV:owner>
> in a LOCK request? Cadaver won't let me do it, and I don't think
> Windows Webfolders or OSX does it either. Maybe I'll just do a
> 'netcat' simulation.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org