Skip to content

Commit 5ff8eb2

Browse files
azatazat
committed
Fix crashing http server when callback do not reply in place
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 <[email protected]>
1 parent da3f2ba commit 5ff8eb2

File tree

2 files changed

+71
-3
lines changed

2 files changed

+71
-3
lines changed

‎http.c‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -368,15 +368,15 @@ evhttp_write_buffer(struct evhttp_connection *evcon,
368368
evcon->cb_arg = arg;
369369

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

379-
bufferevent_enable(evcon->bufev, EV_WRITE);
379+
bufferevent_enable(evcon->bufev, EV_READ|EV_WRITE);
380380
}
381381

382382
static void
@@ -3437,6 +3437,8 @@ evhttp_handle_request(struct evhttp_request *req, void *arg)
34373437
}
34383438

34393439
if ((cb = evhttp_dispatch_callback(&http->callbacks, req)) != NULL) {
3440+
bufferevent_disable(req->evcon->bufev, EV_READ);
3441+
34403442
(*cb->cb)(req, cb->cbarg);
34413443
return;
34423444
}

‎test/regress_http.c‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ static struct event_base *exit_base;
7272
static char const BASIC_REQUEST_BODY[] = "This is funny";
7373

7474
static void http_basic_cb(struct evhttp_request *req, void *arg);
75+
static void http_timeout_cb(struct evhttp_request *req, void *arg);
7576
static void http_large_cb(struct evhttp_request *req, void *arg);
7677
static void http_chunked_cb(struct evhttp_request *req, void *arg);
7778
static void http_post_cb(struct evhttp_request *req, void *arg);
@@ -146,6 +147,7 @@ http_setup(ev_uint16_t *pport, struct event_base *base, int mask)
146147

147148
/* Register a callback for certain types of requests */
148149
evhttp_set_cb(myhttp, "/test", http_basic_cb, myhttp);
150+
evhttp_set_cb(myhttp, "/timeout", http_timeout_cb, myhttp);
149151
evhttp_set_cb(myhttp, "/large", http_large_cb, base);
150152
evhttp_set_cb(myhttp, "/chunked", http_chunked_cb, base);
151153
evhttp_set_cb(myhttp, "/streamed", http_chunked_cb, base);
@@ -346,6 +348,20 @@ http_basic_cb(struct evhttp_request *req, void *arg)
346348
evbuffer_free(evb);
347349
}
348350

351+
static void http_timeout_reply_cb(int fd, short events, void *arg)
352+
{
353+
struct evhttp_request *req = arg;
354+
evhttp_send_reply(req, HTTP_OK, "Everything is fine", NULL);
355+
test_ok++;
356+
}
357+
static void
358+
http_timeout_cb(struct evhttp_request *req, void *arg)
359+
{
360+
struct timeval when = { 0, 100 };
361+
event_base_once(exit_base, -1, EV_TIMEOUT,
362+
http_timeout_reply_cb, req, &when);
363+
}
364+
349365
static void
350366
http_large_cb(struct evhttp_request *req, void *arg)
351367
{
@@ -4510,6 +4526,54 @@ http_request_own_test(void *arg)
45104526
test_ok = 1;
45114527
}
45124528

4529+
static void
4530+
http_request_extra_body_test(void *arg)
4531+
{
4532+
struct basic_test_data *data = arg;
4533+
struct bufferevent *bev = NULL;
4534+
evutil_socket_t fd;
4535+
ev_uint16_t port = 0;
4536+
int i;
4537+
struct evhttp *http = http_setup(&port, data->base, 0);
4538+
struct evbuffer *out, *body = NULL;
4539+
4540+
exit_base = data->base;
4541+
test_ok = 0;
4542+
4543+
fd = http_connect("127.0.0.1", port);
4544+
4545+
/* Stupid thing to send a request */
4546+
bev = create_bev(data->base, fd, 0);
4547+
bufferevent_setcb(bev, http_readcb, http_writecb,
4548+
http_errorcb, data->base);
4549+
out = bufferevent_get_output(bev);
4550+
body = evbuffer_new();
4551+
4552+
for (i = 0; i < 10000; ++i)
4553+
evbuffer_add_printf(body, "this is the body that HEAD should not have");
4554+
4555+
evbuffer_add_printf(out,
4556+
"HEAD /timeout HTTP/1.1\r\n"
4557+
"Host: somehost\r\n"
4558+
"Connection: close\r\n"
4559+
"Content-Length: %i\r\n"
4560+
"\r\n",
4561+
(int)evbuffer_get_length(body)
4562+
);
4563+
evbuffer_add_buffer(out, body);
4564+
4565+
event_base_dispatch(data->base);
4566+
4567+
tt_assert(test_ok == -2);
4568+
4569+
end:
4570+
evhttp_free(http);
4571+
if (bev)
4572+
bufferevent_free(bev);
4573+
if (body)
4574+
evbuffer_free(body);
4575+
}
4576+
45134577
#define HTTP_LEGACY(name) \
45144578
{ #name, run_legacy_test_fn, TT_ISOLATED|TT_LEGACY, &legacy_setup, \
45154579
http_##name##_test }
@@ -4629,6 +4693,8 @@ struct testcase_t http_testcases[] = {
46294693
HTTP(write_during_read),
46304694
HTTP(request_own),
46314695

4696+
HTTP(request_extra_body),
4697+
46324698
#ifdef EVENT__HAVE_OPENSSL
46334699
HTTPS(basic),
46344700
HTTPS(filter_basic),

0 commit comments

Comments
 (0)