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 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.
--
: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;
#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;
}
}
