On Friday 14 July 2006 00:46, William A. Rowe, Jr. wrote:
OK, I have a fix that appears to work. Mostly.
Well, it fixes the problem reported. This was working with
2.2.2, and I attach a patch against that.
> Nick Kew wrote:
> > I had in mind to fix this tonight. But it's too complex to get right
> > late at night after a long day and a couple of glasses of wine.
> >
> > A look at it suggests we have a bug in ap_directory_walk
> > affecting all versions. The "cached dir walk" optimisation at
> > lines 555-583 (Trunk) is losing the symlink check.
FWIW, that was the directory_walk called from ap_internal_fast_redirect
in fixup_dir. Not the one from ap_sub_req_lookup_uri.
> >
> > The quickest fix would be to scrap the optimisation altogether.
> > Otherwise, we need to run the extra checks at lines ~942-1014
> > at that point.
>
> Scrapping the optimization is a red herring.
Possibly.
> The optimization logic -should- be noting where the original pass varies
> from the current pass through merging, and then taking a left turn to then
> construct fresh merges of previously-unmerged sections.
>
> I suspect there 1) might be an overly agressive shortcut out of the merge
> checking, and 2) that r->filename might not be updated early enough by
> mod_dir to catch index.html. If r->filename is still /foo/bar/ and not
> /foo/bar/index.html then the entire dir_walk assumption is a red herring.
It is set to index.html second time through (I walked through that with gdb).
When you request index.html explicitly, the check happens in the
directory walk code (lines 942-1014), which returns FORBIDDEN.
In the problem case, first time through it's the directory, which is of course
allowed (and not a symlink). Second time round - when it is the symlink -
it has optimised out the directory walk on the necessary but not
sufficient grounds that the per-dir config is identical.
Now that's not the whole story. If I wrap index.html in a <Files> stanza,
the per-dir config is not identical, but it still misses the symlink check.
I haven't figured out the full gory details yet, but here's some more.
Directory walk called from ap_sub_req_lookup_uri is returning
index.html as a regular file, not a symlink. I tried fixing that with
a patch:
--- server/request.c.222 2006-04-22 02:53:06.000000000 +0100
+++ server/request.c 2006-07-14 01:58:48.511404250 +0100
@@ -525,7 +525,7 @@
* with APR_ENOENT, knowing that the path is good.
*/
if (!r->finfo.filetype || r->finfo.filetype == APR_LNK) {
- rv = apr_stat(&r->finfo, r->filename, APR_FINFO_MIN, r->pool);
+ rv = apr_stat(&r->finfo, r->filename, APR_FINFO_MIN|APR_FINFO_LINK,
r->pool);
/* some OSs will return APR_SUCCESS/APR_REG if we stat
* a regular file but we have '/' at the end of the name;
@@ -545,7 +545,7 @@
}
}
- if (r->finfo.filetype == APR_REG) {
+ if ((r->finfo.filetype == APR_REG) || (r->finfo.filetype == APR_LNK)) {
entry_dir = ap_make_dirstr_parent(r->pool, entry_dir);
}
else if (r->filename[strlen(r->filename) - 1] != '/') {
This works, in that it's now FORBIDDEN when it should be. However,
it screws up when symlinks are permitted: it then marks index.html
as a directory (line 1023; "thisinfo" having been set at line 937),
so when it reaches fixup_dir in the subreq, it redirects to index.html/
Now, if in addition to the above patch, I remove the optimisation at
lines 923-940, it works correctly with FollowSymLinks either on or off.
I'm not sure about SymLinksIfOwnerMatch: it appears to be ignored,
and It's too late in the wee hours to figure that out.
--
Nick Kew
--- server/request.c.222 2006-04-22 02:53:06.000000000 +0100
+++ server/request.c 2006-07-14 02:55:19.148745250 +0100
@@ -525,7 +525,7 @@
* with APR_ENOENT, knowing that the path is good.
*/
if (!r->finfo.filetype || r->finfo.filetype == APR_LNK) {
- rv = apr_stat(&r->finfo, r->filename, APR_FINFO_MIN, r->pool);
+ rv = apr_stat(&r->finfo, r->filename, APR_FINFO_MIN|APR_FINFO_LINK, r->pool);
/* some OSs will return APR_SUCCESS/APR_REG if we stat
* a regular file but we have '/' at the end of the name;
@@ -545,7 +545,7 @@
}
}
- if (r->finfo.filetype == APR_REG) {
+ if ((r->finfo.filetype == APR_REG) || (r->finfo.filetype == APR_LNK)) {
entry_dir = ap_make_dirstr_parent(r->pool, entry_dir);
}
else if (r->filename[strlen(r->filename) - 1] != '/') {
@@ -919,7 +919,7 @@
if (!*seg_name) {
break;
}
-
+#if 0
/* First optimization;
* If...we knew r->filename was a file, and
* if...we have strict (case-sensitive) filenames, or
@@ -938,6 +938,7 @@
++seg;
continue;
}
+#endif
/* We choose apr_stat with flag APR_FINFO_LINK here, rather that
* plain apr_stat, so that we capture this path object rather than
