On Mon, Aug 15, 2016 at 4:49 AM, <bugzi...@apache.org> wrote:
> https://bz.apache.org/bugzilla/show_bug.cgi?id=59840
>
> --- Comment #10 from Konstantin J. Chernov <k.j.cher...@gmail.com> ---
> Right after enabling http2 again, got a few more segfaults :(
>
> Here's the latest one (+2 more with the same backtraces as in my previous
> comments):
>> ::stack
> libaprutil-1.so.0.5.4`apr_brigade_cleanup+0x43()
> libaprutil-1.so.0.5.4`brigade_cleanup+0x18()
> libapr-1.so.0.5.2`run_cleanups+0x3c()
> libapr-1.so.0.5.2`apr_pool_destroy+0x57()
> eor_bucket_destroy+0x2a()
> libaprutil-1.so.0.5.4`apr_brigade_cleanup+0x60()
> ap_process_request_after_handler+0xc6()
> ap_process_async_request+0x79f()
> ap_process_request+0x24()
> mod_http2.so`h2_task_process_request+0x3fd()
> mod_http2.so`h2_task_process_conn+0x228()
> ap_run_process_connection+0x61()
> mod_http2.so`h2_task_do+0x5e2()
> mod_http2.so`execute+0x50()
> libapr-1.so.0.5.2`dummy_worker+0x30()
> libc.so.1`_thr_setup+0x5b()
> libc.so.1`_lwp_start()
I wonder if this crash (and some others in the same 59840) could be
caused by how we flush the h2_bucket_eoc in
h2_conn_io.c::pass_output().
Currently the flush_bucket follows h2_bucket_eoc, so possibly the
buckets could be flushed after eoc/io is freed, with dangling buckets?
Maybe someting like the attached patch is safer, by first flushing
(and cleanup the brigade for any leftover), and then send the eoc
bucket alone?
Regards,
Yann.
Index: modules/http2/h2_conn_io.c
===================================================================
--- modules/http2/h2_conn_io.c (revision 1757096)
+++ modules/http2/h2_conn_io.c (working copy)
@@ -269,9 +269,11 @@ static void check_write_size(h2_conn_io *io)
}
}
-static apr_status_t pass_output(h2_conn_io *io, int flush, int eoc)
+static apr_status_t pass_output(h2_conn_io *io, int flush,
+ h2_session *session_eoc)
{
conn_rec *c = io->c;
+ apr_bucket_brigade *bb = io->output;
apr_bucket *b;
apr_off_t bblen;
apr_status_t status;
@@ -279,28 +281,37 @@ static void check_write_size(h2_conn_io *io)
append_scratch(io);
if (flush) {
b = apr_bucket_flush_create(c->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(io->output, b);
+ APR_BRIGADE_INSERT_TAIL(bb, b);
}
- if (APR_BRIGADE_EMPTY(io->output)) {
+ if (APR_BRIGADE_EMPTY(bb)) {
return APR_SUCCESS;
}
ap_log_cerror(APLOG_MARK, APLOG_TRACE4, 0, c, "h2_conn_io: pass_output");
ap_update_child_status(c->sbh, SERVER_BUSY_WRITE, NULL);
- apr_brigade_length(io->output, 0, &bblen);
+ apr_brigade_length(bb, 0, &bblen);
- h2_conn_io_bb_log(c, 0, APLOG_TRACE2, "master conn pass", io->output);
- status = ap_pass_brigade(c->output_filters, io->output);
+ h2_conn_io_bb_log(c, 0, APLOG_TRACE2, "master conn pass", bb);
+ status = ap_pass_brigade(c->output_filters, bb);
+ if (status == APR_SUCCESS) {
+ io->bytes_written += (apr_size_t)bblen;
+ io->last_write = apr_time_now();
+ }
+ apr_brigade_cleanup(bb);
- /* careful with access after this, as we might have flushed an EOC bucket
- * that de-allocated us all. */
- if (!eoc) {
- apr_brigade_cleanup(io->output);
+ if (session_eoc) {
+ apr_status_t tmp;
+ b = h2_bucket_eoc_create(c->bucket_alloc, session_eoc);
+ APR_BRIGADE_INSERT_TAIL(bb, b);
+ h2_conn_io_bb_log(c, 0, APLOG_TRACE2, "master conn pass", bb);
+ tmp = ap_pass_brigade(c->output_filters, bb);
if (status == APR_SUCCESS) {
- io->bytes_written += (apr_size_t)bblen;
- io->last_write = apr_time_now();
+ status = tmp;
}
+ /* careful with access to io after this, we have flushed an EOC bucket
+ * that de-allocated us all. */
+ apr_brigade_cleanup(bb);
}
if (status != APR_SUCCESS) {
@@ -313,14 +324,12 @@ static void check_write_size(h2_conn_io *io)
apr_status_t h2_conn_io_flush(h2_conn_io *io)
{
- return pass_output(io, 1, 0);
+ return pass_output(io, 1, NULL);
}
apr_status_t h2_conn_io_write_eoc(h2_conn_io *io, h2_session *session)
{
- apr_bucket *b = h2_bucket_eoc_create(io->c->bucket_alloc, session);
- APR_BRIGADE_INSERT_TAIL(io->output, b);
- return pass_output(io, 1, 1);
+ return pass_output(io, 1, session);
}
apr_status_t h2_conn_io_write(h2_conn_io *io, const char *data, size_t length)
@@ -413,7 +422,7 @@ apr_status_t h2_conn_io_pass(h2_conn_io *io, apr_b
if (!APR_BRIGADE_EMPTY(io->output)) {
apr_off_t len = h2_brigade_mem_size(io->output);
if (len >= io->pass_threshold) {
- return pass_output(io, 0, 0);
+ return pass_output(io, 0, NULL);
}
}
}
