Re: cvs commit: apache-1.3/src/modules/standard mod_autoindex.c mod_include.c

[Raymond S Brand <rs...@rsbx.net>]

Dean Gaudet wrote:
> 
> On Thu, 13 May 1999, Raymond S Brand wrote:
> 
> > Dean Gaudet wrote:
> > >
> > > ap_pool_join is an advisory debugging tool -- alloc.c does nothing to
> > > ensure that you preserve the lifetimes appropriately.
> >
> > I understand that. Please apply  the patch to mod_include and look at
> > where the ap_make_sub_pool() I added is called.
> 
> Yeah I saw that:
> 
>         /* Kludge --- Doing this allows the caller to safely destroy the
>          * sub_req
>          */
>         r->pool = ap_make_sub_pool(r->pool);
> 
> Sorry, but that's just way off my bogosity meter.

No more bogus than the current situation in mod_include. That code path
is only executed for sub_reqs created by mod_include (works fine, they're
about to be destroyed) or for sub_reqs created in another module and flagged
that they should be treated as a sub_req created in mod_inlcude (again
works fine, they're also about to be destroyed).

> Consider this sequence:
> 
>     in module foo: my_p = ap_make_sub_pool(r->pool);
>                    my_table = ap_make_table(my_p, 5);
>     in your hacked mod_include: r->pool = ap_make_sub_pool(r->pool);
>     in module foo: x = ap_pstrdup(r->pool, blahblahblah);
>                    ap_table_setn(my_table, "abc", x);
> 
> That's broken.  And module foo isn't what's broken, it's doing something
> that fits within the ancestor relationship... only the ancestry has been
> broken.  Although I think the damage really only becomes apparent in
> a cleanup.

I fail to see how the above call sequence can happen. Can you flesh it out
some? And are there any ap_*sub_req*() calls in the sequence?

> Let's just say I'm going to take a lot more convincing that futzing with
> r->pool is fine.  In general I consider that to be a read-only field.
> You'd certainly screw up if we had contexts in the pool like we're
> talking about for 2.0.

It may not survive with contexts in 2.0, but it's not clear that the existing
mod_include can coexist with contexts in 2.0 either.

The sub_pool business is ONLY there so that a sub_req can be destroyed. I'm
tired of this argument. Use the following;

In mod_include:
@@ -2376,6 +2365,38 @@
         return OK;
     }
 
+#define SUB_REQ_STRING "Sub request to mod_include"
+#define PARENT_STRING  "Parent request to mod_include"
+
+    if (ap_table_get(r->notes, SUB_REQ_STRING)) {
+       request_rec *p = r->main;
+       /* The note is a flag to mod_include that this request is actually
+        * a subrequest from another module and that mod_include needs to
+        * treat it as if it's a subrequest from mod_include.
+        *
+        * HACK ALERT!
+        * There is no good way to pass the parent request_rec to mod_include.
+        * Tables only take string values and there is nowhere appropriate in
+        * in the request_rec that can safely be used.
+        *
+        * So we search up the chain of requests and redirects looking for
+        * the parent request.
+        */
+       while (p) {
+           if (ap_table_get(p->notes, PARENT_STRING)) {
+               /* Kludge --- See below */
+               ap_set_module_config(r->request_config, &includes_module, p);
+
+               ap_add_common_vars(p);
+               ap_add_cgi_vars(p);
+               add_include_vars(p, DEFAULT_TIME_FORMAT);
+               ap_table_unset(r->notes, SUB_REQ_STRING);
+               break;
+           }
+           p = (p->prev) ? p->prev : p->main;
+       }
+    }
+
     if ((parent = ap_get_module_config(r->request_config, &includes_module))) {
        /* Kludge --- for nested includes, we want to keep the subprocess
         * environment of the base document (for compatibility); that means


In mod_autoindex:
@@ -924,6 +924,9 @@
      ap_rputs("</PRE>\n", r);
  }
  
+ /* See mod_include */
+ #define SUB_REQ_STRING  "Sub request to mod_include"
+ #define PARENT_STRING   "Parent request to mod_include"
  
  /*
   * Handle the preamble through the H1 tag line, inclusive.  Locate
@@ -969,6 +972,8 @@
                  if (! suppress_amble) {
                      emit_preamble(r, title);
                  }
+                 ap_table_add(r->notes, PARENT_STRING, "");
+                 ap_table_add(rr->notes, SUB_REQ_STRING, "");
                  /*
                   * If there's a problem running the subrequest, display the
                   * preamble if we didn't do it before -- the header file
@@ -1006,9 +1011,6 @@
      if (emit_H1) {
          ap_rvputs(r, "<H1>Index of ", title, "</H1>\n", NULL);
      }
-     if (rr != NULL) {
-         ap_destroy_sub_req(rr);
-     }
  }
  
  
@@ -1045,6 +1047,8
           */
          if (rr->content_type != NULL) {
             if (!strcasecmp("text/html", rr->content_type)) {
+                 ap_table_add(r->notes, PARENT_STRING, "");
+                 ap_table_add(rr->notes, SUB_REQ_STRING, "");
                  if (ap_run_sub_req(rr) == OK) {
                      /* worked... */
                      suppress_sig = 1;
@@ -1072,9 +1076,6 @@
      if (!suppress_post) {
          ap_rputs("</BODY></HTML>\n", r);
      }
-     if (rr != NULL) {
-         ap_destroy_sub_req(rr);
      }
  }
  
  


Raymond S Brand

Re: cvs commit: apache-1.3/src/modules/standard mod_autoindex.c mod_include.c

[Dean Gaudet <dg...@arctic.org>]

On Fri, 14 May 1999, Raymond S Brand wrote:

> Dean Gaudet wrote:
> >         /* Kludge --- Doing this allows the caller to safely destroy the
> >          * sub_req
> >          */
> >         r->pool = ap_make_sub_pool(r->pool);
> > 
> > Sorry, but that's just way off my bogosity meter.
> 
> No more bogus than the current situation in mod_include.

Not really, the current situation is that it avoids destroying the
subrequest... which has a similar effect, as you point out with the patch
you just included.  And further, it's local to mod_include. 

> I fail to see how the above call sequence can happen. Can you flesh it out
> some? And are there any ap_*sub_req*() calls in the sequence?

It would require some crap in cleanups I'm sure, so I'll stop stretching. 
But I'm certainly not going to ignore the 2.0 implications w.r.t. 
context... you can ignore them if you want, but I can't. 

> The sub_pool business is ONLY there so that a sub_req can be destroyed. I'm
> tired of this argument. Use the following;

I'm sorry you're tired of this argument, but I'm also of the opinion that
something is wrong with the subrequest mechanism and that there is an
abstraction we're missing... the stuff which is in mod_include already is
an example of something which isn't quite clean -- but is there to support
pre-existing functionality.  What you're talking about is new
functionality.

Maybe the abstraction is as simple as a "root environment" or "global
environment".  Or maybe we need to distinguish between a partial
subrequest and a full subrequest -- partial meaning "provides part of the
full response object".  I don't know.  I stopped hacking on this when I
got mod_include to work properly without memory corruption. 

There's still something else I brought up -- I think this is backwards.  I
don't think mod_autoindex should be bending over to behave like
mod_include.  I think mod_autoindex should provide a way to be included by
mod_include. 

Dean