Hi,
thanks. I'll have a look at it, I would be happy if this provides a
way to get rid of calling the bev->readcb callbacks again.
Reyk
On Tue, May 12, 2015 at 05:34:58PM -0400, Bertrand PROVOST wrote:
> 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
>
--
