You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Wim Lewis <wi...@omnigroup.com> on 2013/04/03 03:20:26 UTC
Patches still languishing in Bugzilla
There are a number of OK-looking patches in Bugzilla that have been sitting there with no action for a while. I just went through the recent "Bug report" looking for bugs of interest to me that have patches. All of the following bugs have a patch, and most of them address a pretty straightforward failure to implement the spec. None of the bugs' comments indicate objections to their patches (although in some cases that's because nobody has commented on them at all, beyond the initial problem description and patch submission):
|35981|New|Maj|2005-08-02|mod_dav overrides dav_fs response on PUT failure |
|39287|New|Nor|2006-04-12|Incorrect If-Modified-Since validation (due to syn|
|50773|New|Nor|2011-02-14|Dav lock database corruption |
|51297|New|Nor|2011-05-31|Improve error handling during "UNLOCK" |
|52559|New|Nor|2012-01-30|[PATCH] Some PROPPATCH causing segfault in mod_dav|
|53525|New|Nor|2012-07-09|PROPPATCH delete (svn propdel) errors not returned|
|53741|New|Min|2012-08-19|Code clean up (Concat string at compile time when |
|53910|New|Nor|2012-09-21|If: check spuriously succeeds with %-encoded URL a|
|54145|New|Min|2012-11-14|Poor error handling in dav_method_put() |
|54611|New|Nor|2013-02-26|Location header for dav_created not URI encoded | (general case of |54367|New|Maj|2013-01-02|Location header in response to PUT is not %-escape|?)
|54610|New|Nor|2013-02-26|COPY doesn't respect If/If-Modified/etc |
I'd like to propose that 35981, 39287, 51297, 52559, 53741, 54145 be committed.
The patches in 50773, 53525, 53910, 54610, and 54611 perhaps should be evaluated by someone with more familiarity with the server internals before they are committed, but I'd be especially grateful if someone would look at 53910, 54610, and/or 54611; these are bugs that make the stock Apache unusable for us.
Re: Patches still languishing in Bugzilla
Posted by William Lewis <wi...@omnigroup.com>.
On 2013-04-03 02:43:34 -0700, Graham Leggett said:
> Can you confirm that the patches you're citing work for you in
> httpd-trunk and solve your problem? The wider testing the patches get
> the better.
They were developed and tested against 2.4.3/2.4.4 --- I'll test them
against trunk.
Re: Patches still languishing in Bugzilla
Posted by William Lewis <wi...@omnigroup.com>.
On 2013-04-27 10:22:41 -0700, Graham Leggett said:
> On 23 Apr 2013, at 4:27 AM, William Lewis <wi...@omnigroup.com> wrote:
>> I've tested the patches in 53910, 54610, and 54611 against trunk as
>> well as 2.4.4; they apply and they fix their respective bugs (at least
>> as far as my test cases go). I don't have an easy way to test 53525,
>> though.
>
> I've committed these to trunk, if you verify for me that the bugs are
> all fixed I can then propose them for backport to v2.4.
Many thanks! I've verified them, below:
Bug 53910: Oddly enough, the version in trunk doesn't fix the bug. It
looks like the reason is that the patch applied was different from the
patch in bugzilla --- the call to ap_unescape_url() was moved to after
the computation of uri_len, which ended up leaving
dav_if_header->uri_len inconsistent.
I assume the reason was to do ap_getparents() before unescaping? Doing
the unescaping after ap_getparents() but before computing uri_len works.
On the other hand, the process of canonicalizing two URI-paths so that
URI equality can be checked using string compares seems like a really
common task that should maybe have its own function. (Assuming it's
possible in the general case! Either that or URIs shouldn't be
string-compared...) In this code, the canonicalization consists of
ap_getparents(), unescaping, and removing any trailing slash, but there
are probably a few other things it should do ... and in DAV code, the
filesystem provider might want to have a say ...
Bug 54610: Passes my tests in trunk now. Having become a little more
familiar with mod_dav, though, I'm unsure about one thing: can anyone
comment on whether DAV_VALIDATE_PARENT should be being passed to
dav_validate_request() for a COPY as well as a MOVE?
Bug 54611: Fixed in trunk, hooray :)
>> The patch in 50773 no longer applies [....]
>
> Does this do the trick?
Hmmm, no it doesnt: I'm still getting lock database corruption after a
lock expires:
[Mon Apr 29 17:19:14.321182 2013] [dav:error] [pid 57745] [client
127.0.0.1:58534] Could not LOCK /wiml/bugzilla50773/dirA/ due to a
failed precondition (e.g. other locks). [500, #0]
[Mon Apr 29 17:19:14.321193 2013] [dav:error] [pid 57745] [client
127.0.0.1:58534] The locks could not be queried for verification
against a possible "If:" header. [500, #0]
[Mon Apr 29 17:19:14.321199 2013] [dav:error] [pid 57745] [client
127.0.0.1:58534] The lock database was found to be corrupt. An indirect
lock's direct lock could not be found. [500, #402]
Wim.
Re: Patches still languishing in Bugzilla
Posted by Graham Leggett <mi...@sharp.fm>.
On 23 Apr 2013, at 4:27 AM, William Lewis <wi...@omnigroup.com> wrote:
> I've tested the patches in 53910, 54610, and 54611 against trunk as well as 2.4.4; they apply and they fix their respective bugs (at least as far as my test cases go). I don't have an easy way to test 53525, though.
I've committed these to trunk, if you verify for me that the bugs are all fixed I can then propose them for backport to v2.4.
> The patch in 50773 no longer applies because the code has been reorganized somewhat. The bug can still be reproduced in trunk, though, and a glance at the new code makes me think the reason for it hasn't changed --- functions have been renamed and files have been split but the code the patch touches is doing the same things it was before. Someone a bit more familiar with the dav lock code probably should be the person to adapt the patch to the new world though.
Does this do the trick?
Index: modules/dav/lock/locks.c
===================================================================
--- modules/dav/lock/locks.c (revision 1476625)
+++ modules/dav/lock/locks.c (working copy)
@@ -646,7 +646,8 @@
ip->key.dptr = apr_pmemdup(p, val.dptr + offset, ip->key.dsize);
offset += ip->key.dsize;
- if (!dav_generic_lock_expired(ip->timeout)) {
+ if (!dav_generic_lock_expired(ip->timeout)
+ && dav_dbm_exists(lockdb->info->db, ip->key)) {
ip->next = *indirect;
*indirect = ip;
}
@@ -847,6 +848,7 @@
else {
/* DAV_GETLOCKS_PARTIAL */
newlock->rectype = DAV_LOCKREC_INDIRECT_PARTIAL;
+ newlock->timeout = ip->timeout;
}
/* hook into the result list */
Regards,
Graham
--
Re: Patches still languishing in Bugzilla
Posted by William Lewis <wi...@omnigroup.com>.
On 2013-04-03 02:43:34 -0700, Graham Leggett said:
> On 03 Apr 2013, at 3:20 AM, Wim Lewis <wi...@omnigroup.com> wrote:
>
>> The patches in 50773, 53525, 53910, 54610, and 54611 perhaps should be
>> evaluated by someone with more familiarity with the server internals
>> before they are committed, but I'd be especially grateful if someone
>> would look at 53910, 54610, and/or 54611; these are bugs that make the
>> stock Apache unusable for us.
>
> Can you confirm that the patches you're citing work for you in
> httpd-trunk and solve your problem? The wider testing the patches get
> the better.
I've tested the patches in 53910, 54610, and 54611 against trunk as
well as 2.4.4; they apply and they fix their respective bugs (at least
as far as my test cases go). I don't have an easy way to test 53525,
though.
The patch in 50773 no longer applies because the code has been
reorganized somewhat. The bug can still be reproduced in trunk, though,
and a glance at the new code makes me think the reason for it hasn't
changed --- functions have been renamed and files have been split but
the code the patch touches is doing the same things it was before.
Someone a bit more familiar with the dav lock code probably should be
the person to adapt the patch to the new world though.
Wim.
Re: Patches still languishing in Bugzilla
Posted by Graham Leggett <mi...@sharp.fm>.
On 03 Apr 2013, at 3:20 AM, Wim Lewis <wi...@omnigroup.com> wrote:
> The patches in 50773, 53525, 53910, 54610, and 54611 perhaps should be evaluated by someone with more familiarity with the server internals before they are committed, but I'd be especially grateful if someone would look at 53910, 54610, and/or 54611; these are bugs that make the stock Apache unusable for us.
Can you confirm that the patches you're citing work for you in httpd-trunk and solve your problem? The wider testing the patches get the better.
Regards,
Graham
--