diff --git a/docs/architecture/networking.md b/docs/architecture/networking.md index 85d5788..1e5b2a2 100644 --- a/docs/architecture/networking.md +++ b/docs/architecture/networking.md @@ -406,6 +406,36 @@ $ curl -s -o /dev/null -w '%{http_code}\n' --path-as-is host/%2e%2e/etc/passwd 403 ``` +### A listening socket is a different object + +Worth its own note, because it cost a kernel panic and the shape of it is not +obvious from the API. + +`tcp_listen()` does not put a pcb into a listening state. It **frees the +connection pcb and returns a new, smaller one** — a `struct tcp_pcb_listen`, +which has no `recv`, `sent`, `err` or `poll` field at all. The two are +different types behind one `struct tcp_pcb *`, and the only callback a +listening one has is `accept`. + +`inet_close()` did not know that, and cleared the callbacks the same way for +both. On a listening socket that is a write past the end of the allocation; +lwIP asserts rather than allowing it, and an assert here is a panic: + +``` +lwip assert: invalid socket state for recv callback at tcp.c:2024 + #1 tcp_recv #2 inet_close #3 sockclose + #4 fileclose #5 exit #6 syscall +``` + +Which is **a user process taking the machine down by pressing Ctrl-C** — the +close path runs from `exit()`, so any program that listened and then died did +it. Nothing had, because until `cmd/httpd` nothing in the tree listened on TCP +and exited; `schdbgd` would have done it too, a few days later. + +The lesson is not about lwIP. It is that `listening` was already tracked one +field away in `struct inet_sock` and the close path did not consult it — the +information was there and the code did not ask. + ## What it does not do - **`ping` is a first-party program**, `cmd/ping`, not toybox's. `recvmsg` now @@ -435,6 +465,10 @@ $ curl -s -o /dev/null -w '%{http_code}\n' --path-as-is host/%2e%2e/etc/passwd `Connection: close`, because the directory listing is generated straight into the socket and so has no `Content-Length` to give. One request per connection is the cost. +- **A queued-but-never-accepted connection is closed without clearing its + callbacks.** `inet_close()` clears the argument and closes, which is enough + because the pcb is freed — but it is a narrower guarantee than the one the + main path makes, and it relies on `tcp_close()` succeeding. - **`httpd` has no timeouts.** A client that connects and says nothing ties up a child process until it goes away; `SO_RCVTIMEO` does not exist here (see below), so there is nothing to set. diff --git a/kernel/net/inet.c b/kernel/net/inet.c index 8fbb977..14170bd 100644 --- a/kernel/net/inet.c +++ b/kernel/net/inet.c @@ -955,13 +955,41 @@ inet_close(void *h) if (is->tpcb) { // Clearing the argument first: lwIP may still deliver a callback for this // pcb after tcp_close, and it would otherwise arrive with a pointer to a - // socket that has just been freed. + // socket that has just been freed. `callback_arg` is in the part of the + // structure both kinds of pcb share, so this one is safe either way. tcp_arg(is->tpcb, 0); - tcp_recv(is->tpcb, 0); - tcp_err(is->tpcb, 0); - if (tcp_close(is->tpcb) != ERR_OK) - tcp_abort(is->tpcb); + + // **A listening pcb is a different, smaller structure.** tcp_listen() + // frees the connection pcb and returns a `struct tcp_pcb_listen`, which + // has no recv, sent, err or poll field at all -- so tcp_recv() on one does + // not merely set something useless, it writes past the end of the + // allocation. lwIP asserts rather than allowing that, and the assert is a + // kernel panic here: + // + // lwip assert: invalid socket state for recv callback tcp.c:2024 + // #1 tcp_recv #2 inet_close #3 sockclose #4 fileclose #5 exit + // + // which is a user process taking the machine down by pressing Ctrl-C. + // Nothing had ever closed a listening TCP socket before -- the first + // program to listen and then exit was cmd/httpd, and it found this on its + // first run. + // + // The callback a listening pcb *does* have is `accept`, and clearing that + // is the equivalent gesture. + if (is->listening) { + tcp_accept(is->tpcb, 0); + // Always ERR_OK for a LISTEN pcb, and tcp_abort() would be wrong on one: + // it reaches for connection state that a listening pcb does not have. + tcp_close(is->tpcb); + } else { + tcp_recv(is->tpcb, 0); + tcp_err(is->tpcb, 0); + tcp_sent(is->tpcb, 0); + if (tcp_close(is->tpcb) != ERR_OK) + tcp_abort(is->tpcb); + } is->tpcb = 0; + is->listening = 0; } if (is->upcb) { udp_recv(is->upcb, 0, 0);