Skip to content

Commit

Permalink
Fix crashing http server when callback do not reply in place
Browse files Browse the repository at this point in the history
General http callback looks like:
  static void http_cb(struct evhttp_request *req, void *arg)
  {
    evhttp_send_reply(req, HTTP_OK, "Everything is fine", NULL);
  }

And they will work fine becuase in this case http will write request
first, and during write preparation it will disable *read callback* (in
evhttp_write_buffer()), but if we don't reply immediately, for example:
  static void http_cb(struct evhttp_request *req, void *arg)
  {
    return;
  }

This will leave connection in incorrect state, and if another request
will be written to the same connection libevent will abort with:
  [err] ../http.c: illegal connection state 7

Because it thinks that read for now is not possible, since there were no
write.

Fix this by disabling EV_READ entirely. We couldn't just reset callbacks
because this will leave EOF detection, which we don't need, since user
hasn't replied to callback yet.

Reported-by: Cory Fields <cory@coryfields.com>
(cherry picked from commit 5ff8eb2)
  • Loading branch information
azat committed Feb 2, 2019
1 parent 7e91622 commit 5b40744
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 3 deletions.
8 changes: 5 additions & 3 deletions http.c
Original file line number Diff line number Diff line change
Expand Up @@ -368,15 +368,15 @@ evhttp_write_buffer(struct evhttp_connection *evcon,
evcon->cb_arg = arg;

/* Disable the read callback: we don't actually care about data;
* we only care about close detection. (We don't disable reading,
* since we *do* want to learn about any close events.) */
* we only care about close detection. (We don't disable reading --
* EV_READ, since we *do* want to learn about any close events.) */
bufferevent_setcb(evcon->bufev,
NULL, /*read*/
evhttp_write_cb,
evhttp_error_cb,
evcon);

bufferevent_enable(evcon->bufev, EV_WRITE);
bufferevent_enable(evcon->bufev, EV_READ|EV_WRITE);
}

static void
Expand Down Expand Up @@ -3437,6 +3437,8 @@ evhttp_handle_request(struct evhttp_request *req, void *arg)
}

if ((cb = evhttp_dispatch_callback(&http->callbacks, req)) != NULL) {
bufferevent_disable(req->evcon->bufev, EV_READ);

(*cb->cb)(req, cb->cbarg);
return;
}
Expand Down
66 changes: 66 additions & 0 deletions test/regress_http.c
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ static struct event_base *exit_base;
static char const BASIC_REQUEST_BODY[] = "This is funny";

static void http_basic_cb(struct evhttp_request *req, void *arg);
static void http_timeout_cb(struct evhttp_request *req, void *arg);
static void http_large_cb(struct evhttp_request *req, void *arg);
static void http_chunked_cb(struct evhttp_request *req, void *arg);
static void http_post_cb(struct evhttp_request *req, void *arg);
Expand Down Expand Up @@ -146,6 +147,7 @@ http_setup(ev_uint16_t *pport, struct event_base *base, int mask)

/* Register a callback for certain types of requests */
evhttp_set_cb(myhttp, "/test", http_basic_cb, myhttp);
evhttp_set_cb(myhttp, "/timeout", http_timeout_cb, myhttp);
evhttp_set_cb(myhttp, "/large", http_large_cb, base);
evhttp_set_cb(myhttp, "/chunked", http_chunked_cb, base);
evhttp_set_cb(myhttp, "/streamed", http_chunked_cb, base);
Expand Down Expand Up @@ -346,6 +348,20 @@ http_basic_cb(struct evhttp_request *req, void *arg)
evbuffer_free(evb);
}

static void http_timeout_reply_cb(int fd, short events, void *arg)
{
struct evhttp_request *req = arg;
evhttp_send_reply(req, HTTP_OK, "Everything is fine", NULL);
test_ok++;
}
static void
http_timeout_cb(struct evhttp_request *req, void *arg)
{
struct timeval when = { 0, 100 };
event_base_once(exit_base, -1, EV_TIMEOUT,
http_timeout_reply_cb, req, &when);
}

static void
http_large_cb(struct evhttp_request *req, void *arg)
{
Expand Down Expand Up @@ -4510,6 +4526,54 @@ http_request_own_test(void *arg)
test_ok = 1;
}

static void
http_request_extra_body_test(void *arg)
{
struct basic_test_data *data = arg;
struct bufferevent *bev = NULL;
evutil_socket_t fd;
ev_uint16_t port = 0;
int i;
struct evhttp *http = http_setup(&port, data->base, 0);
struct evbuffer *out, *body = NULL;

exit_base = data->base;
test_ok = 0;

fd = http_connect("127.0.0.1", port);

/* Stupid thing to send a request */
bev = create_bev(data->base, fd, 0);
bufferevent_setcb(bev, http_readcb, http_writecb,
http_errorcb, data->base);
out = bufferevent_get_output(bev);
body = evbuffer_new();

for (i = 0; i < 10000; ++i)
evbuffer_add_printf(body, "this is the body that HEAD should not have");

evbuffer_add_printf(out,
"HEAD /timeout HTTP/1.1\r\n"
"Host: somehost\r\n"
"Connection: close\r\n"
"Content-Length: %i\r\n"
"\r\n",
(int)evbuffer_get_length(body)
);
evbuffer_add_buffer(out, body);

event_base_dispatch(data->base);

tt_assert(test_ok == -2);

end:
evhttp_free(http);
if (bev)
bufferevent_free(bev);
if (body)
evbuffer_free(body);
}

#define HTTP_LEGACY(name) \
{ #name, run_legacy_test_fn, TT_ISOLATED|TT_LEGACY, &legacy_setup, \
http_##name##_test }
Expand Down Expand Up @@ -4629,6 +4693,8 @@ struct testcase_t http_testcases[] = {
HTTP(write_during_read),
HTTP(request_own),

HTTP(request_extra_body),

#ifdef EVENT__HAVE_OPENSSL
HTTPS(basic),
HTTPS(filter_basic),
Expand Down

0 comments on commit 5b40744

Please sign in to comment.