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
