On Thu, Apr 15, 2021 at 03:48:25PM +0200, Claudio Jeker wrote: > 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.
This diff is a bit better, it adds a signal(SIGPIPE, SIG_IGN) early on. There is no way to handle POLLHUP so precisely to never hit a SIGPIPE case. -- :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 14:00:47 -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 */ @@ -1040,7 +1059,6 @@ http_handle(struct http_connection *conn return data_write(conn); case STATE_DONE: return http_close(conn); - case STATE_INIT: case STATE_FREE: errx(1, "bad http state"); } @@ -1058,7 +1076,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 +1094,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"); } @@ -1152,13 +1148,6 @@ http_do(struct http_connection *conn, in return 0; } -static void -gotpipe(int sig __attribute__((unused))) -{ - warnx("http: unexpected sigpipe\n"); - kill(getpid(), SIGABRT); -} - void proc_http(char *bind_addr, int fd) { @@ -1166,9 +1155,6 @@ proc_http(char *bind_addr, int fd) struct pollfd pfds[MAX_CONNECTIONS + 1]; size_t i; int active_connections; - - /* XXX for now track possible SIGPIPE */ - signal(SIGPIPE, gotpipe); if (bind_addr != NULL) { struct addrinfo hints, *res; Index: main.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v retrieving revision 1.136 diff -u -p -r1.136 main.c --- main.c 14 Apr 2021 18:05:47 -0000 1.136 +++ main.c 15 Apr 2021 14:00:18 -0000 @@ -687,6 +687,8 @@ main(int argc, char *argv[]) else if (argc > 1) goto usage; + signal(SIGPIPE, SIG_IGN); + if (timeout) { signal(SIGALRM, suicide); /* Commit suicide eventually - cron will normally start a new one */