httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Francis Daly <d...@daoine.org>
Subject [PATCH] mod_autoindex missing some filenames
Date Tue, 26 Nov 2002 18:04:42 GMT
Hi there,

Some files with % in the name aren't listed by mod_autoindex.

(See http://nagoya.apache.org/bugzilla/show_bug.cgi?id=13598 for a
report)

The behaviour seems to have become broken this way some time between
2.0.25 and 2.0.28. 

As far as I can tell, if a file name is like x%33x, then ap_unescape_url
allows it, and the file is listed.  If it is like x%ggx, then it fails
it, and it is not listed.  If the file is listed, it is listed correctly,
with the correct html and uri escaping in both the href and the content
part.

In the cases above, "33" is a valid %-encoding, and "gg" isn't.

The breakage seems to be in ap_process_request_internal(), when
ap_unescape_url(r->parsed_uri.path) is called.  I don't believe it
needs to be called at all, since r->uri has just come straight from
the filesystem.  The obvious place to change things seems to be in
ap_sub_req_lookup_dirent().  Near the end of that function is the comment
"fill in parsed_uri values".  Is it actually necessary to do that?

In the 2.0.43 code, ap_sub_req_lookup_dirent is called in two places:

mod_autoindex.c: ap_sub_req_lookup_dirent(dirent, r, 
                                          AP_SUBREQ_NO_ARGS, NULL)
mod_negotiation.c: ap_sub_req_lookup_dirent(&dirent, r, 
                                            AP_SUBREQ_MERGE_ARGS, NULL)

mod_autoindex doesn't appear to directly care about ->unparsed_uri,
or ->parsed_uri.*, which seem to be what ap_parse_uri modifies.
mod_negotiation also doesn't appear to directly care about them.
Possibly they are needed by some later potential module/filter?

As a test of both mod_autoindex and mod_negotiation, create some
files:

cd DOCROOT
mkdir scratch
echo t%31.html > scratch/t%31.html
echo t%g1.html > scratch/t%g1.html

<Location /scratch>
Options +Indexes +MultiViews
</Location>

Vanilla:
$ GET http://localhost:2043/scratch/
..
<img src="/icons/text.gif" alt="[TXT]" /> <a href="t%2531.html">t%31.html</a>
              26-Nov-2002 17:34   10
..
$ GET http://localhost:2043/scratch/t%2531
t%31.html
$ GET http://localhost:2043/scratch/t%25g1
..
404 Not Found
..

Patch server/request.c by commenting out lines 1757 to 1763 (cvs
version 1.114)

$ GET http://localhost:2043/scratch/
..
<img src="/icons/text.gif" alt="[TXT]" /> <a href="t%2531.html">t%31.html</a>
              26-Nov-2002 17:34   10   
<img src="/icons/text.gif" alt="[TXT]" /> <a href="t%25g1.html">t%g1.html</a>
              26-Nov-2002 17:34   10   
..
$ GET http://localhost:2043/scratch/t%2531
t%31.html
$ GET http://localhost:2043/scratch/t%25g1
t%g1.html

The output looks right to me, although I'm not sure what nasty side
effects might be brought in by removing the call to ap_parse_uri().

Those lines are 1774 to 1780 in the current cvs version, 1.121, and
that is what this patch is against.

(Of course, if this is the right fix, then the right implementation of
the fix is just to delete those 7 lines.)

There may be some better way of not calling ap_unescape_url() on a
plain filename which is more appropriate.

All the best,

	f
-- 
Francis Daly        deva@daoine.org

--- request.c-1.121	Tue Nov 26 17:42:35 2002
+++ request.c	Tue Nov 26 17:58:07 2002
@@ -1771,6 +1771,7 @@
 
     /* fill in parsed_uri values
      */
+    /*
     if (r->args && *r->args && (subtype == AP_SUBREQ_MERGE_ARGS)) {
         ap_parse_uri(rnew, apr_pstrcat(r->pool, rnew->uri, "?",
                                        r->args, NULL));
@@ -1778,6 +1779,7 @@
     else {
         ap_parse_uri(rnew, rnew->uri);
     }
+    */
 
     if ((res = ap_process_request_internal(rnew))) {
         rnew->status = res;

Mime
View raw message