You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Brian Pane <br...@cnet.com> on 2002/03/28 06:21:15 UTC
[PATCH] Re: mod_include bug(s)?
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
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
Re: [PATCH] Re: mod_include bug(s)?
Posted by Brian Pane <br...@cnet.com>.
Paul J. Reder wrote:
> That patch seems to solve at least one of the problems that I am seeing,
> but I have at least one other problem and a core dump inside
> send_parsed_content. I'm currently stepping though, trying to find the
> source of the core dump.
Thanks, I'll commit this patch for now
--Brian
Re: mod_include bug(s)?
Posted by "Paul J. Reder" <re...@remulak.net>.
Okay, I have found the source of the core dump that I have been
experiencing in mod_include. There are actually two problems that
are working together to cause this.
The first is that the code is too aggressive in getting rid of
buckets/brigades during a non-printing state (i.e. a false if/else
condition). The second relates to the case where the start sequence
ends at the end of a bucket and the tag starts at the beginning of
the next bucket.
I am working on this over the weekend as I travel and in my spare
time. If I can get connected I'll post something as soon as possible.
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein
Re: [PATCH] Re: mod_include bug(s)?
Posted by Cliff Woolley <jw...@virginia.edu>.
On Thu, 28 Mar 2002, Paul J. Reder wrote:
> That patch seems to solve at least one of the problems that I am seeing,
> but I have at least one other problem and a core dump inside
> send_parsed_content. I'm currently stepping though, trying to find the
> source of the core dump.
> I'll let you know what I find.
Thanks guys!
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: [PATCH] Re: mod_include bug(s)?
Posted by "Paul J. Reder" <re...@remulak.net>.
That patch seems to solve at least one of the problems that I am seeing,
but I have at least one other problem and a core dump inside
send_parsed_content. I'm currently stepping though, trying to find the
source of the core dump.
I'll let you know what I find.
Paul J. Reder
Brian Pane wrote:
> 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). */
>
>
> incl_patch.txt
>
> Content-Type:
>
> text/plain
> Content-Encoding:
>
> 7bit
>
>
--
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it. Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein