On Thu, Apr 08, 2021 at 04:47:15PM +0200, Claudio Jeker wrote:
> This diff is a first step in tightening the code in http.c
> It should cleanup the poll handling and make adds some code to ensure that
> only expected results are returned. The goal is that http_handle() only
> does IO processing and http_nextstep() is used for transitions into new
> states.
I like this direction.
> I did shuffle the code around a bit. So that similar functions are together.
> Also by shuffling the poll loop now connections are added after the all
> active connections have been processed.
This added quite a bit of noise to the diff (especially some parts
shuffling _write() functions are hard to read).
I'm ok with this, one small comment below.
>
> --
> :wq Claudio
>
> Index: http.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 http.c
> --- http.c 7 Apr 2021 16:40:38 -0000 1.21
> +++ http.c 8 Apr 2021 14:17:01 -0000
> @@ -107,9 +107,7 @@ struct http_connection {
> size_t bufsz;
> size_t bufpos;
> size_t id;
> - size_t chunksz;
> - off_t filesize;
> - off_t bytes;
> + off_t iosz;
> int status;
> int redirect_loop;
> int fd;
> @@ -132,7 +130,7 @@ size_t tls_ca_size;
> static const char *
> http_info(const char *url)
> {
> - static char buf[64];
> + static char buf[80];
>
> if (strnvis(buf, url, sizeof buf, VIS_SAFE) >= (int)sizeof buf) {
> /* overflow, add indicator */
> @@ -312,24 +310,6 @@ http_free(struct http_connection *conn)
> }
>
> static int
> -http_close(struct http_connection *conn)
> -{
> - if (conn->tls != NULL) {
> - switch (tls_close(conn->tls)) {
> - case TLS_WANT_POLLIN:
> - return WANT_POLLIN;
> - case TLS_WANT_POLLOUT:
> - return WANT_POLLOUT;
> - case 0:
> - case -1:
> - break;
> - }
> - }
> -
> - return -1;
> -}
> -
> -static int
> http_parse_uri(char *uri, char **ohost, char **oport, char **opath)
> {
> char *host, *port = NULL, *path;
> @@ -426,50 +406,12 @@ http_new(size_t id, char *uri, char *mod
> }
>
> static int
> -http_redirect(struct http_connection *conn, char *uri)
> -{
> - char *host, *port, *path;
> -
> - logx("redirect to %s", http_info(uri));
> -
> - if (http_parse_uri(uri, &host, &port, &path) == -1) {
> - free(uri);
> - return -1;
> - }
> -
> - free(conn->url);
> - conn->url = uri;
> - free(conn->host);
> - conn->host = host;
> - free(conn->port);
> - conn->port = port;
> - conn->path = path;
> - /* keep modified_since since that is part of the request */
> - free(conn->last_modified);
> - conn->last_modified = NULL;
> - free(conn->buf);
> - conn->buf = NULL;
> - conn->bufpos = 0;
> - conn->bufsz = 0;
> - tls_close(conn->tls);
> - tls_free(conn->tls);
> - conn->tls = NULL;
> - close(conn->fd);
> - conn->state = STATE_INIT;
> -
> - /* TODO proxy support (overload of host and port) */
> -
> - if (http_resolv(conn, host, port) == -1)
> - return -1;
> -
> - return 0;
> -}
> -
> -static int
> http_connect(struct http_connection *conn)
> {
> const char *cause = NULL;
>
> + conn->state = STATE_CONNECT;
> +
> if (conn->fd != -1) {
> close(conn->fd);
> conn->fd = -1;
> @@ -531,6 +473,7 @@ http_connect(struct http_connection *con
>
> freeaddrinfo(conn->res0);
> conn->res0 = NULL;
> + conn->res = NULL;
A few lines before this, there's another freeaddrinfo() where you don't
NULL out conn->res. I don't think it matters since you'll promptly end
in the case -1: of http_do(), but I would still do it for consistency.
>
> #if 0
> /* TODO proxy connect */
> @@ -560,6 +503,7 @@ http_finish_connect(struct http_connecti
>
> freeaddrinfo(conn->res0);
> conn->res0 = NULL;
> + conn->res = NULL;
>
> #if 0
> /* TODO proxy connect */
> @@ -666,29 +610,6 @@ http_request(struct http_connection *con
> }
>
> static int
> -http_write(struct http_connection *conn)
> -{
> - ssize_t s;
> -
> - s = tls_write(conn->tls, conn->buf + conn->bufpos,
> - conn->bufsz - conn->bufpos);
> - if (s == -1) {
> - warnx("%s: TLS write: %s", http_info(conn->url),
> - tls_error(conn->tls));
> - return -1;
> - } else if (s == TLS_WANT_POLLIN) {
> - return WANT_POLLIN;
> - } else if (s == TLS_WANT_POLLOUT) {
> - return WANT_POLLOUT;
> - }
> -
> - conn->bufpos += s;
> - if (conn->bufpos == conn->bufsz)
> - return 0;
> - return WANT_POLLOUT;
> -}
> -
> -static int
> http_parse_status(struct http_connection *conn, char *buf)
> {
> const char *errstr;
> @@ -746,6 +667,46 @@ http_isredirect(struct http_connection *
> }
>
> static int
> +http_redirect(struct http_connection *conn, char *uri)
> +{
> + char *host, *port, *path;
> +
> + logx("redirect to %s", http_info(uri));
> +
> + if (http_parse_uri(uri, &host, &port, &path) == -1) {
> + free(uri);
> + return -1;
> + }
> +
> + free(conn->url);
> + conn->url = uri;
> + free(conn->host);
> + conn->host = host;
> + free(conn->port);
> + conn->port = port;
> + conn->path = path;
> + /* keep modified_since since that is part of the request */
> + free(conn->last_modified);
> + conn->last_modified = NULL;
> + free(conn->buf);
> + conn->buf = NULL;
> + conn->bufpos = 0;
> + conn->bufsz = 0;
> + tls_close(conn->tls);
> + tls_free(conn->tls);
> + conn->tls = NULL;
> + close(conn->fd);
> + conn->state = STATE_INIT;
> +
> + /* TODO proxy support (overload of host and port) */
> +
> + if (http_resolv(conn, host, port) == -1)
> + return -1;
> +
> + return http_connect(conn);
> +}
> +
> +static int
> http_parse_header(struct http_connection *conn, char *buf)
> {
> #define CONTENTLEN "Content-Length: "
> @@ -765,10 +726,10 @@ http_parse_header(struct http_connection
> cp += sizeof(CONTENTLEN) - 1;
> if ((s = strcspn(cp, " \t")) != 0)
> *(cp+s) = 0;
> - conn->filesize = strtonum(cp, 0, LLONG_MAX, &errstr);
> + conn->iosz = strtonum(cp, 0, LLONG_MAX, &errstr);
> if (errstr != NULL) {
> - warnx("Improper response from %s",
> - http_info(conn->url));
> + warnx("Content-Length of %s is %s",
> + http_info(conn->url), errstr);
> return -1;
> }
> } else if (http_isredirect(conn) &&
> @@ -858,7 +819,7 @@ http_parse_chunked(struct http_connectio
> {
> char *header = buf;
> char *end;
> - int chunksize;
> + unsigned long chunksize;
>
> /* ignore empty lines, used between chunk and next header */
> if (*header == '\0')
> @@ -868,14 +829,14 @@ http_parse_chunked(struct http_connectio
> header[strcspn(header, ";\r\n")] = '\0';
> errno = 0;
> chunksize = strtoul(header, &end, 16);
> - if (errno != 0 || header[0] == '\0' || *end != '\0' ||
> - chunksize > INT_MAX) {
> + if (header[0] == '\0' || *end != '\0' || (errno == ERANGE &&
> + chunksize == ULONG_MAX) || chunksize > INT_MAX) {
> warnx("%s: Invalid chunk size", http_info(conn->url));
> return -1;
> }
> - conn->chunksz = chunksize;
> + conn->iosz = chunksize;
>
> - if (conn->chunksz == 0) {
> + if (conn->iosz == 0) {
> http_done(conn, HTTP_OK);
> return 0;
> }
> @@ -933,11 +894,11 @@ http_read(struct http_connection *conn)
> return WANT_POLLIN;
> case STATE_RESPONSE_DATA:
> if (conn->bufpos == conn->bufsz ||
> - conn->filesize - conn->bytes <= (off_t)conn->bufpos)
> + conn->iosz <= (off_t)conn->bufpos)
> return 0;
> return WANT_POLLIN;
> case STATE_RESPONSE_CHUNKED:
> - while (conn->chunksz == 0) {
> + while (conn->iosz == 0) {
> buf = http_get_line(conn);
> if (buf == NULL)
> return WANT_POLLIN;
> @@ -953,7 +914,7 @@ http_read(struct http_connection *conn)
> }
>
> if (conn->bufpos == conn->bufsz ||
> - conn->chunksz <= conn->bufpos)
> + conn->iosz <= (off_t)conn->bufpos)
> return 0;
> return WANT_POLLIN;
> default:
> @@ -962,67 +923,106 @@ http_read(struct http_connection *conn)
> }
>
> static int
> -data_write(struct http_connection *conn)
> +http_write(struct http_connection *conn)
> {
> ssize_t s;
> - size_t bsz = conn->bufpos;
> -
> - if (conn->filesize - conn->bytes < (off_t)bsz)
> - bsz = conn->filesize - conn->bytes;
>
> - s = write(conn->outfd, conn->buf, bsz);
> + s = tls_write(conn->tls, conn->buf + conn->bufpos,
> + conn->bufsz - conn->bufpos);
> if (s == -1) {
> - warn("%s: Data write", http_info(conn->url));
> + warnx("%s: TLS write: %s", http_info(conn->url),
> + tls_error(conn->tls));
> return -1;
> + } else if (s == TLS_WANT_POLLIN) {
> + return WANT_POLLIN;
> + } else if (s == TLS_WANT_POLLOUT) {
> + return WANT_POLLOUT;
> }
>
> - conn->bufpos -= s;
> - conn->bytes += s;
> - memmove(conn->buf, conn->buf + s, conn->bufpos);
> -
> - if (conn->bytes == conn->filesize) {
> - http_done(conn, HTTP_OK);
> + conn->bufpos += s;
> + if (conn->bufpos == conn->bufsz)
> return 0;
> - }
> + return WANT_POLLOUT;
> +}
>
> - if (conn->bufpos == 0)
> - return 0;
> +static int
> +http_close(struct http_connection *conn)
> +{
> + if (conn->tls != NULL) {
> + switch (tls_close(conn->tls)) {
> + case TLS_WANT_POLLIN:
> + return WANT_POLLIN;
> + case TLS_WANT_POLLOUT:
> + return WANT_POLLOUT;
> + case 0:
> + case -1:
> + break;
> + }
> + }
>
> - return WANT_POLLOUT;
> + return -1;
> }
>
> static int
> -chunk_write(struct http_connection *conn)
> +data_write(struct http_connection *conn)
> {
> ssize_t s;
> size_t bsz = conn->bufpos;
>
> - if (bsz > conn->chunksz)
> - bsz = conn->chunksz;
> + if (conn->iosz < (off_t)bsz)
> + bsz = conn->iosz;
>
> s = write(conn->outfd, conn->buf, bsz);
> if (s == -1) {
> - warn("%s: Chunk write", http_info(conn->url));
> + warn("%s: data write", http_info(conn->url));
> return -1;
> }
>
> conn->bufpos -= s;
> - conn->chunksz -= s;
> - conn->bytes += s;
> + conn->iosz -= s;
> memmove(conn->buf, conn->buf + s, conn->bufpos);
>
> - if (conn->bufpos == 0 || conn->chunksz == 0)
> + /* check if regular file transfer is finished */
> + if (conn->iosz == 0 && conn->chunked == 0) {
> + http_done(conn, HTTP_OK);
> + return 0;
> + }
> +
> + /* all data written, switch back to read */
> + if (conn->bufpos == 0 || conn->iosz == 0)
> return 0;
>
> + /* still more data to write in buffer */
> return WANT_POLLOUT;
> }
>
> +/*
> + * Do one IO call depending on the connection state.
> + * Return WANT_POLLIN or WANT_POLLOUT to poll for more data.
> + * If 0 is returned this stage is finished and the protocol should move
> + * to the next stage by calling http_nextstep(). On error return -1.
> + */
> static int
> -http_handle(struct http_connection *conn)
> +http_handle(struct http_connection *conn, int events)
> {
> + if (conn->state == STATE_INIT)
> + return http_connect(conn);
> +
> + if (events & POLLHUP) {
> + if (conn->state == STATE_WRITE_DATA)
> + warnx("%s: output pipe closed", http_info(conn->url));
> + else
> + warnx("%s: server closed connection",
> + http_info(conn->url));
> + return -1;
> + }
> +
> + if ((events & conn->events) == 0) {
> + warnx("%s: unexpected event", http_info(conn->url));
> + return -1;
> + }
> +
> switch (conn->state) {
> - case STATE_INIT:
> - return 0;
> case STATE_CONNECT:
> if (http_finish_connect(conn) == -1)
> /* something went wrong, try other host */
> @@ -1038,28 +1038,34 @@ http_handle(struct http_connection *conn
> case STATE_RESPONSE_CHUNKED:
> return http_read(conn);
> case STATE_WRITE_DATA:
> - if (conn->chunked)
> - return chunk_write(conn);
> - else
> - return data_write(conn);
> + return data_write(conn);
> case STATE_DONE:
> return http_close(conn);
> + case STATE_INIT:
> case STATE_FREE:
> errx(1, "bad http state");
> }
> errx(1, "unknown http state");
> }
>
> +/*
> + * Move the state machine forward until IO needs to happen.
> + * Returns either WANT_POLLIN or WANT_POLLOUT or -1 on error.
> + */
> static int
> http_nextstep(struct http_connection *conn)
> {
> + int r;
> +
> switch (conn->state) {
> case STATE_INIT:
> - conn->state = STATE_CONNECT;
> - return http_connect(conn);
> + errx(1, "bad http state");
> case STATE_CONNECT:
> conn->state = STATE_TLSCONNECT;
> - return http_tls_connect(conn);
> + r = http_tls_connect(conn);
> + if (r != 0)
> + return r;
> + /* FALLTHROUGH */
> case STATE_TLSCONNECT:
> conn->state = STATE_REQUEST;
> return http_request(conn);
> @@ -1107,9 +1113,9 @@ http_nextstep(struct http_connection *co
> }
>
> static int
> -http_do(struct http_connection *conn)
> +http_do(struct http_connection *conn, int events)
> {
> - switch (http_handle(conn)) {
> + switch (http_handle(conn, events)) {
> case -1:
> /* connection failure */
> if (conn->state != STATE_DONE)
> @@ -1129,6 +1135,9 @@ http_do(struct http_connection *conn)
> http_fail(conn->id);
> http_free(conn);
> return -1;
> + case 0:
> + errx(1, "%s: http_nextstep returned 0, state %d",
> + http_info(conn->url), conn->state);
> }
> break;
> case WANT_POLLIN:
> @@ -1219,6 +1228,22 @@ proc_http(char *bind_addr, int fd)
> err(1, "write");
> }
> }
> +
> + /* process active http requests */
> + for (i = 0; i < MAX_CONNECTIONS; i++) {
> + struct http_connection *conn = http_conns[i];
> +
> + if (conn == NULL)
> + continue;
> + /* event not ready */
> + if (pfds[i].revents == 0)
> + continue;
> +
> + if (http_do(conn, pfds[i].revents) == -1)
> + http_conns[i] = NULL;
> + }
> +
> + /* process new requests last */
> if (pfds[MAX_CONNECTIONS].revents & POLLIN) {
> struct http_connection *h;
> size_t id;
> @@ -1236,23 +1261,11 @@ proc_http(char *bind_addr, int fd)
> if (http_conns[i] != NULL)
> continue;
> http_conns[i] = h;
> - if (http_do(h) == -1)
> + if (http_do(h, 0) == -1)
> http_conns[i] = NULL;
> break;
> }
> }
> - }
> - for (i = 0; i < MAX_CONNECTIONS; i++) {
> - struct http_connection *conn = http_conns[i];
> -
> - if (conn == NULL)
> - continue;
> - /* event not ready */
> - if (!(pfds[i].revents & (conn->events | POLLHUP)))
> - continue;
> -
> - if (http_do(conn) == -1)
> - http_conns[i] = NULL;
> }
> }
>
>
