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). */