oxidecomputer / propolis

VMM userspace for illumos bhyve
Mozilla Public License 2.0
181 stars 22 forks source link

server serial task's send-to-listeners future is not cancel-safe #650

Open gjcolombo opened 9 months ago

gjcolombo commented 9 months ago

Discovered this during some ad hoc testing with multiple connections to a single guest's serial console, where I observed some data being sent a second time to a listener that had already received it.

If the serial console task has read some data from the guest that needs to be written to listeners, it generates a future to do that writing and then includes it in the main select! that determines the task's next action (lines 116-125 below where cur_output is Some): https://github.com/oxidecomputer/propolis/blob/1b34075ee5dd4235dc7eb46061e1e648fb4d121b/bin/propolis-server/src/lib/serial/mod.rs#L109-L130

cur_output is only set to None if this future is the one chosen by select!: https://github.com/oxidecomputer/propolis/blob/1b34075ee5dd4235dc7eb46061e1e648fb4d121b/bin/propolis-server/src/lib/serial/mod.rs#L226-L230

But if this branch is not chosen, for_each_concurrent may have sent the current output bytes to some listeners and not to others. (The underlying websocket send may not be cancel-safe either but I haven't looked at its implementation.) This will cause the same data to be re-sent in the next loop iteration.

An approach along the lines suggested in RFD 400 section 5.2 might help here (and other approaches may work too):

gjcolombo commented 9 months ago

I'm also a little unsure about the read-from-clients side of this: https://github.com/oxidecomputer/propolis/blob/1b34075ee5dd4235dc7eb46061e1e648fb4d121b/bin/propolis-server/src/lib/serial/mod.rs#L132-L141

If ws.next() reads a message from a client, but another select! branch is chosen before the message can be sent through send_ch, I think the message is lost (per Sender::send's documentation of its cancel safety).