You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Greg Ames <gr...@remulak.net> on 2001/09/26 22:44:59 UTC
[PATCH] autoindex instead of index.html
Greg Ames wrote:
>
> Ryan Bloom wrote:
> >
> > On Tuesday 25 September 2001 03:32 pm, Greg Ames wrote:
> >
> > Yeah, that should fail. Why isn't that an absolute path?
>
> That's the bug. We're not mapping the subrequest URI into a path in
> this case.
OK, this takes care of it. mod_rewrite's translation phase
(hook_uri2file) uses r->filename all over the place, and leaves it set
even if no rules apply. A recent change to ap_core_translate bails out
if it's a subrequest and r->filename is already set. I don't know what
the intention of this change was, and I don't plan on rewriting
mod_rewrite, but I will test this little patch more thoroughly.
Greg
Index: server/core.c
===================================================================
RCS file: /cvs/apache/httpd-2.0/server/core.c,v
retrieving revision 1.61
diff -u -d -b -r1.61 core.c
--- core.c 2001/09/19 05:52:42 1.61
+++ core.c 2001/09/26 20:44:48
@@ -2555,8 +2555,6 @@
/* XXX We have already been here, or another module did the work
* for us. At this moment, we will enable only file subrequests.
*/
- if (r->main && r->filename)
- return OK;
/* XXX this seems too specific, this should probably become
* some general-case test
Re: [PATCH] autoindex instead of index.html
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Greg Ames" <gr...@remulak.net>
Sent: Thursday, September 27, 2001 4:01 PM
> "William A. Rowe, Jr." wrote:
>
> > HERE is the difference. In 1.2-1.3, we have never called the normal
> > request cycle for subrequests, we've had hackish, occasionally broken and
> > unmaintainable variants for each subrequest flavor. Several months ago
> > I joined those in common code. Within the last month I rerouted all that
> > brokenness through ONE block of code (ap_internal_process_request) that
> > will draw out any discrepancies.
>
> > I'd argue the right fix -could- be a translate_name RUN_VERY_FIRST hook
> > in the request.c module, that says, "Hmmm... a _file_ subreq, better skip
> > the translate name phase" and returns OK.
>
> I like the sound of that. Or why not have ap_process_request_internal
> do exactly the same test, and skip running the hook altogether? That
> would save an extra computed branch in mainline code, and make life
> slightly easier for the CPU's branch prediction logic.
If we must skip the <Location > walk as well, this may be the better hack.
I'm entirely unhappy with the prospects of ongoing sub_req_lookup_file
bogusness. But this seems like a bigger fish than Apache 2.0, and we want
to keep moving.
I'll commit a fix to write a note on the request, and skip the location_walk(s)
and translate_name as appropriate if that note is detected. Look for the
commit this evening, it will back out the original hack you've complained about.
Bill
Re: [PATCH] autoindex instead of index.html
Posted by Greg Ames <gr...@remulak.net>.
"William A. Rowe, Jr." wrote:
>
>
> Then back it out already, if you like.
If backing it out breaks something else, I'd rather wait. daedalus
seems happier with it off, but I still have a bunch of other new errors
to work thru. A lot of them are due to site content changes, but they
still take time to investigate.
> This is a very hackish hack. Let's revisit the original problem.
>
> translate_names was never invoked before for file subrequests.
OK, thanks, I didn't know that.
> Since mod_rewrite returns DECLINED - I entirely agree with your perspective.
> Now there are bigger questions - but those can wait a few hours. Let's
> revert back to the original broken behavior, and evaluate a better solution
> than the 1.58 patch above.
> HERE is the difference. In 1.2-1.3, we have never called the normal
> request cycle for subrequests, we've had hackish, occasionally broken and
> unmaintainable variants for each subrequest flavor. Several months ago
> I joined those in common code. Within the last month I rerouted all that
> brokenness through ONE block of code (ap_internal_process_request) that
> will draw out any discrepancies.
> I'd argue the right fix -could- be a translate_name RUN_VERY_FIRST hook
> in the request.c module, that says, "Hmmm... a _file_ subreq, better skip
> the translate name phase" and returns OK.
I like the sound of that. Or why not have ap_process_request_internal
do exactly the same test, and skip running the hook altogether? That
would save an extra computed branch in mainline code, and make life
slightly easier for the CPU's branch prediction logic.
Greg
Re: [PATCH] autoindex instead of index.html
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Greg Ames" <gr...@remulak.net>
Sent: Thursday, September 27, 2001 10:14 AM
> "William A. Rowe, Jr." wrote:
> >
> > -1 on this patch!!!
>
> Forget it. Your revision 1.58 patch to core.c broke configurations with
> mod_rewrite enabled. Because of that, I am herebyvetoing your 1.58
> patch. If I find that the server performs more correctly without it, I
> will back it out. If you come up with something else that works with
> mod_rewrite in the mean time, great.
Then back it out already, if you like. From the CVS commit;
Can it be this simple? No, probably not, but this fast-hack will get
us going again for a while.
We are currently rejecting some internal file_sub_req()'s in the
translate phase. I don't like this hack because of risks it potentially
exposes, but for today, if we have a filename - and we are a subrequest,
then let it fly without further mapping. This allows us to serve up
the default "/" request (run through mod_dir->mod_negotiation->mod_mime)
without a 400 error. The right solution is to set up some traps and
escapes for the subreq mechanism, possibly with a subreq translate hook,
and drop the URI entirely for these cases.
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/core.c,v
retrieving revision 1.57
retrieving revision 1.58
diff -u -r1.57 -r1.58
--- httpd-2.0/server/core.c 2001/08/30 14:54:50 1.57
+++ httpd-2.0/server/core.c 2001/08/31 13:45:16 1.58
@@ -2867,13 +2867,19 @@
void *sconf = r->server->module_config;
core_server_config *conf = ap_get_module_config(sconf, &core_module);
+ /* XXX We have already been here, or another module did the work
+ * for us. At this moment, we will enable only file subrequests.
+ */
+ if (r->main && r->filename)
+ return OK;
+
/* XXX this seems too specific, this should probably become
* some general-case test
*/
if (r->proxyreq) {
return HTTP_FORBIDDEN;
}
- if ((r->uri[0] != '/') && strcmp(r->uri, "*")) {
+ if (!r->uri || ((r->uri[0] != '/') && strcmp(r->uri, "*"))) {
ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0, r,
"Invalid URI in request %s", r->the_request);
return HTTP_BAD_REQUEST;
This is a very hackish hack. Let's revisit the original problem.
translate_names was never invoked before for file subrequests.
> > If this is a subrequest, we DON'T WANT TO MUCK THIS UP! It isn't
> > subject to all this remapping. [Think sub_req_lookup_file.]
>
> huh? since when are subrequests not subject to URI translation? That's
> where this goes south.
Since mod_rewrite returns DECLINED - I entirely agree with your perspective.
Now there are bigger questions - but those can wait a few hours. Let's
revert back to the original broken behavior, and evaluate a better solution
than the 1.58 patch above.
> > If someone munges filename, then they've broken things. My question is
> > what changed that introduced this bug.
>
> it was your rev. 1.58 patch to core.c.
>
> > And, why rewrite is getting tangled in it.
>
> mod_rewrite's URI translation phase has been setting r->filename for
> subrequests since at least Apache 1.2.
>
> You tried to give an old API a new meaning. If you want to pass new
> info to ap_core_translate, it would be better if you did it in a way
> that won't impact existing modules (new field, hide it in
> r->request_config for core, whatever). But if you must use an old API
> in a new way, that's fine, as long as you also change the modules that
> use the API, and document the new restriction.
HERE is the difference. In 1.2-1.3, we have never called the normal
request cycle for subrequests, we've had hackish, occasionally broken and
unmaintainable variants for each subrequest flavor. Several months ago
I joined those in common code. Within the last month I rerouted all that
brokenness through ONE block of code (ap_internal_process_request) that
will draw out any discrepancies.
Now we are back at square one, each ap_internal_process_request() step
needs to be optimized. Several folks have carefully examined the loc_walk
optimizations that cancel out the penalties here. dir_walk and file_walk
will soon have similar optimizations. proxy_walk overrode those costly
operations, and other 3rd party modules may do the same.
Anyone can look at internal subrequest/redirect authn/authz optimizations
to spare those steps. Anyone can look at the other phases and make the
same decisions. But we are arguing here about the translate_names phase.
I'd argue the right fix -could- be a translate_name RUN_VERY_FIRST hook
in the request.c module, that says, "Hmmm... a _file_ subreq, better skip
the translate name phase" and returns OK.
File subrequests are for backwards-brokenness (e.g. they escape from a
bunch of potential rules) since any given file may not have a vhost/uri
mapping at all. That's what we've used forever, I don't see that being
resolved for 2.0.
Would a hook satisfy everyone (preventing mod_rewrite expansion, which
never happened in 2.0 subreq_lookup_file in the first place)?
Bill
Re: [PATCH] autoindex instead of index.html
Posted by Greg Ames <gr...@remulak.net>.
"William A. Rowe, Jr." wrote:
>
> From: "Greg Ames" <gr...@remulak.net>
> Sent: Wednesday, September 26, 2001 3:44 PM
>
> > Greg Ames wrote:
> > >
> > > Ryan Bloom wrote:
> > > >
> > > > On Tuesday 25 September 2001 03:32 pm, Greg Ames wrote:
> > > >
> > > > Yeah, that should fail. Why isn't that an absolute path?
> > >
> > > That's the bug. We're not mapping the subrequest URI into a path in
> > > this case.
> >
> > OK, this takes care of it. mod_rewrite's translation phase
> > (hook_uri2file) uses r->filename all over the place, and leaves it set
> > even if no rules apply. A recent change to ap_core_translate bails out
> > if it's a subrequest and r->filename is already set. I don't know what
> > the intention of this change was, and I don't plan on rewriting
> > mod_rewrite, but I will test this little patch more thoroughly.
> >
> > Greg
> >
> > Index: server/core.c
> > ===================================================================
> > RCS file: /cvs/apache/httpd-2.0/server/core.c,v
> > retrieving revision 1.61
> > diff -u -d -b -r1.61 core.c
> > --- core.c 2001/09/19 05:52:42 1.61
> > +++ core.c 2001/09/26 20:44:48
> > @@ -2555,8 +2555,6 @@
> > /* XXX We have already been here, or another module did the work
> > * for us. At this moment, we will enable only file subrequests.
> > */
> > - if (r->main && r->filename)
> > - return OK;
>
> -1 on this patch!!!
Forget it. Your revision 1.58 patch to core.c broke configurations with
mod_rewrite enabled. Because of that, I am herebyvetoing your 1.58
patch. If I find that the server performs more correctly without it, I
will back it out. If you come up with something else that works with
mod_rewrite in the mean time, great.
> If this is a subrequest, we DON'T WANT TO MUCK THIS UP! It isn't
> subject to all this remapping. [Think sub_req_lookup_file.]
huh? since when are subrequests not subject to URI translation? That's
where this goes south.
> If someone munges filename, then they've broken things. My question is
> what changed that introduced this bug.
it was your rev. 1.58 patch to core.c.
> And, why rewrite is getting tangled in it.
mod_rewrite's URI translation phase has been setting r->filename for
subrequests since at least Apache 1.2.
You tried to give an old API a new meaning. If you want to pass new
info to ap_core_translate, it would be better if you did it in a way
that won't impact existing modules (new field, hide it in
r->request_config for core, whatever). But if you must use an old API
in a new way, that's fine, as long as you also change the modules that
use the API, and document the new restriction.
Greg
Re: [PATCH] autoindex instead of index.html
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Greg Ames" <gr...@remulak.net>
Sent: Wednesday, September 26, 2001 3:44 PM
> Greg Ames wrote:
> >
> > Ryan Bloom wrote:
> > >
> > > On Tuesday 25 September 2001 03:32 pm, Greg Ames wrote:
> > >
> > > Yeah, that should fail. Why isn't that an absolute path?
> >
> > That's the bug. We're not mapping the subrequest URI into a path in
> > this case.
>
> OK, this takes care of it. mod_rewrite's translation phase
> (hook_uri2file) uses r->filename all over the place, and leaves it set
> even if no rules apply. A recent change to ap_core_translate bails out
> if it's a subrequest and r->filename is already set. I don't know what
> the intention of this change was, and I don't plan on rewriting
> mod_rewrite, but I will test this little patch more thoroughly.
>
> Greg
>
> Index: server/core.c
> ===================================================================
> RCS file: /cvs/apache/httpd-2.0/server/core.c,v
> retrieving revision 1.61
> diff -u -d -b -r1.61 core.c
> --- core.c 2001/09/19 05:52:42 1.61
> +++ core.c 2001/09/26 20:44:48
> @@ -2555,8 +2555,6 @@
> /* XXX We have already been here, or another module did the work
> * for us. At this moment, we will enable only file subrequests.
> */
> - if (r->main && r->filename)
> - return OK;
-1 on this patch!!!
If this is a subrequest, we DON'T WANT TO MUCK THIS UP! It isn't
subject to all this remapping. [Think sub_req_lookup_file.]
If someone munges filename, then they've broken things. My question is
what changed that introduced this bug. And, why rewrite is getting
tangled in it.
Please spend a bit more time on this, and I will join you in the effort.
mod_rewrite has been a great source of joy and constranation for admins
for many years - I'm really not surprized it might have some broken
behavior, and the time to clean it up is before GA, when we can tell
the users what to expect, rather than after when it 'bites' them.
Bill