httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ron Park <ronald.p...@cnet.com>
Subject RE: Two mod_include problems
Date Wed, 09 Jul 2003 20:03:05 GMT
Oops, I apologize.

I accidently included another minor enhancement in this patch.

At line 3141 of the code, there was some code to call the
apr_brigade_cleanup method if the context's ssi_tag_brigade
is not empty... but then there was another exact same call
to apr_brigade_cleanup like 20 lines later.  It seemed very
redundant.

If anyone knows why this was done and it is important, I can
provide a patch for the two bugs without this change.

Thanks,
Ron

> -----Original Message-----
> From: Ron Park [mailto:ronald.park@cnet.com] 
> Sent: Wednesday, July 09, 2003 3:50 PM
> To: 'dev@httpd.apache.org'
> Subject: Two mod_include problems
> 
> 
> We have run across two problems in mod_include that were
> corrupting our pages and would like to share our patches.
> 
> Both of these situations are highlighted by situations where
> bucketeering and brigading are accentuated (mostly through
> the use of mod_proxy to a remote host, but should be testable
> with simple includes of appropriately sized (8K+) files).
> 
> The first problem was appearing as a few (important) chars
> being truncated right before the occurrence of a virtual
> include.  The situation was heightened by heavy use of
> mod_proxy to retrieve these files and the smaller brigade
> size that employs (about 1.5K).
> 
> After snooping the results of the proxy request, we verified
> that the returned file was looking just fine with various
> calls to <!--#include virtual... --> ... but after processing
> by mod_include, some of the (valid) includes where failing.
> 
> In particular, we tracked down a case where the start of a
> comment at the very end of the proceeding bucket would cause
> a miscount of the start of the location of the include in
> the current bucket.
> 
> As an example, consider a bucket that ends as follows:
> 
> ... <!
> 
> and the new bucket starts as
> 
> -- some comment --><img src="a.gif"><!--#include ...
> 
> The problem we were seeing in the mod_include parsed page was
> that the two chars proceeding this include were being eaten.
> So, we would see the following string:
> 
> ... <!-- some comment --><img src="a.gif<!-- missing include -->
> 
> As you might imagine, the missing end quote was troublesome. :)
> (Not to mention the bad include...)
> 
> At line 438 of mod_include, we see the code is set up to handle
> this 'left over partial match' and it increments the temporary
> buffer pointer, c, by 2 as it walks past the '--' at the start
> of this bucket.  So it (as far as I could determine) adds the
> left over '<!' to this bucket and continues on, calling the
> quick scanner function, bndm(), passing in the recently updated
> parameter 'c'.  The result of this is 'pos', an offset into
> the bucket supposedly representing the start of the include...
> but it's off by 2!  Later, it proceeds to 'split' the bucket
> at what it thinks is the start of the include (but is actually
> the '">' from the proceeding tag) and fails to figure out what
> include command is being requested, hence the 'missing' comment.
> 
> This patch which doesn't really fix the problem but corrects the
> resulting problem... we hope someone with a better understanding
> of mod_include and buckets can come up with a true solution.
> 
> ======
> 
> The second problem actually occurs across brigades (we learned
> an awful lot about buckets and brigades in diagnosing these
> problems! :D)  And, instead of removing characters was instead
> adding large extra chunks.
> 
> This problem revolves around the use of 'if' 'else' and 'elseif'.
> Originally, we thought the problem was related to <!--#else -->
> but later we found an troublesome <!--#if ... --> that lead to
> us finding the real culprit.
> 
> As an example, consider the following example:
> 
> ... <!--#if ... -->Some if text ... end if text <!--#endif -->
> 
> The problem reveals itself if the 'Some if text ...' is in one
> brigade and the 'end if text' is in the next *and* the if's
> condition is false (hence the if's conditional body should
> *not* be present after mod_include works it's magic).
> 
> What we found is that we were getting 'Some if text ...' out
> of mod_include... but not the 'end if text' when the condition
> was false. :(
> 
> We finally tracked the problem down because of the comments
> on line 3200 of mod_include.c:
> 
>         /* Inside a false conditional (if, elif, else), so 
> toss it all... */
>         if ((dptr != APR_BRIGADE_SENTINEL(*bb)) &&
>             (!(ctx->flags & FLAG_PRINTING))) {
>             apr_bucket *free_bucket;
>             do {
>                 free_bucket = dptr;
>                 dptr = APR_BUCKET_NEXT (dptr);
>                 apr_bucket_delete(free_bucket);
>             } while (dptr != APR_BRIGADE_SENTINEL(*bb));
>         }
> 
> Rightly so, this loop makes sure it doesn't throw out the
> brigade's sentinel.  Surely this would be a 'bad thing'.
> 
> However, earlier in the code (3015 ... 3044), we have this:
> 
>             if (tmp_dptr != NULL) {
>                 ...
>             }
>             else {
>                  /* remainder of this brigade...    */
>                 dptr = APR_BRIGADE_SENTINEL(*bb);
>             }
> 
> And tmp_dptr, is set via line 2937:
> 
>             tmp_dptr = find_start_sequence(dptr, ctx, *bb, 
> &do_cleanup);
>  
> But since there's no start sequences in the if's conditional
> text, tmp_dptr is set to NULL (left as an exercise for the
> reader :).
> 
> So the code would get to line 3200... and dptr would *always*
> be pointing at the sentinel. That just clearly seems wrong.
> The do loop would never be executed and the <!--#if-->'s body
> would never be thrown away.  Exactly what we were seeing. :(
> 
> Once again, we don't claim the patch really fixes the problem;
> it just tries to correct the situation we were seeing in the
> most minimal way.  In fact, we kept (what we think is) the 'bad'
> code in case there's some strange scenario where the code really
> is good.
> 
> Attached is a single patch to fix both problems.
> 
> 

Mime
View raw message