Here's a patch (against the current CVS head) that addresses the two
problems I know about:
* The ctx->tag_length computation in find_end_sequence() was a bit
broken in cases where there was a "false alarm" match on a partial
"-->"
* The ap_ssi_get_tag_and_value() function needs to avoid walking off
the end of the string. After debugging this some more, I ended up
using Cliff's original patch.
With this patch, both the current CVS head and the bucket_allocator-patched
httpd pass all but one of the mod_include tests in httpd-test. The test
that
reports a failure is the if6.shtml one. It's expecting three instances of
"[an error occurred..." but only two are output. I believe the third one
that the test expects is due to an imbalanced "<!--#endif-->". But based
on the code, I wouldn't expect to see the "[an error occurred..." message
for this situation.
Cliff and Paul (and anyone else with good test cases for this stuff), can
you test this patch against your test cases?
Thanks,
--Brian
Paul J. Reder wrote:
> Brian,
>
> Please give me a chance to fix this. I indicated that I was looking
> at this problem. There is no reason to duplicate work. I have identified
> several problems and am working on fixes for them. I should have
> something
> tested and ready by the end of day on Thursday or Friday during the day
> at the latest.
>
> Paul J. Reder
>
>
> Paul J. Reder wrote:
>
>> Okay, I have recreated at least two problems in include processing, one
>> of which results in a core dump. I am in process of tracking them down.
>> It might be tomorrow before I have a patch.
>>
>> Paul J. Reder
>>
>> Paul J. Reder wrote:
>>
>>> Brian,
>>>
>>> I'm looking into this right now. I'll let you all know what I find out.
>>>
>>> I have some concerns about the suggested fix. I hope to have a fix
>>> by this afternoon.
>>>
>>> Paul J. Reder
>>>
>>> Brian Pane wrote:
>>>
>>>> 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
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
Index: modules/filters/mod_include.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.c,v
retrieving revision 1.208
diff -u -r1.208 mod_include.c
--- modules/filters/mod_include.c 28 Mar 2002 01:57:03 -0000 1.208
+++ modules/filters/mod_include.c 28 Mar 2002 04:58:30 -0000
@@ -652,10 +652,10 @@
ctx->state = PARSE_TAIL;
ctx->tail_start_bucket = dptr;
ctx->tail_start_index = c - buf;
- ctx->tag_length += ctx->parse_pos;
ctx->parse_pos = 1;
}
else {
+ ctx->tag_length++;
if (ctx->tag_length > ctx->directive_length) {
ctx->state = PARSE_TAG;
}
@@ -665,7 +665,6 @@
}
ctx->tail_start_bucket = NULL;
ctx->tail_start_index = 0;
- ctx->tag_length += ctx->parse_pos;
ctx->parse_pos = 0;
}
}
@@ -867,6 +866,10 @@
char term = '\0';
*tag_val = NULL;
+ if (ctx->curr_tag_pos - ctx->combined_tag > ctx->tag_length) {
+ *tag = NULL;
+ return;
+ }
SKIP_TAG_WHITESPACE(c);
*tag = c; /* First non-whitespace character (could be NULL). */
