You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ryan Bloom <rb...@covalent.net> on 2002/04/05 08:34:06 UTC
[PATCH] Fix mod_autoindex
This is another attempt at fixing mod_autoindex. This works for me on
my computer, and it explains the problem. I haven't been able to
actually reproduce the problem, but I have seen the symptoms. Can
somebody who can reproduce PLEASE test this on their setup. I am hoping
to setup a failure condition soon so that I can test this myself.
I may commit this either way, because even if this doesn't fix the
problem from apache.org, it does solve a problem with data ordering.
This is the cleanest way I could find to solve the problem, but I am
perfectly willing to listen to other ideas.
Ryan
Index: modules/generators/mod_autoindex.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/generators/mod_autoindex.c,v
retrieving revision 1.102
diff -u -d -b -w -u -r1.102 mod_autoindex.c
--- modules/generators/mod_autoindex.c 20 Mar 2002 17:41:54 -0000
1.102
+++ modules/generators/mod_autoindex.c 5 Apr 2002 06:26:08 -0000
@@ -1017,6 +1017,7 @@
if (rr->content_type != NULL) {
if (!strcasecmp(ap_field_noparam(r->pool,
rr->content_type),
"text/html")) {
+ ap_filter_t *f;
/* Hope everything will work... */
emit_amble = 0;
emit_H1 = 0;
@@ -1024,6 +1025,22 @@
if (! suppress_amble) {
emit_preamble(r, title);
}
+ /* This is a hack, but I can't find any better way to
do this.
+ * The problem is that we have already created the
sub-request,
+ * but we just inserted the OLD_WRITE filter, and the
+ * sub-request needs to pass its data through the
OLD_WRITE
+ * filter, or things go horribly wrong (missing data,
data in
+ * the wrong order, etc). To fix it, if you create a
+ * sub-request and then insert the OLD_WRITE filter
before you
+ * run the request, you need to make sure that the
sub-request
+ * data goes through the OLD_WRITE filter. Just steal
this
+ * code. The long-term solution is to remove the ap_r*
+ * functions.
+ */
+ for (f=rr->output_filters;
+ f->frec != ap_subreq_core_filter_handle; f =
f->next);
+ f->next = r->output_filters;
+
/*
* If there's a problem running the subrequest, display
the
* preamble if we didn't do it before -- the header
file
----------------------------------------------
Ryan Bloom rbb@covalent.net
645 Howard St. rbb@apache.org
San Francisco, CA
Re: [PATCH] Fix mod_autoindex
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 10:06 AM 4/5/2002, you wrote:
>(This is really from me :) )
>
> > > Greg and I discussed this issue (create subrequest, create output via
> > > ap_rXXX, run subrequest) a couple of days ago. What we preferred
> > > overall was to require an app to call something like ap_rinit() if
> > > they were going to use ap_rXXX functions. ap_rinit() would add
> > > old_write filter. ap_rinit() would have to be called at the start of
> > > content generation, before creating any subrequests.
Then why are we still encouraging users to use the ap_rfoo() API? This seems
like we've abandoned that approach.
Perhaps a simpler solution is to allow a call such as;
ap_rwrite("", 0, r);
Which aught to initialize the OLD_WRITE filter, even if we don't create
a bucket at that time [due to the 0 bytes, no OLD_WRITE bucket needed.]
Bill
RE: [PATCH] Fix mod_autoindex
Posted by Ryan Bloom <rb...@covalent.net>.
OKAY, I have fully diagnosed the problem.
The problem only occurs if you have multi-views enabled for the HEADER
and README files. What is killing us is that we are creating a
sub-request from within another sub-request, but we only have one
SUB_REQ_OUTPUT_FILTER on the chain. This means that we end up removing
the filter and then we finalize the first sub_request.
I'm creating a patch now.
Ryan
----------------------------------------------
Ryan Bloom rbb@covalent.net
645 Howard St. rbb@apache.org
San Francisco, CA
> -----Original Message-----
> From: trawick@rdu88-250-035.nc.rr.com [mailto:trawick@rdu88-250-
> 035.nc.rr.com] On Behalf Of Jeff Trawick
> Sent: Friday, April 05, 2002 8:06 AM
> To: rbb@covalent.net
> Cc: dev@httpd.apache.org
> Subject: Re: [PATCH] Fix mod_autoindex
>
> (This is really from me :) )
>
> "Ryan Bloom" <rb...@covalent.net> writes:
>
> > > > This is another attempt at fixing mod_autoindex. This works for
me
> > on
> > > > my computer, and it explains the problem. I haven't been able
to
> > > > actually reproduce the problem, but I have seen the symptoms.
Can
> > > > somebody who can reproduce PLEASE test this on their setup. I
am
> > hoping
> > > > to setup a failure condition soon so that I can test this
myself.
> > > >
> > > > I may commit this either way, because even if this doesn't fix
the
> > > > problem from apache.org, it does solve a problem with data
ordering.
> > > >
> > > > This is the cleanest way I could find to solve the problem, but
I am
> > > > perfectly willing to listen to other ideas.
> > >
> > > Greg and I discussed this issue (create subrequest, create output
via
> > > ap_rXXX, run subrequest) a couple of days ago. What we preferred
> > > overall was to require an app to call something like ap_rinit() if
> > > they were going to use ap_rXXX functions. ap_rinit() would add
> > > old_write filter. ap_rinit() would have to be called at the start
of
> > > content generation, before creating any subrequests.
> > >
> > > Yes it is ugly, but yes it is very simple to implement and keep
> > > working, and yes it is easy to give the right feedback to the app
if
> > > they forget to call ap_rinit() (return 500 and log an error if
> > > buffer_output is called with no old_write filter; currently if we
get
> > > to buffer_output with no old_write filter we add it).
> > >
> > > This seems to be a reasonable worse-is-better tradeoff. It is
easier
> > > for an app writer to do something like call ap_rinit() than it is
to
> > > do a trick like in your patch. It is easier for us to maintain
such
> > > an API too, which in turn makes life simpler for the app writer.
> >
> > The problem I have with that is that in 99% of the cases, the call
to
> > ap_rinit is unnecessary. The whole point of the ap_r* functions was
to
> > provide a level of source backwards compatibility with Apache 1.3,
so
> > that developers wouldn't have to modify their modules. If you force
> > them all to run ap_rinit before they can run ap_r*, then you have
> > completely defeated that goal.
>
> It is a hindrance that can be solved in a couple of minutes (as long
> as they read the error_log). This is similar in scale to the
> hindrance of needing to remove the call to ap_send_http_header().
>
> > And, as I have said, this is an edge case. If you look at all of
the
> > other places in the code that we create a sub-request, we always
create
> > it and run it. This module happens to output some data in between
those
> > two operations. I don't think we want to force all module authors
to
> > jump through hoops for an edge case.
> >
> > We can create the function, and advise its use, but we should not
> > mandate it.
>
> A benefit of mandating it is that it ensures that a module with the
> pattern seen in mod_autoindex isn't broken (because it forgot to call
> ap_rinit()). And the cost to the app writer is trivial. But either
> way (mandate or not), it would seem to be a simple change for Apache
> and for the module.
>
> --
> Jeff Trawick | trawick@attglobal.net
> Born in Roswell... married an alien...
Re: [PATCH] Fix mod_autoindex
Posted by Jeff Trawick <tr...@attglobal.net>.
(This is really from me :) )
"Ryan Bloom" <rb...@covalent.net> writes:
> > > This is another attempt at fixing mod_autoindex. This works for me
> on
> > > my computer, and it explains the problem. I haven't been able to
> > > actually reproduce the problem, but I have seen the symptoms. Can
> > > somebody who can reproduce PLEASE test this on their setup. I am
> hoping
> > > to setup a failure condition soon so that I can test this myself.
> > >
> > > I may commit this either way, because even if this doesn't fix the
> > > problem from apache.org, it does solve a problem with data ordering.
> > >
> > > This is the cleanest way I could find to solve the problem, but I am
> > > perfectly willing to listen to other ideas.
> >
> > Greg and I discussed this issue (create subrequest, create output via
> > ap_rXXX, run subrequest) a couple of days ago. What we preferred
> > overall was to require an app to call something like ap_rinit() if
> > they were going to use ap_rXXX functions. ap_rinit() would add
> > old_write filter. ap_rinit() would have to be called at the start of
> > content generation, before creating any subrequests.
> >
> > Yes it is ugly, but yes it is very simple to implement and keep
> > working, and yes it is easy to give the right feedback to the app if
> > they forget to call ap_rinit() (return 500 and log an error if
> > buffer_output is called with no old_write filter; currently if we get
> > to buffer_output with no old_write filter we add it).
> >
> > This seems to be a reasonable worse-is-better tradeoff. It is easier
> > for an app writer to do something like call ap_rinit() than it is to
> > do a trick like in your patch. It is easier for us to maintain such
> > an API too, which in turn makes life simpler for the app writer.
>
> The problem I have with that is that in 99% of the cases, the call to
> ap_rinit is unnecessary. The whole point of the ap_r* functions was to
> provide a level of source backwards compatibility with Apache 1.3, so
> that developers wouldn't have to modify their modules. If you force
> them all to run ap_rinit before they can run ap_r*, then you have
> completely defeated that goal.
It is a hindrance that can be solved in a couple of minutes (as long
as they read the error_log). This is similar in scale to the
hindrance of needing to remove the call to ap_send_http_header().
> And, as I have said, this is an edge case. If you look at all of the
> other places in the code that we create a sub-request, we always create
> it and run it. This module happens to output some data in between those
> two operations. I don't think we want to force all module authors to
> jump through hoops for an edge case.
>
> We can create the function, and advise its use, but we should not
> mandate it.
A benefit of mandating it is that it ensures that a module with the
pattern seen in mod_autoindex isn't broken (because it forgot to call
ap_rinit()). And the cost to the app writer is trivial. But either
way (mandate or not), it would seem to be a simple change for Apache
and for the module.
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
RE: [PATCH] Fix mod_autoindex
Posted by Ryan Bloom <rb...@covalent.net>.
> > This is another attempt at fixing mod_autoindex. This works for me
on
> > my computer, and it explains the problem. I haven't been able to
> > actually reproduce the problem, but I have seen the symptoms. Can
> > somebody who can reproduce PLEASE test this on their setup. I am
hoping
> > to setup a failure condition soon so that I can test this myself.
> >
> > I may commit this either way, because even if this doesn't fix the
> > problem from apache.org, it does solve a problem with data ordering.
> >
> > This is the cleanest way I could find to solve the problem, but I am
> > perfectly willing to listen to other ideas.
>
> Greg and I discussed this issue (create subrequest, create output via
> ap_rXXX, run subrequest) a couple of days ago. What we preferred
> overall was to require an app to call something like ap_rinit() if
> they were going to use ap_rXXX functions. ap_rinit() would add
> old_write filter. ap_rinit() would have to be called at the start of
> content generation, before creating any subrequests.
>
> Yes it is ugly, but yes it is very simple to implement and keep
> working, and yes it is easy to give the right feedback to the app if
> they forget to call ap_rinit() (return 500 and log an error if
> buffer_output is called with no old_write filter; currently if we get
> to buffer_output with no old_write filter we add it).
>
> This seems to be a reasonable worse-is-better tradeoff. It is easier
> for an app writer to do something like call ap_rinit() than it is to
> do a trick like in your patch. It is easier for us to maintain such
> an API too, which in turn makes life simpler for the app writer.
The problem I have with that is that in 99% of the cases, the call to
ap_rinit is unnecessary. The whole point of the ap_r* functions was to
provide a level of source backwards compatibility with Apache 1.3, so
that developers wouldn't have to modify their modules. If you force
them all to run ap_rinit before they can run ap_r*, then you have
completely defeated that goal.
And, as I have said, this is an edge case. If you look at all of the
other places in the code that we create a sub-request, we always create
it and run it. This module happens to output some data in between those
two operations. I don't think we want to force all module authors to
jump through hoops for an edge case.
We can create the function, and advise its use, but we should not
mandate it.
>
> > Index: modules/generators/mod_autoindex.c
> > ===================================================================
> > RCS file: /home/cvs/httpd-2.0/modules/generators/mod_autoindex.c,v
> > retrieving revision 1.102
> > diff -u -d -b -w -u -r1.102 mod_autoindex.c
> > --- modules/generators/mod_autoindex.c 20 Mar 2002 17:41:54 -0000
> > 1.102
> > +++ modules/generators/mod_autoindex.c 5 Apr 2002 06:26:08 -0000
> > @@ -1017,6 +1017,7 @@
> > if (rr->content_type != NULL) {
> > if (!strcasecmp(ap_field_noparam(r->pool,
> > rr->content_type),
> > "text/html")) {
> > + ap_filter_t *f;
> > /* Hope everything will work... */
> > emit_amble = 0;
> > emit_H1 = 0;
> > @@ -1024,6 +1025,22 @@
> > if (! suppress_amble) {
> > emit_preamble(r, title);
> > }
> > + /* This is a hack, but I can't find any better way
to
> > do this.
> > + * The problem is that we have already created the
> > sub-request,
> > + * but we just inserted the OLD_WRITE filter, and
the
> > + * sub-request needs to pass its data through the
> > OLD_WRITE
> > + * filter, or things go horribly wrong (missing
data,
> > data in
> > + * the wrong order, etc). To fix it, if you create
a
> > + * sub-request and then insert the OLD_WRITE filter
> > before you
> > + * run the request, you need to make sure that the
> > sub-request
> > + * data goes through the OLD_WRITE filter. Just
steal
> > this
> > + * code. The long-term solution is to remove the
ap_r*
> > + * functions.
> > + */
> > + for (f=rr->output_filters;
> > + f->frec != ap_subreq_core_filter_handle; f =
> > f->next);
> > + f->next = r->output_filters;
> > +
> > /*
> > * If there's a problem running the subrequest,
display
> > the
> > * preamble if we didn't do it before -- the header
> > file
>
> I didn't realize this was the only problem. I thought there was still
> an issue of getting the subrequest filter in place in certain
> conditions.
I have been unable to reproduce that. If I can get the permissions to
debug the server running on daedalus, I can most likely find and fix
that later today. My problem right now is that I'm not seeing the same
things you are in the tests.
Finally, I would actually like proof that this does solve the problem we
are seeing. Have we tested this patch on daedalus yet?
> (also, can you fix your e-mail client to not wrap patches at column
72?)
Yeah, LookOut really sucks, but I am working on it.
Ryan
Re: [PATCH] Fix mod_autoindex
Posted by Jeff Trawick <tr...@attglobal.net>.
"Ryan Bloom" <rb...@covalent.net> writes:
> This is another attempt at fixing mod_autoindex. This works for me on
> my computer, and it explains the problem. I haven't been able to
> actually reproduce the problem, but I have seen the symptoms. Can
> somebody who can reproduce PLEASE test this on their setup. I am hoping
> to setup a failure condition soon so that I can test this myself.
>
> I may commit this either way, because even if this doesn't fix the
> problem from apache.org, it does solve a problem with data ordering.
>
> This is the cleanest way I could find to solve the problem, but I am
> perfectly willing to listen to other ideas.
Greg and I discussed this issue (create subrequest, create output via
ap_rXXX, run subrequest) a couple of days ago. What we preferred
overall was to require an app to call something like ap_rinit() if
they were going to use ap_rXXX functions. ap_rinit() would add
old_write filter. ap_rinit() would have to be called at the start of
content generation, before creating any subrequests.
Yes it is ugly, but yes it is very simple to implement and keep
working, and yes it is easy to give the right feedback to the app if
they forget to call ap_rinit() (return 500 and log an error if
buffer_output is called with no old_write filter; currently if we get
to buffer_output with no old_write filter we add it).
This seems to be a reasonable worse-is-better tradeoff. It is easier
for an app writer to do something like call ap_rinit() than it is to
do a trick like in your patch. It is easier for us to maintain such
an API too, which in turn makes life simpler for the app writer.
> Index: modules/generators/mod_autoindex.c
> ===================================================================
> RCS file: /home/cvs/httpd-2.0/modules/generators/mod_autoindex.c,v
> retrieving revision 1.102
> diff -u -d -b -w -u -r1.102 mod_autoindex.c
> --- modules/generators/mod_autoindex.c 20 Mar 2002 17:41:54 -0000
> 1.102
> +++ modules/generators/mod_autoindex.c 5 Apr 2002 06:26:08 -0000
> @@ -1017,6 +1017,7 @@
> if (rr->content_type != NULL) {
> if (!strcasecmp(ap_field_noparam(r->pool,
> rr->content_type),
> "text/html")) {
> + ap_filter_t *f;
> /* Hope everything will work... */
> emit_amble = 0;
> emit_H1 = 0;
> @@ -1024,6 +1025,22 @@
> if (! suppress_amble) {
> emit_preamble(r, title);
> }
> + /* This is a hack, but I can't find any better way to
> do this.
> + * The problem is that we have already created the
> sub-request,
> + * but we just inserted the OLD_WRITE filter, and the
> + * sub-request needs to pass its data through the
> OLD_WRITE
> + * filter, or things go horribly wrong (missing data,
> data in
> + * the wrong order, etc). To fix it, if you create a
> + * sub-request and then insert the OLD_WRITE filter
> before you
> + * run the request, you need to make sure that the
> sub-request
> + * data goes through the OLD_WRITE filter. Just steal
> this
> + * code. The long-term solution is to remove the ap_r*
> + * functions.
> + */
> + for (f=rr->output_filters;
> + f->frec != ap_subreq_core_filter_handle; f =
> f->next);
> + f->next = r->output_filters;
> +
> /*
> * If there's a problem running the subrequest, display
> the
> * preamble if we didn't do it before -- the header
> file
I didn't realize this was the only problem. I thought there was still
an issue of getting the subrequest filter in place in certain
conditions.
(also, can you fix your e-mail client to not wrap patches at column 72?)
--
Jeff Trawick | trawick@attglobal.net
Born in Roswell... married an alien...
RE: [PATCH] Fix mod_autoindex
Posted by Ryan Bloom <rb...@covalent.net>.
I'll try this on my test machine today.
Ryan
----------------------------------------------
Ryan Bloom rbb@covalent.net
645 Howard St. rbb@apache.org
San Francisco, CA
> -----Original Message-----
> From: trawick@rdu88-250-035.nc.rr.com [mailto:trawick@rdu88-250-
> 035.nc.rr.com] On Behalf Of Jeff Trawick
> Sent: Friday, April 05, 2002 7:56 AM
> To: dev@httpd.apache.org
> Cc: rbb@covalent.net
> Subject: Re: [PATCH] Fix mod_autoindex
>
> (This is a reply from gregames@us.ibm.com... His normal e-mail access
> is fsck-ed at the moment. I'm just the messenger.)
>
> "Ryan Bloom" <rb...@covalent.net> writes:
>
> > This is another attempt at fixing mod_autoindex. This works for me
on
> > my computer, and it explains the problem. I haven't been able to
> > actually reproduce the problem, but I have seen the symptoms. Can
> > somebody who can reproduce PLEASE test this on their setup. I am
hoping
> > to setup a failure condition soon so that I can test this myself.
>
> the following from apache.org's config file might help:
>
> <IfModule mod_autoindex.c>
>
> # If MultiViews are amongst the Options in effect, the server will
> # first look for name.html and include it if found. If name.html
> # doesn't exist, the server will then look for name.txt and
> # include
> # it as plaintext if found.
> #
> ReadmeName README
> HeaderName HEADER
>
> </IfModule>
>
> If you want to get rid of the dependency on
> MultiViews/mod_negotiation,
>
> ReadmeName README.html
> ReadmeName HEADER.html
>
> ..would probably work.
>
> > I may commit this either way, because even if this doesn't fix the
> > problem from apache.org, it does solve a problem with data ordering.
> >
> > This is the cleanest way I could find to solve the problem, but I am
> > perfectly willing to listen to other ideas.
>
> I see three problems with mod_autoindex in 2.0.34 when it has to serve
> both a HEADER and README file (highest priority first):
>
> 1. the subrequest filter has to be present for normal subrequests that
> generate output,
>
> 2. if the main request does ap_rputs, then creates & runs a
> subrequest, the subrequest filter and any resource filters need to
> be inserted on top of the main request's OLD_WRITE instance so the
> older ap_rputs data gets flushed, and
>
> 3. (the problem that Ryan's patch is attempting to fix) if a module
> creates a subrequest, then does ap_r* for the first time, then runs
> the subrequest, you get out of order data. This problem pre-dates
> 2.0.34.
>
> You can see problem #3 on the production server at
> http://www.apache.org/dist/httpd/ . The page displays OK, but if you
> view the html source, notice that the <h1> thru the 2.0.32 beta
> announcement (from the HEADER.html file) is in front of <!DOCTYPE thru
> <body> (the first content that mod_autoindex generated via ap_rputs).
>
> I think the first thing we need is something like Jeff's patch for
> inserting the subrequest filter, reworked to accomodate the "promoted"
> subrequests like DirectoryIndex.
>
> #2 is what produces the visible out-of-order data at the bottom of the
> autoindex display in 2.0.34. I suspect this happens because we now
call
> OLD_WRITE a resource filter, but that's just a guess. I'll volunteer
> to analyze this more closely.
>
> Then we need something for #3. IMO it is low priority because it has
> existed for ages, and isn't visible AFAIK. Clearly we should address
> it before GA.
>
> Greg
>
> p.s. I've been without Apache e-mail for a couple of days due to a
> server upgrade. I posted a detailed analysis of #3 from an alternate
> e-mail address, but still don't see it.
Re: [PATCH] Fix mod_autoindex
Posted by Jeff Trawick <tr...@attglobal.net>.
(This is a reply from gregames@us.ibm.com... His normal e-mail access
is fsck-ed at the moment. I'm just the messenger.)
"Ryan Bloom" <rb...@covalent.net> writes:
> This is another attempt at fixing mod_autoindex. This works for me on
> my computer, and it explains the problem. I haven't been able to
> actually reproduce the problem, but I have seen the symptoms. Can
> somebody who can reproduce PLEASE test this on their setup. I am hoping
> to setup a failure condition soon so that I can test this myself.
the following from apache.org's config file might help:
<IfModule mod_autoindex.c>
# If MultiViews are amongst the Options in effect, the server will
# first look for name.html and include it if found. If name.html
# doesn't exist, the server will then look for name.txt and
# include
# it as plaintext if found.
#
ReadmeName README
HeaderName HEADER
</IfModule>
If you want to get rid of the dependency on
MultiViews/mod_negotiation,
ReadmeName README.html
ReadmeName HEADER.html
..would probably work.
> I may commit this either way, because even if this doesn't fix the
> problem from apache.org, it does solve a problem with data ordering.
>
> This is the cleanest way I could find to solve the problem, but I am
> perfectly willing to listen to other ideas.
I see three problems with mod_autoindex in 2.0.34 when it has to serve
both a HEADER and README file (highest priority first):
1. the subrequest filter has to be present for normal subrequests that
generate output,
2. if the main request does ap_rputs, then creates & runs a
subrequest, the subrequest filter and any resource filters need to
be inserted on top of the main request's OLD_WRITE instance so the
older ap_rputs data gets flushed, and
3. (the problem that Ryan's patch is attempting to fix) if a module
creates a subrequest, then does ap_r* for the first time, then runs
the subrequest, you get out of order data. This problem pre-dates
2.0.34.
You can see problem #3 on the production server at
http://www.apache.org/dist/httpd/ . The page displays OK, but if you
view the html source, notice that the <h1> thru the 2.0.32 beta
announcement (from the HEADER.html file) is in front of <!DOCTYPE thru
<body> (the first content that mod_autoindex generated via ap_rputs).
I think the first thing we need is something like Jeff's patch for
inserting the subrequest filter, reworked to accomodate the "promoted"
subrequests like DirectoryIndex.
#2 is what produces the visible out-of-order data at the bottom of the
autoindex display in 2.0.34. I suspect this happens because we now call
OLD_WRITE a resource filter, but that's just a guess. I'll volunteer
to analyze this more closely.
Then we need something for #3. IMO it is low priority because it has
existed for ages, and isn't visible AFAIK. Clearly we should address
it before GA.
Greg
p.s. I've been without Apache e-mail for a couple of days due to a
server upgrade. I posted a detailed analysis of #3 from an alternate
e-mail address, but still don't see it.