You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ian Holsman <ia...@apache.org> on 2001/12/14 18:32:11 UTC
Core dump in mod-include
hi guys.
we've been investigating a couple of core dumps in some testing and we
think we found one in mod-include.
here is the email thread.
does anyone have any reason NOT to reset the state when reseting
the bytes parsed ?
It seems to fix this problem, but I'm fearful that it will cause
something else to break...
>
> > - apparently, the brigade only has an EOS, it is not splittable.
> > But here mod_include is trying to do so, hence messed up
> > the brigade and
> > the later usage of *bb caused core.
> > Every time I saw, when mod_include reach that line, it cores soon.
> >
> > - Question is, why a brigade that only has EOS bucket can reach here?
> > If it supposed to be that way, how to split such a brigade
> > properly?
>
> I think it is valid for the brigade to have only EOS in that situation.
> The brigade has several buckets before the EOS when it enters the
> send_parsed_content function, but the buckets before the EOS may have
> been sent on to the next filter by the time it gets to the end.
>
> The strange thing, though, is that if you reach line 2981,it means that
> there was a partial SSI directive at the end of the content. And indeed
> the ctx->head_start_bucket appears to have an SSI directive at the start
> of its data. But two things are wrong: that bucket isn't on the
> brigade,
> and it looks like a complete (not partial) SSI directive.
Yeah, the PARSE_DIRECTIVE state shoule not reach that line for EOS.
Because the EOS is from ap_finalize_request_protocol(), it seems to
be the only bucket. So I checked the includes_filter().
It tried to reuse the filter's previous ctx in the begining,
but it only cleaned bytes_parsed.
if (!f->ctx) {
...create new ctx here...
}
else {
ctx->bytes_parsed = 0;
}
I added a line after the ctx->bytes_parsed = 0 to reset the state:
else {
ctx->bytes_parsed = 0;
/**/ ctx->state = PRE_HEAD;
}
It is running for over half hour now (with all SSI turned back on and
with dw, ad module turned back on), no more core dump for port 8001 !
(Port 80 still has some though, I am contacting Justin Wang to see
if I can do some tests there too.)
So the ctx->state not cleaned when reuse, is it on purpose?
Is this a good fix? (The problem still could be caused by something
before this...)
-- Jin
Re: Core dump in mod-include
Posted by "Paul J. Reder" <re...@remulak.net>.
Brian Pane wrote:
> I haven't had time to read through through Justin's patch in detail,
> but conceptually I think it's the right approach: check for the EOS
> condition as a special case in the end-of-brigade code that's crashing,
> rather than resetting the state every time we enter the filter.
Setting the state in the else (as Ian first suggested) is wrong. Tags
can span buckets or brigades. State must be maintained across those
boundaries. That is the whole reason for the state info.
Justin's patch looks much better. If the code runs into a premature
end, then it needs to give up and clean up.
--
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: Core dump in mod-include
Posted by Jin Hong <ji...@cnet.com>.
That's what I worried...
I applied Justin's patch, so far so good. So this is the fix we'll use.
BTW, by APR_BUCKET_INSERT_TAIL, I think you mean APR_BRIGADE_INSERT_TAIL
Thanks,
-- Jin
Brian Pane wrote:
>
> Justin Erenkrantz wrote:
>
> >On Fri, Dec 14, 2001 at 09:32:11AM -0800, Ian Holsman wrote:
> >
> >>hi guys.
> >>we've been investigating a couple of core dumps in some testing and we
> >>think we found one in mod-include.
> >>
> >>here is the email thread.
> >>does anyone have any reason NOT to reset the state when reseting
> >>the bytes parsed ?
> >>
> >
> >Just to make sure I understand what you guys are thinking of,
> >this is your proposed patch, right? (more follows)
> >
> >Index: modules/filters/mod_include.c
> >===================================================================
> >RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.c,v
> >retrieving revision 1.166
> >diff -u -r1.166 mod_include.c
> >--- modules/filters/mod_include.c 2001/12/08 03:14:50 1.166
> >+++ modules/filters/mod_include.c 2001/12/14 18:12:31
> >@@ -3066,6 +3066,7 @@
> > }
> > else {
> > ctx->bytes_parsed = 0;
> >+ ctx->state = PRE_HEAD;
> > }
> >
> > if ((parent = ap_get_module_config(r->request_config, &include_module))) {
> >
> >If so, then I'm kind of leery about this.
> >
> >The reason is that filters can be called many times over the
> >lifetime of the data. So, we could leave the parse engine in a
> >ctx->state = POST_HEAD between calls because we are still waiting
> >for a bucket-brigade containing the rest of the tag. We'd obliterate
> >that value with this. =) So, I don't think resetting the state is
> >going to do you any good here.
> >
> >Based on your description, what seems to be happening is that the
> >client either disconnects or timeouts and the finalize_request is
> >passing down an EOS but mod_include is stuck in some internal
> >state. mod_include just isn't handling it well.
> >
> >So, I'm thinking the appropriate check is to stop at EOS or
> >SENTINEL. There is no good reason to continue processing if we
> >see an EOS. But, we have a potentially saved brigade to handle.
> >So, what do we do? Perhaps this patch might be better:
> >
>
> I haven't had time to read through through Justin's patch in detail,
> but conceptually I think it's the right approach: check for the EOS
> condition as a special case in the end-of-brigade code that's crashing,
> rather than resetting the state every time we enter the filter.
>
> --Brian
Re: Core dump in mod-include
Posted by Brian Pane <br...@cnet.com>.
Justin Erenkrantz wrote:
>On Fri, Dec 14, 2001 at 09:32:11AM -0800, Ian Holsman wrote:
>
>>hi guys.
>>we've been investigating a couple of core dumps in some testing and we
>>think we found one in mod-include.
>>
>>here is the email thread.
>>does anyone have any reason NOT to reset the state when reseting
>>the bytes parsed ?
>>
>
>Just to make sure I understand what you guys are thinking of,
>this is your proposed patch, right? (more follows)
>
>Index: modules/filters/mod_include.c
>===================================================================
>RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.c,v
>retrieving revision 1.166
>diff -u -r1.166 mod_include.c
>--- modules/filters/mod_include.c 2001/12/08 03:14:50 1.166
>+++ modules/filters/mod_include.c 2001/12/14 18:12:31
>@@ -3066,6 +3066,7 @@
> }
> else {
> ctx->bytes_parsed = 0;
>+ ctx->state = PRE_HEAD;
> }
>
> if ((parent = ap_get_module_config(r->request_config, &include_module))) {
>
>If so, then I'm kind of leery about this.
>
>The reason is that filters can be called many times over the
>lifetime of the data. So, we could leave the parse engine in a
>ctx->state = POST_HEAD between calls because we are still waiting
>for a bucket-brigade containing the rest of the tag. We'd obliterate
>that value with this. =) So, I don't think resetting the state is
>going to do you any good here.
>
>Based on your description, what seems to be happening is that the
>client either disconnects or timeouts and the finalize_request is
>passing down an EOS but mod_include is stuck in some internal
>state. mod_include just isn't handling it well.
>
>So, I'm thinking the appropriate check is to stop at EOS or
>SENTINEL. There is no good reason to continue processing if we
>see an EOS. But, we have a potentially saved brigade to handle.
>So, what do we do? Perhaps this patch might be better:
>
I haven't had time to read through through Justin's patch in detail,
but conceptually I think it's the right approach: check for the EOS
condition as a special case in the end-of-brigade code that's crashing,
rather than resetting the state every time we enter the filter.
--Brian
Re: Core dump in mod-include
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Fri, Dec 14, 2001 at 04:44:26PM -0800, Brian Pane wrote:
> Thanks, this patch seems to fix the problem. One question,
> though: why remove the bucket and then insert it at the end
> of the same brigade? Is that just in case there's an EOS in
> the middle of the brigade?
Oh, yeah, the EOS is coming from *bb. You could remove it, I
guess. I was barely awake when I wrote that patch. And, I'm
barely awake as I write this. =) -- justin
Re: Core dump in mod-include
Posted by Cliff Woolley <cl...@yahoo.com>.
On Fri, 14 Dec 2001, Brian Pane wrote:
> Thanks, this patch seems to fix the problem. One question,
> though: why remove the bucket and then insert it at the end
> of the same brigade? Is that just in case there's an EOS in
> the middle of the brigade?
EOS buckets aren't allowed in the middle of a brigade. If there is one,
it's a bug.
--Cliff
--------------------------------------------------------------
Cliff Woolley
cliffwoolley@yahoo.com
Charlottesville, VA
Re: Core dump in mod-include
Posted by Brian Pane <br...@cnet.com>.
Justin Erenkrantz wrote:
>So, I'm thinking the appropriate check is to stop at EOS or
>SENTINEL. There is no good reason to continue processing if we
>see an EOS. But, we have a potentially saved brigade to handle.
>So, what do we do? Perhaps this patch might be better:
>
>Index: modules/filters/mod_include.c
>===================================================================
>RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.c,v
>retrieving revision 1.166
>diff -u -r1.166 mod_include.c
>--- modules/filters/mod_include.c 2001/12/08 03:14:50 1.166
>+++ modules/filters/mod_include.c 2001/12/14 18:37:48
>@@ -2700,7 +2700,7 @@
> ap_escape_shell_cmd(r->pool, arg_copy));
> }
>
>- while (dptr != APR_BRIGADE_SENTINEL(*bb)) {
>+ while (dptr != APR_BRIGADE_SENTINEL(*bb) || !APR_BUCKET_IS_EOS(dptr)) {
> /* State to check for the STARTING_SEQUENCE. */
> if ((ctx->state == PRE_HEAD) || (ctx->state == PARSE_HEAD)) {
> int do_cleanup = 0;
>@@ -2941,6 +2941,22 @@
>
> ctx->state = PRE_HEAD;
> }
>+ }
>+
>+ /* We have nothing more to send, stop now. */
>+ if (APR_BUCKET_IS_EOS(dptr)) {
>+ /* Ensure that EOS is sent along. */
>+ APR_BUCKET_REMOVE(dptr);
>+ APR_BUCKET_INSERT_TAIL(*bb, dptr);
>
Thanks, this patch seems to fix the problem. One question,
though: why remove the bucket and then insert it at the end
of the same brigade? Is that just in case there's an EOS in
the middle of the brigade?
--Brian
Re: Core dump in mod-include
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Fri, Dec 14, 2001 at 09:32:11AM -0800, Ian Holsman wrote:
> hi guys.
> we've been investigating a couple of core dumps in some testing and we
> think we found one in mod-include.
>
> here is the email thread.
> does anyone have any reason NOT to reset the state when reseting
> the bytes parsed ?
Just to make sure I understand what you guys are thinking of,
this is your proposed patch, right? (more follows)
Index: modules/filters/mod_include.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.c,v
retrieving revision 1.166
diff -u -r1.166 mod_include.c
--- modules/filters/mod_include.c 2001/12/08 03:14:50 1.166
+++ modules/filters/mod_include.c 2001/12/14 18:12:31
@@ -3066,6 +3066,7 @@
}
else {
ctx->bytes_parsed = 0;
+ ctx->state = PRE_HEAD;
}
if ((parent = ap_get_module_config(r->request_config, &include_module))) {
If so, then I'm kind of leery about this.
The reason is that filters can be called many times over the
lifetime of the data. So, we could leave the parse engine in a
ctx->state = POST_HEAD between calls because we are still waiting
for a bucket-brigade containing the rest of the tag. We'd obliterate
that value with this. =) So, I don't think resetting the state is
going to do you any good here.
Based on your description, what seems to be happening is that the
client either disconnects or timeouts and the finalize_request is
passing down an EOS but mod_include is stuck in some internal
state. mod_include just isn't handling it well.
So, I'm thinking the appropriate check is to stop at EOS or
SENTINEL. There is no good reason to continue processing if we
see an EOS. But, we have a potentially saved brigade to handle.
So, what do we do? Perhaps this patch might be better:
Index: modules/filters/mod_include.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/filters/mod_include.c,v
retrieving revision 1.166
diff -u -r1.166 mod_include.c
--- modules/filters/mod_include.c 2001/12/08 03:14:50 1.166
+++ modules/filters/mod_include.c 2001/12/14 18:37:48
@@ -2700,7 +2700,7 @@
ap_escape_shell_cmd(r->pool, arg_copy));
}
- while (dptr != APR_BRIGADE_SENTINEL(*bb)) {
+ while (dptr != APR_BRIGADE_SENTINEL(*bb) || !APR_BUCKET_IS_EOS(dptr)) {
/* State to check for the STARTING_SEQUENCE. */
if ((ctx->state == PRE_HEAD) || (ctx->state == PARSE_HEAD)) {
int do_cleanup = 0;
@@ -2941,6 +2941,22 @@
ctx->state = PRE_HEAD;
}
+ }
+
+ /* We have nothing more to send, stop now. */
+ if (APR_BUCKET_IS_EOS(dptr)) {
+ /* Ensure that EOS is sent along. */
+ APR_BUCKET_REMOVE(dptr);
+ APR_BUCKET_INSERT_TAIL(*bb, dptr);
+
+ /* We might have something saved that we never completed, but send
+ * down unparsed. This allows for <!-- at the end of files to be
+ * sent correctly. */
+ if (!APR_BRIGADE_EMPTY(ctx->ssi_tag_brigade)) {
+ APR_BRIGADE_CONCAT(ctx->ssi_tag_brigade, *bb);
+ return ap_pass_brigade(f->next, ctx->ssi_tag_brigade);
+ }
+ return ap_pass_brigade(f->next, *bb);
}
/* If I am in the middle of parsing an SSI tag then I need to set aside
This might be better. I debated whether we should even send anything
if we see ctx->ssi_tag_brigade and I think we should. I also notice
that we typically call apr_brigade_cleanup on ssi_tag_brigade - not
sure why. We may need to add that.
Thoughts? This may be completely wrong. =) -- justin