open
https://gitlab.synchro.net/main/sbbs/-/issues/1194
*(Filed by Claude on Rob's behalf.)*
`sbbs_t::WaitForOutbufDrained()` was added in 9cf170c024 to close the "`empty_event` means ring-drained, not sent" gap (#1157), and 473e880ec1 now uses it in `sbbs_t::hangup()` so the last output before a disconnect is transmitted before teardown. On **SSH** that guarantee does not actually hold: the drain wait proves the bytes reached *cryptlib*, not the wire. It's the same class of defect as #1157, one layer further down the stack — the pipeline is `outbuf` ring buffer -> `output_thread` linear buffer -> **cryptlib `sendBuffer`** -> socket, and we only verify the first two hops.
This is a latent gap, not a regression; the fast-log-off (`/O`) echo loss that prompted the audit is fixed. Filing to get @Deuce's read on the right remedy before anyone writes one, since it lives on the cryptlib boundary.
Line references below are against cryptlib 3.4.8 as vendored/patched in `3rdp/dist/cryptlib.zip`.
## 1. `cryptPushData()` reports bytes *buffered*, not bytes *sent*
`putSessionData()` (`session/sess_rw.c`) `memcpy`s the caller's data into `sessionInfoPtr->sendBuffer` and returns `CRYPT_OK` with `*bytesCopied` set to the full length whenever it fits. `flushData()` -> `swrite()` runs only when the
buffer fills or on an explicit flush. Terminal-server writes are clamped to the MSS (~536-1460 bytes) against a send buffer several times that, so in practice **every push is a pure `memcpy` and nothing reaches the socket until our `cryptFlushData()`**.
`output_thread()` (`main.cpp`) does `bufbot += i;` with `i` from `cryptPushData()`, then:
```c
if (bufbot == buftop) // linear buffer fully transmitted
sbbs->output_thread_busy = false;
```
The comment says "transmitted"; on SSH the value only means "copied into cryptlib". `WaitForOutbufDrained()` waits on exactly that flag, so its contract holds on SSH only for as long as the `cryptFlushData()` immediately after the push succeeds.
## 2. A timed-out flush is treated as a completed send, and never retried
```c
if (cryptStatusError((err = cryptFlushData(sbbs->ssh_session)))) {
GCESSTR(err, node, sbbs->ssh_session, "flushing data");
if (err != CRYPT_ERROR_TIMEOUT) {
sbbs->online = false;
i = buftop - bufbot; // Pretend we sent it all
}
}
```
Not dropping the session on `CRYPT_ERROR_TIMEOUT` is right — a slow client isn't
a dead one. But the code then falls through with `i` still holding the *pushed* count, so `bufbot` advances to `buftop` and `output_thread_busy` clears, i.e. a partial write is recorded as a completed one.
On cryptlib's side the remainder is retained, not lost: `flushData()` sets `partialWrite = TRUE` and leaves `sendBufPartialBufPos ... sendBufPos` unsent, so
it goes out on the *next* push or flush. The problem is that there may not be a next one — with `bufbot == buftop`, `output_thread` returns to `WaitForEvent(outbuf.data_event, 1000)` and only re-enters the send block when new output is queued. **A goodbye message or a hang-up echo is precisely the case
where no further output ever arrives**, so the remainder sits in `sendBuffer` until the session is destroyed.
## 3. Session teardown does not force the buffer out
`closeSession()` (`session/session.c`) calls the SSH `shutdownFunction()` (`session/ssh2.c`), which in the data-transfer phase is just `closeChannel(sessionInfoPtr, TRUE)`, then `sNetDisconnect()`. Nothing performs a
blocking flush of `sendBuffer`.
`closeChannel()` does end up in `encodeSendResponse()` (`session/ssh2_chn.c`), which appends the channel-close packet after the pending remainder and calls `putSessionData(NULL, 0)` — so cryptlib *will* try to write remainder + close-packet together. But `putSessionData()` starts with:
```c
sioctlSet( &sessionInfoPtr->stream, STREAM_IOCTL_WRITETIMEOUT,
sessionInfoPtr->writeTimeout );
```
...and our "READ = WRITE TIMEOUT HACK" sets `CRYPT_OPTION_NET_WRITETIMEOUT` back
to **0** after every flush. So that close-time write is non-blocking: if the socket isn't writable at that instant it times out with nothing written, `closeChannel()` ignores the error, `sNetDisconnect()` runs, and the data is gone. (Contrast `sendCloseNotification()`, which deliberately raises the write timeout to 5-15s for a clean shutdown — but that path is only taken when the handshake never completed.)
## 4. cryptlib itself flags the resulting state as unexpected
In `encodeSendResponse()`, the branch reached when a partial write is pending (so `sendBufPos != sendBufStartOfs` and `adjustedStartOffset` stays `FALSE`) is:
```c
else
{
assert( 0 ); /* When does this occur? */
sessionInfoPtr->sendBufPos += encodedResponseSize;
}
```
Closing a channel while a partial write is outstanding is a state the author didn't expect to see. A debug cryptlib build would trip that assert; ours is a release build, so it falls through silently.
## Impact
Every current `WaitForOutbufDrained()` caller inherits the hole on SSH:
- `sbbs_t::hangup()` (473e880ec1) — followed immediately by
`ssh_session_destroy()`
- the `nonodes.txt` / node-init-failure disconnects in `main.cpp` (9cf170c024) - `trashcan_msg()` in `str.cpp`
Trigger is a client that stops reading for >5s (zero window / suspended terminal / flaky link) with a hang-up right behind it. Rare, and telnet is unaffected — `sendsocket()` hands off to the kernel send buffer, which survives
`close()`.
## Possible remedies (input wanted)
1. **Track the pending flush.** On `CRYPT_ERROR_TIMEOUT`, leave
`output_thread_busy` set (or add a distinct `ssh_flush_pending` atomic) and
have `output_thread` re-attempt `cryptFlushData()` on subsequent iterations
instead of going idle. `WaitForOutbufDrained()` then blocks on the real
condition and its timeout bounds the wait.
2. **Raise the write timeout for the final flush.** Before
`ssh_session_destroy()`, set `CRYPT_OPTION_NET_WRITETIMEOUT` to a few seconds
and issue one last `cryptFlushData()`, so the close-time write can block
rather than being non-blocking by accident.
3. **Stop resetting the write timeout to 0.** The "READ = WRITE TIMEOUT HACK"
comment says the reset exists because the read timeout tracks the write
timeout, which matters for `input_thread`'s `cryptPopData()`. Since both
threads serialize on `ssh_mutex`, is a persistent non-zero write timeout
actually safe now? That would fix the teardown case without extra
bookkeeping. (Related: the 5s write timeout is set *while holding
`ssh_mutex`*, so one slow flush can stall `input_thread` for 5s.)
## Unrelated bug noticed in the same block
`main.cpp` computes the 8KB clamp but never applies it:
```c
size_t sendbytes = buftop - bufbot;
if (sendbytes > 0x2000)
sendbytes = 0x2000;
if (cryptStatusError((err = cryptPushData(sbbs->ssh_session, (char*)buf + bufbot, buftop - bufbot, &i)))) {
```
`sendbytes` is dead — the push uses `buftop - bufbot`. Both sibling call sites
pass it (`sftp.cpp` `cryptPushData(..., sendbytes, &i)`, `websrvr.cpp` `cryptPushData(..., sendbytes, &tls_sent)`). It's harmless where `getsockopt(TCP_MAXSEG)` succeeds, since `mss` then caps `avail` well under 8KB,
but where `TCP_MAXSEG` is unavailable `mss` stays `IO_THREAD_BUF_SIZE` (20000) and pushes up to ~20KB bypass the intended limit. Because `sendbytes` is *read* by the `if`, no compiler warns about it.
--- SBBSecho 3.37-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)