This one is annoying. Again poll handling is throwing me a curve ball.
On MacOS X POLLHUP is signaled super early and many downloads fail.
The issue here is that tls_read() and tls_write() are not like read() and
write() from a poll perspective. While parsing the HTTP header for example
one should call tls_read() until a TLS_WANT_* return happens. Only then a
pass via poll() should happen.
In the current code there are many cases where WANT_POLLIN is returned
where actually another tls_read() should happen. It is similar but not as
bad for tls_write().
This diff tries to cleanup the mess a bit. In http_read() WANT_POLLIN
should only be triggered by tls_read(). In most cases we try to push
forward through multiple states as long as possible. I solved this with
two goto labels in http_read().
Because of this some of the http_nextstep() handling was pulled into
http_read() and e.g. data_write() is now also directly calling into
http_read() instead of passing via poll().
I tried this on OpenBSD, Linux and MacOS X and this version seems to work
on all three systems.
Please test since I may have missed or introduced another poll related
issue.
--
:wq Claudio
Index: http.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.28
diff -u -p -r1.28 http.c
--- http.c 13 Apr 2021 13:54:15 -0000 1.28
+++ http.c 15 Apr 2021 13:24:57 -0000
@@ -123,6 +123,8 @@ struct tls_config *tls_config;
uint8_t *tls_ca_mem;
size_t tls_ca_size;
+static int http_write(struct http_connection *);
+
/*
* Return a string that can be used in error message to identify the
* connection.
@@ -699,7 +701,7 @@ http_redirect(struct http_connection *co
if (http_resolv(conn, host, port) == -1)
return -1;
- return http_connect(conn);
+ return -2;
}
static int
@@ -846,7 +848,9 @@ http_read(struct http_connection *conn)
{
ssize_t s;
char *buf;
+ int done;
+read_more:
s = tls_read(conn->tls, conn->buf + conn->bufpos,
conn->bufsz - conn->bufpos);
if (s == -1) {
@@ -867,38 +871,63 @@ http_read(struct http_connection *conn)
conn->bufpos += s;
+again:
switch (conn->state) {
case STATE_RESPONSE_STATUS:
buf = http_get_line(conn);
if (buf == NULL)
- return WANT_POLLIN;
+ goto read_more;
if (http_parse_status(conn, buf) == -1) {
free(buf);
return -1;
}
free(buf);
conn->state = STATE_RESPONSE_HEADER;
- /* FALLTHROUGH */
+ goto again;
case STATE_RESPONSE_HEADER:
- while ((buf = http_get_line(conn)) != NULL) {
+ done = 0;
+ while (!done) {
int rv;
+ buf = http_get_line(conn);
+ if (buf == NULL)
+ goto read_more;
+
rv = http_parse_header(conn, buf);
free(buf);
- if (rv != 1)
- return rv;
+ if (rv == -1)
+ return -1;
+ if (rv == -2) /* redirect */
+ return 0;
+ if (rv == 0)
+ done = 1;
}
- return WANT_POLLIN;
+
+ /* Check status header and decide what to do next */
+ if (conn->status == 200) {
+ if (conn->chunked)
+ conn->state = STATE_RESPONSE_CHUNKED;
+ else
+ conn->state = STATE_RESPONSE_DATA;
+ goto again;
+ } else if (conn->status == 304) {
+ http_done(conn->id, HTTP_NOT_MOD, conn->last_modified);
+ } else {
+ http_done(conn->id, HTTP_FAILED, conn->last_modified);
+ }
+
+ conn->state = STATE_DONE;
+ return 0;
case STATE_RESPONSE_DATA:
if (conn->bufpos == conn->bufsz ||
conn->iosz <= (off_t)conn->bufpos)
return 0;
- return WANT_POLLIN;
+ goto read_more;
case STATE_RESPONSE_CHUNKED:
while (conn->iosz == 0) {
buf = http_get_line(conn);
if (buf == NULL)
- return WANT_POLLIN;
+ goto read_more;
switch (http_parse_chunked(conn, buf)) {
case -1:
free(buf);
@@ -913,7 +942,7 @@ http_read(struct http_connection *conn)
if (conn->bufpos == conn->bufsz ||
conn->iosz <= (off_t)conn->bufpos)
return 0;
- return WANT_POLLIN;
+ goto read_more;
default:
errx(1, "unexpected http state");
}
@@ -939,6 +968,7 @@ http_write(struct http_connection *conn)
conn->bufpos += s;
if (conn->bufpos == conn->bufsz)
return 0;
+
return WANT_POLLOUT;
}
@@ -987,8 +1017,13 @@ data_write(struct http_connection *conn)
}
/* all data written, switch back to read */
- if (conn->bufpos == 0 || conn->iosz == 0)
- return 0;
+ if (conn->bufpos == 0 || conn->iosz == 0) {
+ if (conn->chunked)
+ conn->state = STATE_RESPONSE_CHUNKED;
+ else
+ conn->state = STATE_RESPONSE_DATA;
+ return http_read(conn);
+ }
/* still more data to write in buffer */
return WANT_POLLOUT;
@@ -1003,25 +1038,9 @@ data_write(struct http_connection *conn)
static int
http_handle(struct http_connection *conn, int events)
{
- /* special case for INIT, there is no event to start a request */
- 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 http_connect(conn);
case STATE_CONNECT:
if (http_finish_connect(conn) == -1)
/* something went wrong, try other host */
@@ -1037,10 +1056,13 @@ http_handle(struct http_connection *conn
case STATE_RESPONSE_CHUNKED:
return http_read(conn);
case STATE_WRITE_DATA:
+ if (events & POLLHUP) {
+ warnx("%s: output closed", http_info(conn->url));
+ return -1;
+ }
return data_write(conn);
case STATE_DONE:
return http_close(conn);
- case STATE_INIT:
case STATE_FREE:
errx(1, "bad http state");
}
@@ -1058,7 +1080,7 @@ http_nextstep(struct http_connection *co
switch (conn->state) {
case STATE_INIT:
- errx(1, "bad http state");
+ return http_connect(conn);
case STATE_CONNECT:
conn->state = STATE_TLSCONNECT;
r = http_tls_connect(conn);
@@ -1076,38 +1098,16 @@ http_nextstep(struct http_connection *co
err(1, NULL);
conn->bufpos = 0;
conn->bufsz = HTTP_BUF_SIZE;
- return WANT_POLLIN;
- case STATE_RESPONSE_STATUS:
- conn->state = STATE_RESPONSE_HEADER;
- return WANT_POLLIN;
- case STATE_RESPONSE_HEADER:
- if (conn->status == 200) {
- conn->state = STATE_RESPONSE_DATA;
- } else {
- if (conn->status == 304)
- http_done(conn->id, HTTP_NOT_MOD,
- conn->last_modified);
- else
- http_done(conn->id, HTTP_FAILED,
- conn->last_modified);
- conn->state = STATE_DONE;
- return http_close(conn);
- }
- return WANT_POLLIN;
+ return http_read(conn);
case STATE_RESPONSE_DATA:
- conn->state = STATE_WRITE_DATA;
- return WANT_POLLOUT;
case STATE_RESPONSE_CHUNKED:
conn->state = STATE_WRITE_DATA;
return WANT_POLLOUT;
- case STATE_WRITE_DATA:
- if (conn->chunked)
- conn->state = STATE_RESPONSE_CHUNKED;
- else
- conn->state = STATE_RESPONSE_DATA;
- return WANT_POLLIN;
case STATE_DONE:
return http_close(conn);
+ case STATE_RESPONSE_STATUS:
+ case STATE_RESPONSE_HEADER:
+ case STATE_WRITE_DATA:
case STATE_FREE:
errx(1, "bad http state");
}
