Hi,
I found a crash in relayd when using http relay. `bev` pointer is
used after a free in `relay_http.c` lines: 438, 492 and 609
In `relay_http.c` there is 3 functions, used as read callback for
libevent buffer:
* `relay_read_http`: parse http headers
* `relay_read_httpcontent`: parse simple http content
* `relay_read_httpchunks`: parse http content sent using 'chunked'
method
When one of the three functions is finishing its work and data are sill
available in the buffer , the function calls `bev->readcb(bev, arg);`
to handle the remaining data. This last action is mandatory, because
these remaining data would have been read from the socket and are in
the the current bufferevent Libevent will not call the callback anymore,
as a result the request will timeout.
This breaks the callback designs and leads to bugy software.
A crash occurs when the connection is closed.
In this case, the context had freed inside the callback.
The callback return no information as requested by libevent's design.
Nevertheless the context is used just afer.
For example:
The function `relay_read_httpchunks` finish to read all
chunk of data for the current request, but there is still data
remaining, so it directly call `relay_read_http` to parse the next HTTP
request, with the following code:
if (EVBUFFER_LENGTH(src))
bev->readcb(bev, arg);
The problem is that inside these 3 functions, if an error occure the
functions `relay_close` or `relay_abort_http` are called, and then it
free all data related to the current connection. Then after this line:
bev->readcb(bev, arg);
`bev` has been free, and the following line which is:
bufferevent_enable(bev, EV_READ);
cause a crash (SIGBUS/SIGSEGV), when trying to accesss to one of the
field of `bev`
Please review the following patch that do not manually call the
callback. Moreover this implementation explicitly shows the state
machine that was hidden inside the libevent context data.
The new callback has 3 states just like there was 3 callbacks before.
The callback calls the previous processing and check if more processing
must be done before calling `bufferevent_enable`
So the functions:
* `relay_read_http`
* `relay_read_httpcontent`
* `relay_read_httpchunks`
now returns:
* -1 if datas had been free
* 0 if all is OK, and it should try to parse remaining data
* 1 if data is OK, and it should not try to parse remaining data
Index: http.h
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/http.h,v
retrieving revision 1.5
diff -u -p -r1.5 http.h
--- http.h 14 Aug 2014 10:30:52 -0000 1.5
+++ http.h 12 May 2015 13:14:21 -0000
@@ -180,6 +180,14 @@ struct http_mediatype {
{ NULL } \
}
+/* Define state of current http relay */
+enum httpstate {
+ HTTP_STATE_READ_HEADER = 0,
+ HTTP_STATE_READ_CONTENT,
+ HTTP_STATE_READ_CHUNKS,
+ HTTP_STATE_READ_DATA,
+};
+
/* Used during runtime */
struct http_descriptor {
struct kv http_pathquery;
@@ -202,6 +210,8 @@ struct http_descriptor {
/* A tree of headers and attached lists for repeated headers. */
struct kv *http_lastheader;
struct kvtree http_headers;
+
+ enum httpstate http_state;
};
#endif /* _HTTP_H */
Index: relay.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
retrieving revision 1.193
diff -u -p -r1.193 relay.c
--- relay.c 29 Apr 2015 08:41:24 -0000 1.193
+++ relay.c 12 May 2015 13:14:21 -0000
@@ -683,7 +683,7 @@ relay_connected(int fd, short sig, void
return;
}
con->se_out.toread = TOREAD_HTTP_HEADER;
- outrd = relay_read_http;
+ outrd = relay_read_http_cb;
break;
case RELAY_PROTO_TCP:
/* Use defaults */
@@ -734,7 +734,7 @@ relay_input(struct rsession *con)
return;
}
con->se_in.toread = TOREAD_HTTP_HEADER;
- inrd = relay_read_http;
+ inrd = relay_read_http_cb;
break;
case RELAY_PROTO_TCP:
/* Use defaults */
Index: relay_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v
retrieving revision 1.44
diff -u -p -r1.44 relay_http.c
--- relay_http.c 29 Apr 2015 08:41:24 -0000 1.44
+++ relay_http.c 12 May 2015 13:14:21 -0000
@@ -49,8 +49,9 @@ int relay_lookup_url(struct ctl_relay_
int relay_lookup_query(struct ctl_relay_event *, struct kv *);
int relay_lookup_cookie(struct ctl_relay_event *, const char *,
struct kv *);
-void relay_read_httpcontent(struct bufferevent *, void *);
-void relay_read_httpchunks(struct bufferevent *, void *);
+int relay_read_http(struct bufferevent *, void *);
+int relay_read_httpcontent(struct bufferevent *, void *);
+int relay_read_httpchunks(struct bufferevent *, void *);
char *relay_expand_http(struct ctl_relay_event *, char *,
char *, size_t);
int relay_writeheader_kv(struct ctl_relay_event *, struct kv *);
@@ -151,7 +152,53 @@ relay_httpdesc_free(struct http_descript
kv_purge(&desc->http_headers);
}
-void
+void relay_read_http_cb(struct bufferevent *bev, void *arg) {
+ struct ctl_relay_event *cre = arg;
+ struct http_descriptor *desc = cre->desc;
+ struct rsession *con = cre->con;
+ struct evbuffer *src = EVBUFFER_INPUT(bev);
+ int rc = 0;
+ int stop = 0;
+ int try_splice = 0;
+
+ do {
+ switch (desc->http_state) {
+ case HTTP_STATE_READ_HEADER:
+ rc = relay_read_http(bev, arg);
+ if (rc != -1 && desc->http_state == HTTP_STATE_READ_HEADER) {
+ // The state does not change, so it can stop here
+ // this mean the http headers has not been fully parsed
+ // because there is not enough data available
+ stop = 1;
+ try_splice = 1;
+ }
+ break;
+ case HTTP_STATE_READ_CONTENT:
+ rc = relay_read_httpcontent(bev, arg);
+ if (rc != -1 && desc->http_state == HTTP_STATE_READ_CONTENT) {
+ // The state does not change, so it can stop here
+ stop = 1;
+ try_splice = 0;
+ }
+ break;
+ case HTTP_STATE_READ_CHUNKS:
+ rc = relay_read_httpchunks(bev, arg);
+ try_splice = 0;
+ break;
+ case HTTP_STATE_READ_DATA:
+ relay_read(bev, arg);
+ return;
+ }
+ } while (!stop && rc == 0 && EVBUFFER_LENGTH(src));
+
+ if (rc == 0) {
+ bufferevent_enable(bev, EV_READ);
+ if (try_splice && relay_splice(cre) == -1)
+ relay_close(con, strerror(errno));
+ }
+}
+
+int
relay_read_http(struct bufferevent *bev, void *arg)
{
struct ctl_relay_event *cre = arg;
@@ -174,7 +221,7 @@ relay_read_http(struct bufferevent *bev,
__func__, con->se_id, size, cre->toread);
if (!size) {
if (cre->dir == RELAY_DIR_RESPONSE)
- return;
+ return 1;
cre->toread = TOREAD_HTTP_HEADER;
goto done;
}
@@ -198,7 +245,7 @@ relay_read_http(struct bufferevent *bev,
if (cre->headerlen > RELAY_MAXHEADERLENGTH) {
free(line);
relay_abort_http(con, 413, "request too large", 0);
- return;
+ return -1;
}
/*
@@ -216,7 +263,7 @@ relay_read_http(struct bufferevent *bev,
if (cre->line == 1) {
free(line);
relay_abort_http(con, 400, "malformed", 0);
- return;
+ return -1;
}
/* Append line to the last header, if present */
@@ -354,23 +401,23 @@ relay_read_http(struct bufferevent *bev,
if (cre->done) {
if (desc->http_method == HTTP_METHOD_NONE) {
relay_abort_http(con, 406, "no method", 0);
- return;
+ return -1;
}
action = relay_test(proto, cre);
if (action == RES_FAIL) {
relay_close(con, "filter rule failed");
- return;
+ return -1;
} else if (action != RES_PASS) {
relay_abort_http(con, 403, "Forbidden", con->se_label);
- return;
+ return -1;
}
switch (desc->http_method) {
case HTTP_METHOD_CONNECT:
/* Data stream */
cre->toread = TOREAD_UNLIMITED;
- bev->readcb = relay_read;
+ desc->http_state = HTTP_STATE_READ_DATA;
break;
case HTTP_METHOD_DELETE:
case HTTP_METHOD_GET:
@@ -383,24 +430,24 @@ relay_read_http(struct bufferevent *bev,
case HTTP_METHOD_RESPONSE:
/* HTTP request payload */
if (cre->toread > 0)
- bev->readcb = relay_read_httpcontent;
+ desc->http_state = HTTP_STATE_READ_CONTENT;
/* Single-pass HTTP body */
if (cre->toread < 0) {
cre->toread = TOREAD_UNLIMITED;
- bev->readcb = relay_read;
+ desc->http_state = HTTP_STATE_READ_DATA;
}
break;
default:
/* HTTP handler */
cre->toread = TOREAD_HTTP_HEADER;
- bev->readcb = relay_read_http;
+ desc->http_state = HTTP_STATE_READ_HEADER;
break;
}
if (desc->http_chunked) {
/* Chunked transfer encoding */
cre->toread = TOREAD_HTTP_CHUNK_LENGTH;
- bev->readcb = relay_read_httpchunks;
+ desc->http_state = HTTP_STATE_READ_CHUNKS;
}
if (cre->dir == RELAY_DIR_REQUEST) {
@@ -421,35 +468,32 @@ relay_read_http(struct bufferevent *bev,
cre->dst->bev == NULL) {
if (rlay->rl_conf.fwdmode == FWD_TRANS) {
relay_bindanyreq(con, 0, IPPROTO_TCP);
- return;
+ return -1;
}
if (relay_connect(con) == -1) {
relay_abort_http(con, 502, "session failed", 0);
- return;
+ return -1;
}
}
}
if (con->se_done) {
relay_close(con, "last http read (done)");
- return;
+ return -1;
}
- if (EVBUFFER_LENGTH(src) && bev->readcb != relay_read_http)
- bev->readcb(bev, arg);
- bufferevent_enable(bev, EV_READ);
- if (relay_splice(cre) == -1)
- relay_close(con, strerror(errno));
- return;
+ return 0;
fail:
relay_abort_http(con, 500, strerror(errno), 0);
- return;
+ return -1;
abort:
free(line);
+ return -1;
}
-void
+int
relay_read_httpcontent(struct bufferevent *bev, void *arg)
{
struct ctl_relay_event *cre = arg;
+ struct http_descriptor *desc = cre->desc;
struct rsession *con = cre->con;
struct evbuffer *src = EVBUFFER_INPUT(bev);
size_t size;
@@ -461,7 +505,7 @@ relay_read_httpcontent(struct buffereven
DPRINTF("%s: session %d: size %lu, to read %lld", __func__,
con->se_id, size, cre->toread);
if (!size)
- return;
+ return 1;
if (relay_spliceadjust(cre) == -1)
goto fail;
@@ -483,25 +527,24 @@ relay_read_httpcontent(struct buffereven
}
if (cre->toread == 0) {
cre->toread = TOREAD_HTTP_HEADER;
- bev->readcb = relay_read_http;
+ desc->http_state = HTTP_STATE_READ_HEADER;
}
if (con->se_done)
goto done;
- if (bev->readcb != relay_read_httpcontent)
- bev->readcb(bev, arg);
- bufferevent_enable(bev, EV_READ);
- return;
+ return 0;
done:
relay_close(con, "last http content read");
- return;
+ return -1;
fail:
relay_close(con, strerror(errno));
+ return -1;
}
-void
+int
relay_read_httpchunks(struct bufferevent *bev, void *arg)
{
struct ctl_relay_event *cre = arg;
+ struct http_descriptor *desc = cre->desc;
struct rsession *con = cre->con;
struct evbuffer *src = EVBUFFER_INPUT(bev);
char *line;
@@ -515,7 +558,7 @@ relay_read_httpchunks(struct bufferevent
DPRINTF("%s: session %d: size %lu, to read %lld", __func__,
con->se_id, size, cre->toread);
if (!size)
- return;
+ return 1;
if (relay_spliceadjust(cre) == -1)
goto fail;
@@ -541,7 +584,7 @@ relay_read_httpchunks(struct bufferevent
if (line == NULL) {
/* Ignore empty line, continue */
bufferevent_enable(bev, EV_READ);
- return;
+ return 1;
}
if (strlen(line) == 0) {
free(line);
@@ -555,7 +598,7 @@ relay_read_httpchunks(struct bufferevent
if (sscanf(line, "%llx", &llval) != 1 || llval < 0) {
free(line);
relay_close(con, "invalid chunk size");
- return;
+ return -1;
}
if (relay_bufferevent_print(cre->dst, line) == -1 ||
@@ -576,7 +619,7 @@ relay_read_httpchunks(struct bufferevent
if (line == NULL) {
/* Ignore empty line, continue */
bufferevent_enable(bev, EV_READ);
- return;
+ return -1;
}
if (relay_bufferevent_print(cre->dst, line) == -1 ||
relay_bufferevent_print(cre->dst, "\r\n") == -1) {
@@ -586,7 +629,7 @@ relay_read_httpchunks(struct bufferevent
if (strlen(line) == 0) {
/* Switch to HTTP header mode */
cre->toread = TOREAD_HTTP_HEADER;
- bev->readcb = relay_read_http;
+ desc->http_state = HTTP_STATE_READ_HEADER;
}
free(line);
break;
@@ -604,16 +647,14 @@ relay_read_httpchunks(struct bufferevent
next:
if (con->se_done)
goto done;
- if (EVBUFFER_LENGTH(src))
- bev->readcb(bev, arg);
- bufferevent_enable(bev, EV_READ);
- return;
+ return 0;
done:
relay_close(con, "last http chunk read (done)");
- return;
+ return -1;
fail:
relay_close(con, strerror(errno));
+ return -1;
}
void
Index: relayd.h
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/relayd.h,v
retrieving revision 1.209
diff -u -p -r1.209 relayd.h
--- relayd.h 2 May 2015 13:15:24 -0000 1.209
+++ relayd.h 12 May 2015 13:14:21 -0000
@@ -1184,7 +1184,7 @@ void relay_http(struct relayd *);
void relay_http_init(struct relay *);
void relay_abort_http(struct rsession *, u_int, const char *,
u_int16_t);
-void relay_read_http(struct bufferevent *, void *);
+void relay_read_http_cb(struct bufferevent *, void *);
void relay_close_http(struct rsession *);
u_int relay_httpmethod_byname(const char *);
const char
--
Bertrand PROVOST
