You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@httpd.apache.org by Dirk-Willem van Gulik <di...@webweaving.org> on 2008/02/08 16:32:06 UTC

ap_sub_req_lookup_* loosing filter context

In a number of places we are usingL

		... = ap_sub_req_lookup_uri(..., r, NULL);
		... = ap_sub_req_lookup_file(..., r, NULL);
		... = ap_sub_req_lookup_dirent...., NULL);
		... = ap_sub_req_lookup_uri( ... ,NULL);

Where null is the output filter to preserve, if any (at NULL  
proto_output_filter is used).

We do this to see what a sub request would look like; and then later  
on we clean it up; perhaps after capturing some result, modified path,  
filling out some SSI etc (e.g. mod_include, mod_isapi, mod_auto_index,  
mod_cgid, mod_cern, mod_mime_magic or mod_rewrite). And then we  
discart the sub request and continue with the real request.

However we also occasionally do NOT do this - but make the thus  
created sub request as the main request  i.e. some redirect where we  
do not discard the sub_req -- but continue to operate on it (e.g. the  
core of mod_dir, mod_negotiation) - and pass it to a  
internal(fast)redirect.

Am I right to understand that this is not correct ?

As far as I can see (see patch below) - there is no harmful side  
effect from fixing this - while it does seem to fix mod_cache when  
dealing with content remappings done by mod_dir and mod_negotation.

Thoughts ?

Dw

Index: modules/mappers/mod_negotiation.c
===================================================================
--- modules/mappers/mod_negotiation.c	(revision 618646)
+++ modules/mappers/mod_negotiation.c	(working copy)
@@ -1165,8 +1165,10 @@

          /* Double check, we still don't multi-resolve non-ordinary  
files
           */
-        if (sub_req->finfo.filetype != APR_REG)
+        if (sub_req->finfo.filetype != APR_REG) {
+	    /* XXX sub req not destroyed */
              continue;
+	}

          /* If it has a handler, we'll pretend it's a CGI script,
           * since that's a good indication of the sort of thing it
@@ -3122,7 +3124,7 @@
           * a sub_req structure yet.  Get one now.
           */

-        sub_req = ap_sub_req_lookup_file(best->file_name, r, NULL);
+        sub_req = ap_sub_req_lookup_file(best->file_name, r, r- 
 >output_filter);
          if (sub_req->status != HTTP_OK) {
              res = sub_req->status;
              ap_destroy_sub_req(sub_req);
Index: modules/mappers/mod_dir.c
===================================================================
--- modules/mappers/mod_dir.c	(revision 618646)
+++ modules/mappers/mod_dir.c	(working copy)
@@ -176,7 +185,8 @@
              name_ptr = apr_pstrcat(r->pool, name_ptr, "?", r->args,  
NULL);
          }

-        rr = ap_sub_req_lookup_uri(name_ptr, r, NULL);
+        rr = ap_sub_req_lookup_uri(name_ptr, r, r->output_filters);

          /* The sub request lookup is very liberal, and the core  
map_to_storage
           * handler will almost always result in HTTP_OK as /foo/ 
index.html