httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Pane <bp...@pacbell.net>
Subject Re: mod_include bug(s)?
Date Wed, 27 Mar 2002 15:20:33 GMT
Cliff Woolley wrote:

>I've spent the entire evening chasing some wacky mod_include bugs that
>surfaced as I was doing final testing on the bucket API patch.  At first I
>assumed they were my fault, but upon further investigation I think the
>fact that they haven't surfaced until now is a coincidence.  There are two
>problems that I can see -- the if6.shtml and if7.shtml files I committed
>to httpd-test last week to check for the mod_include 1.3 bug has turned up
>quasi-related problems in mod_include 2.0 as well.
>
>Problem 1:
>
>When in an #if or #elif or several other kinds of tags,
>ap_ssi_get_tag_and_value() is called from within a while(1) loop that
>continues until that function returns with tag==NULL.  But in the case of
>if6.shtml, ap_ssi_get_tag_and_value() steps right past the end of the
>buffer and never bothers to check and see how long the thing it's supposed
>to be processing actually is.  The following patch fixes it, but there
>could be a better way to do it.  I'm hoping someone out there who knows
>this code better will come up with a better way to do it.
>
>Index: mod_include.c
>===================================================================
>RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.c,v
>retrieving revision 1.205
>diff -u -d -r1.205 mod_include.c
>--- mod_include.c       24 Mar 2002 06:42:14 -0000      1.205
>+++ mod_include.c       27 Mar 2002 06:41:55 -0000
>@@ -866,6 +866,11 @@
>     int   shift_val = 0;
>     char  term = '\0';
>
>+    if (ctx->curr_tag_pos - ctx->combined_tag > ctx->tag_length) {
>+        *tag = NULL;
>+        return;
>+    }
>+
>

My only objection to this is that ctx->curr_tag_pos is supposed
to point to a null-terminated copy of the directive, and all the
subsequent looping logic in ap_ssi_tag_and_value() depends on
that.  Are we hitting a case where this string isn't null-terminated
(meaning that the root cause of the problem is somewhere else)?


>
>     *tag_val = NULL;
>     SKIP_TAG_WHITESPACE(c);
>     *tag = c;             /* First non-whitespace character (could be NULL). */
>
>
>Problem 2:
>
>In the case of if7.shtml, for some reason, the null-terminating character
>is placed one character too far forward.  So instead of #endif you get
>#endif\b or some such garbage:
>
...

>Note the trailing \b in curr_tag_pos that shouldn't be there.
>
>I'm at a bit of a loss on this one.  I would think the problem must be in
>get_combined_directive(), but I could be wrong.  Again, more eyes would be
>appreciated.
>

I'm willing to take a look at this later today.  The only problem
is that I can't recreate this problem (or the first one) with the
latest CVS head of httpd-test and httpd-2.0.  Is there any special
configuration needed to trigger the bug?

--Brian




Mime
View raw message