Skip to content
Permalink
Browse files Browse the repository at this point in the history
h2: use after free on premature connection close #920
lib/http2/connection.c:on_read() calls parse_input(), which might free
`conn`. It does so in particular if the connection preface isn't
the expected one in expect_preface(). `conn` is then used after the free
in `if (h2o_timeout_is_linked(&conn->_write.timeout_entry)`.
We fix this by adding a return value to close_connection that returns a
negative value if `conn` has been free'd and can't be used anymore.

Credits for finding the bug to Tim Newsham.
  • Loading branch information
deweerdt authored and kazuho committed May 26, 2016
1 parent 92ef196 commit 1c0808d
Showing 1 changed file with 10 additions and 7 deletions.
17 changes: 10 additions & 7 deletions lib/http2/connection.c
Expand Up @@ -55,7 +55,7 @@ static const h2o_iovec_t SETTINGS_HOST_BIN = {H2O_STRLIT("\x00\x00\x0c" /* f
static __thread h2o_buffer_prototype_t wbuf_buffer_prototype = {{16}, {H2O_HTTP2_DEFAULT_OUTBUF_SIZE}};

static void initiate_graceful_shutdown(h2o_context_t *ctx);
static void close_connection(h2o_http2_conn_t *conn);
static int close_connection(h2o_http2_conn_t *conn);
static void send_stream_error(h2o_http2_conn_t *conn, uint32_t stream_id, int errnum);
static ssize_t expect_default(h2o_http2_conn_t *conn, const uint8_t *src, size_t len, const char **err_desc);
static int do_emit_writereq(h2o_http2_conn_t *conn);
Expand Down Expand Up @@ -261,15 +261,17 @@ static void close_connection_now(h2o_http2_conn_t *conn)
free(conn);
}

void close_connection(h2o_http2_conn_t *conn)
int close_connection(h2o_http2_conn_t *conn)
{
conn->state = H2O_HTTP2_CONN_STATE_IS_CLOSING;

if (conn->_write.buf_in_flight != NULL || h2o_timeout_is_linked(&conn->_write.timeout_entry)) {
/* there is a pending write, let on_write_complete actually close the connection */
} else {
close_connection_now(conn);
return -1;
}
return 0;
}

void send_stream_error(h2o_http2_conn_t *conn, uint32_t stream_id, int errnum)
Expand Down Expand Up @@ -808,7 +810,7 @@ static ssize_t expect_preface(h2o_http2_conn_t *conn, const uint8_t *src, size_t
return CONNECTION_PREFACE.len;
}

static void parse_input(h2o_http2_conn_t *conn)
static int parse_input(h2o_http2_conn_t *conn)
{
size_t http2_max_concurrent_requests_per_connection = conn->super.ctx->globalconf->http2.max_concurrent_requests_per_connection;
int perform_early_exit = 0;
Expand All @@ -831,20 +833,20 @@ static void parse_input(h2o_http2_conn_t *conn)
enqueue_goaway(conn, (int)ret,
err_desc != NULL ? (h2o_iovec_t){(char *)err_desc, strlen(err_desc)} : (h2o_iovec_t){});
}
close_connection(conn);
return;
return close_connection(conn);
}
/* advance to the next frame */
h2o_buffer_consume(&conn->sock->input, ret);
}

if (!h2o_socket_is_reading(conn->sock))
h2o_socket_read_start(conn->sock, on_read);
return;
return 0;

EarlyExit:
if (h2o_socket_is_reading(conn->sock))
h2o_socket_read_stop(conn->sock);
return 0;
}

static void on_read(h2o_socket_t *sock, int status)
Expand All @@ -858,7 +860,8 @@ static void on_read(h2o_socket_t *sock, int status)
}

update_idle_timeout(conn);
parse_input(conn);
if (parse_input(conn) != 0)
return;

/* write immediately, if there is no write in flight and if pending write exists */
if (h2o_timeout_is_linked(&conn->_write.timeout_entry)) {
Expand Down

0 comments on commit 1c0808d

Please sign in to comment.