net: closing a listening socket panicked the kernel
Ctrl-C on httpd took the whole machine down:
lwip assert: invalid socket state for recv callback at tcp.c:2024
#1 tcp_recv #2 inet_close #3 sockclose #4 fileclose #5 exit
tcp_listen() does not put a pcb into a listening state -- it frees the
connection pcb and returns a *different, smaller* structure, a
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() cleared the callbacks identically for both, so on a listening
socket tcp_recv() wrote past the end of the allocation. lwIP asserts rather
than allowing that, and an assert here is a panic -- which means any program
that listened and then exited killed the kernel, because the close path runs
from exit(). A user process, ending normally, halting the system.
Nothing had ever done it: until cmd/httpd nothing in the tree listened on TCP
and exited. schdbgd would have found it a few days later.
inet_close() now branches on is->listening -- which was already there, one
field away, and the close path simply never asked. The listening case clears
accept and calls tcp_close (always ERR_OK for a LISTEN pcb, and tcp_abort
would be wrong on one: it reaches for connection state that does not exist).
The connected case also clears `sent`, which it had been leaving set.
Verified: Ctrl-C the listener, no panic, and a second httpd binds port 80
again -- which is the part that proves the pcb was actually freed. Six
requests after the change, all 200.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQ7Qx8E1q1M1bbCq8QLHS2
This commit is contained in:
parent
4a032c9c62
commit
6db9cc1230
2 changed files with 67 additions and 5 deletions
|
|
@ -406,6 +406,36 @@ $ curl -s -o /dev/null -w '%{http_code}\n' --path-as-is host/%2e%2e/etc/passwd
|
||||||
403
|
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
|
## What it does not do
|
||||||
|
|
||||||
- **`ping` is a first-party program**, `cmd/ping`, not toybox's. `recvmsg` now
|
- **`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
|
`Connection: close`, because the directory listing is generated straight into
|
||||||
the socket and so has no `Content-Length` to give. One request per connection
|
the socket and so has no `Content-Length` to give. One request per connection
|
||||||
is the cost.
|
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
|
- **`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
|
a child process until it goes away; `SO_RCVTIMEO` does not exist here (see
|
||||||
below), so there is nothing to set.
|
below), so there is nothing to set.
|
||||||
|
|
|
||||||
|
|
@ -955,13 +955,41 @@ inet_close(void *h)
|
||||||
if (is->tpcb) {
|
if (is->tpcb) {
|
||||||
// Clearing the argument first: lwIP may still deliver a callback for this
|
// 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
|
// 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_arg(is->tpcb, 0);
|
||||||
tcp_recv(is->tpcb, 0);
|
|
||||||
tcp_err(is->tpcb, 0);
|
// **A listening pcb is a different, smaller structure.** tcp_listen()
|
||||||
if (tcp_close(is->tpcb) != ERR_OK)
|
// frees the connection pcb and returns a `struct tcp_pcb_listen`, which
|
||||||
tcp_abort(is->tpcb);
|
// 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->tpcb = 0;
|
||||||
|
is->listening = 0;
|
||||||
}
|
}
|
||||||
if (is->upcb) {
|
if (is->upcb) {
|
||||||
udp_recv(is->upcb, 0, 0);
|
udp_recv(is->upcb, 0, 0);
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue