You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Sergey Raevskiy <se...@visualsvn.com> on 2014/03/20 09:46:43 UTC
[PATCH] Fix lock timeout values handling in libsvn_ra_serf
Hi!
I've noticed, that Subversion 1.8.x client can't break lock if it has a
non-infinite timeout (for example, if lock was acquired by non-standard WebDAV
client, like MS Office). The 'svn unlock' fails with following error:
[[[
svn: E170003: Server does not support locking features
svn: E125003: Bogus date
]]]
I've discovered, that this happens because lock timeout values is parsed
incorrectly in libsvn_ra_serf (while libsvn_ra_neon works correct).
According to section 14.29 of RFC 4918 [1] the value of timeout XML element is
transferred as TimeType (see section 10.7 of the same RFC). Instead of handling
it properly, libsvn_ra_serf tries to parse it like X-SVN-Creation-Date header
(which uses ISO-8601 date format):
[[[
if (leaving_state == TIMEOUT)
{
if (strcmp(cdata->data, "Infinite") == 0)
lock_ctx->lock->expiration_date = 0;
else
SVN_ERR(svn_time_from_cstring(&lock_ctx->lock->creation_date,
cdata->data, lock_ctx->pool));
}
]]]
It seems, that code is just copy-pasted from function handle_lock():
[[[
val = serf_bucket_headers_get(headers, SVN_DAV_CREATIONDATE_HEADER);
if (val)
{
SVN_ERR(svn_time_from_cstring(&ctx->lock->creation_date, val,
ctx->pool));
}
]]]
I have attached a patch that fixes this problem. The corresponding code is
taken from libsvn_ra_neon (with little tweaks from me). Log message:
[[[
Fix lock timeout value handling in libsvn_ra_serf: parse value in correct way.
* subversion/libsvn_ra_serf/lock.c
(locks_closed): Parse XML element value as TimeType
Patch by: Sergey Raevskiy <sergey.raevskiy{_AT_}visualsvn.com>
]]]
[1] https://tools.ietf.org/html/rfc4918
Thanks and regards,
Sergey Raevskiy
Re: [PATCH] Fix lock timeout values handling in libsvn_ra_serf
Posted by Sergey Raevskiy <se...@visualsvn.com>.
I'm sorry that I did not test the patch properly on trunk.
I actually wasn't sure about adding regression tests, since the original
client has no support for the timeout feature and there is no common practice
of handmade HTTP requests in Python tests.
Thanks for fixing and committing this!
On Thu, Mar 20, 2014 at 3:04 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Sergey Raevskiy [mailto:sergey.raevskiy@visualsvn.com]
>> Sent: donderdag 20 maart 2014 09:47
>> To: Subversion Development
>> Subject: [PATCH] Fix lock timeout values handling in libsvn_ra_serf
>>
>> Hi!
>>
>> I've noticed, that Subversion 1.8.x client can't break lock if it has a
>> non-infinite timeout (for example, if lock was acquired by non-standard
>> WebDAV
>> client, like MS Office). The 'svn unlock' fails with following error:
>> [[[
>> svn: E170003: Server does not support locking features
>> svn: E125003: Bogus date
>> ]]]
>>
>> I've discovered, that this happens because lock timeout values is parsed
>> incorrectly in libsvn_ra_serf (while libsvn_ra_neon works correct).
>>
>> According to section 14.29 of RFC 4918 [1] the value of timeout XML
> element
>> is
>> transferred as TimeType (see section 10.7 of the same RFC). Instead of
>> handling
>> it properly, libsvn_ra_serf tries to parse it like X-SVN-Creation-Date
> header
>> (which uses ISO-8601 date format):
>
> Thanks...
>
> With a bit of python hacking I was able to reproduce this issue in our test
> suite.
>
> It looks like your fix was not tested on trunk, because it applies the fix
> to only one of the locations where we parse this value... And exactly the
> other one than that used by 'svn unlock'.
> (I think this code was shared between the two implementations on <= 1.8.x)
>
> In r1579588 I added the test and a slightly different patch.
> (The timeout value in the lock response is relative from the current time
> (=seconds remaining), not the time of the lock creation.)
>
> Bert
>
RE: [PATCH] Fix lock timeout values handling in libsvn_ra_serf
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Sergey Raevskiy [mailto:sergey.raevskiy@visualsvn.com]
> Sent: donderdag 20 maart 2014 09:47
> To: Subversion Development
> Subject: [PATCH] Fix lock timeout values handling in libsvn_ra_serf
>
> Hi!
>
> I've noticed, that Subversion 1.8.x client can't break lock if it has a
> non-infinite timeout (for example, if lock was acquired by non-standard
> WebDAV
> client, like MS Office). The 'svn unlock' fails with following error:
> [[[
> svn: E170003: Server does not support locking features
> svn: E125003: Bogus date
> ]]]
>
> I've discovered, that this happens because lock timeout values is parsed
> incorrectly in libsvn_ra_serf (while libsvn_ra_neon works correct).
>
> According to section 14.29 of RFC 4918 [1] the value of timeout XML
element
> is
> transferred as TimeType (see section 10.7 of the same RFC). Instead of
> handling
> it properly, libsvn_ra_serf tries to parse it like X-SVN-Creation-Date
header
> (which uses ISO-8601 date format):
Thanks...
With a bit of python hacking I was able to reproduce this issue in our test
suite.
It looks like your fix was not tested on trunk, because it applies the fix
to only one of the locations where we parse this value... And exactly the
other one than that used by 'svn unlock'.
(I think this code was shared between the two implementations on <= 1.8.x)
In r1579588 I added the test and a slightly different patch.
(The timeout value in the lock response is relative from the current time
(=seconds remaining), not the time of the lock creation.)
Bert
Re: [PATCH] Fix lock timeout values handling in libsvn_ra_serf
Posted by Philip Martin <ph...@wandisco.com>.
Sergey Raevskiy <se...@visualsvn.com> writes:
> Index: subversion/libsvn_ra_serf/lock.c
> ===================================================================
> --- subversion/libsvn_ra_serf/lock.c (revision 1579559)
> +++ subversion/libsvn_ra_serf/lock.c (working copy)
> @@ -150,9 +150,17 @@ locks_closed(svn_ra_serf__xml_estate_t *xes,
> {
> if (strcmp(cdata->data, "Infinite") == 0)
> lock_ctx->lock->expiration_date = 0;
> + else if (strncmp(cdata->data, "Second-", 7) == 0)
> + {
> + int time_offset;
> +
> + SVN_ERR(svn_cstring_atoi(&time_offset, cdata->data + 7));
> + lock_ctx->lock->expiration_date = lock_ctx->lock->creation_date
> + + apr_time_from_sec(time_offset);
> + }
> else
> - SVN_ERR(svn_time_from_cstring(&lock_ctx->lock->creation_date,
> - cdata->data, lock_ctx->pool));
> + return svn_error_create(SVN_ERR_RA_DAV_RESPONSE_HEADER_BADNESS,
> + NULL, _("Invalid timeout value"));
> }
> else if (leaving_state == HREF)
> {
We need the same fix in get_lock.c as well. I'm looking at a regression
test.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*