• js_socket: TLS send failures are indistinguishable from short writes

    From Rob Swindell@1:103/705 to GitLab issue in main/sbbs on Sat Jul 25 17:50:27 2026
    open https://gitlab.synchro.net/main/sbbs/-/issues/1199

    ## Summary

    `js_socket_sendsocket()` (`src/sbbs3/js_socket.cpp:381`) reports send failures two different ways depending on whether the socket is TLS-wrapped:

    * **Plaintext** (`p->session == -1`, `:388`) — returns `sendsocket()`'s result,
    i.e. `-1` on error.
    * **TLS** (`:389-416`) — returns a byte count for *every* outcome, including a
    session closed by the peer (`:404-410` returns `total`, typically `0`).

    So on a TLS session a caller cannot distinguish "connection closed" from "short write", and a routine client disconnect is indistinguishable from a real I/O error.

    ## Observed

    ```
    Sat Jul 25 17:31:36 2026 master/9fc5505cbc vert.synchro.net
    srvc 3408 WSS Error: SendToWebSocketClientVersion7 only sent 0 of 138 bytes
    ```

    A `wss://` client went away while the Terminal Server still had a 138-byte chunk queued for it. `exec/websocketservice.js` guards its write with `client.socket.is_connected`, which only reflects the TCP socket — the cryptlib
    session was already gone, `cryptPushData()` failed, and `Socket.send()` returned
    `0`.

    The literal `0` is the TLS signature: on the plaintext `WS` service a failed `sendsocket()` returns `-1`, `js_send()` leaves the return value `JSVAL_VOID` (`:1175-1178`), and the same message would have read `only sent undefined of 138 bytes`. The absence of a companion `TLS ... pushing data` error line points specifically at `CRYPT_ERROR_COMPLETE`, which `:405` deliberately doesn't log.

    The WSS symptom itself is addressed separately in `exec/websocketservice.js` (treat a short write as a disconnect and break the proxy loop rather than throwing). This issue covers the C side.

    ## Proposed changes

    **1. Return `-1` from the TLS branch on failure** — but only when nothing was copied. `:404-410` can be reached after a partial push, and that count must still be returned or the caller will resend already-delivered bytes.

    **2. Fix `js_socket_sendfilesocket()` in the same change — `:435`.** This is a
    prerequisite, not a follow-up:

    ```c
    ptrdiff_t wr = js_socket_sendsocket(p, buf + sent, rd - sent, false);
    if (wr > 0) { sent += wr; }
    else if (wr == SOCKET_ERROR && SOCKET_ERRNO == EWOULDBLOCK) { wr = 0; SLEEP(1); }
    else { ...return wr; }
    ```

    It is the only caller that inspects `SOCKET_ERROR`, and it consults `SOCKET_ERRNO` unconditionally. For a TLS session that errno belongs to cryptlib's own I/O on the underlying socket, where a leftover `EWOULDBLOCK`/`WSAEWOULDBLOCK` is routine. A `-1` return would then take the middle branch, reset `wr` to 0, sleep 1ms, and re-run `while (sent < rd)` with `sent` unadvanced — an infinite loop hanging the JS service thread. Today's `0`
    return falls into the final `else` and unwinds cleanly. Fix by gating the errno check on `p->session == -1`, or by having `js_socket_sendsocket()` distinguish "would block" from "closed" instead of overloading `-1`.

    **3. Cleanup: `js_sendbin()` declares `size_t wr` (`:1362`).** The `-1` from the
    plaintext path already lands there as `SIZE_MAX`; the `wr == (size_t)size` test at `:1401` then fails and the correct error path is taken — right answer, wrong
    reason. Make it `ptrdiff_t`.

    Verified as already handling `-1` correctly and needing no change:
    `js_send()` (`:1172`, `if (ret >= 0)`) and `js_sendline()` (`:1211`, `== len` with short-circuit).

    ## Do NOT surface `-1` to JavaScript

    `js_send()` leaves the return value `undefined` on error, and stock scripts are written against that convention. Changing `Socket.send()` to return `-1` would break:

    * `exec/imapservice.js:198` — `full_send()` tests `sret == undefined ||
    sret == 0`; `-1` slips through, `sent` goes negative, and `str.substr(-1)`
    re-sends the tail in a loop.
    * `exec/load/http.js:106` — `do_send()` tests `ret === 0 || null || false`;
    `-1` slips through and `sent` decreases monotonically, so
    `while (sent < str.length)` never terminates.
    * `exec/broker.js:922` (tests `bytes === null`) and `exec/load/ircd/core.js:587`
    (`if (sent > 0)`) degrade rather than hang — the transmit buffer stops
    draining.

    `Socket.sendline()` and `Socket.sendBin()` return booleans and are unaffected.

    — *Authored by Claude (Claude Code), on behalf of @rswindell*
    --- SBBSecho 3.37-Linux
    * Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)
  • From Deucе@1:103/705 to GitLab note in main/sbbs on Sat Jul 25 20:10:49 2026
    https://gitlab.synchro.net/main/sbbs/-/issues/1199#note_9712

    A few comments on the issue description:

    on a TLS session a caller cannot distinguish "connection closed" from "short write"

    Neither can the "Plaintext" socket object.

    a routine client disconnect is indistinguishable from a real I/O error.

    In connected sockets, a routine client disconnect is distinguished by a recv(), not a send(). The remote *cannot* cleanly close the send side of the socket in this end, it can only indicate that is will send no more data. Only *non-routine* client disconnects are distinguished by send().

    js_sendbin() declares size_t wr (:1362). The -1 from the plaintext path already lands there as SIZE_MAX; the wr == (size_t)size test at :1401 then fails and the correct error path is taken — right answer, wrong reason. Make it ptrdiff_t.

    ptrdiff_t is for the result of subtracting two pointers, it is *not* a signed size type... we have ssize_t for that.
    --- SBBSecho 3.37-Linux
    * Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)
  • From Deucе@1:103/705 to GitLab note in main/sbbs on Sat Jul 25 20:18:35 2026
    https://gitlab.synchro.net/main/sbbs/-/issues/1199#note_9713

    As an aside, the `ptrdiff_t` return values are from before we added `ssize_t` to xpdev and are explicitly a hack for that (see 0319521a9c3)
    --- SBBSecho 3.37-Linux
    * Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)
  • From Rob Swindell@1:103/705 to GitLab note in main/sbbs on Sat Jul 25 20:39:07 2026
    https://gitlab.synchro.net/main/sbbs/-/issues/1199#note_9717

    All three corrections taken; I've rewritten the description accordingly.

    On the disconnect framing — you're right, and it invalidates how I stated the problem rather than just the wording. A peer closing its send side is invisible to our `send()`; only a non-routine teardown surfaces there, so "a routine client disconnect is indistinguishable from a real I/O error" is wrong on both socket types. (The `websocketservice.js` side already learns about routine disconnects from the recv path returning null — the change there only covers the non-routine case.)

    What I think still stands, stated more narrowly: on the TLS path a `0` return is ambiguous between *"would block, retry"* — `:395-396`, nonblocking with a full buffer, `CRYPT_OK` with `copied == 0` — and *"the push failed"* — `:404-410`. The plaintext path separates those as `0` vs `-1`. That's an error-vs-partial-write distinction, not a closed-vs-error one, and it's what produced the literal `0` in the log line. The `SOCKET_ERRNO == EWOULDBLOCK` hazard in `js_socket_sendfilesocket()` is independent of all of this.

    On `ptrdiff_t` — understood, and thanks for the pointer to 0319521a9c3 (bras-22-limits). `gen_defs.h:201,270` now typedefs `ssize_t` from `SSIZE_T` for MSVC/Watcom/Borland, so the constraint that commit worked around is gone. Suggesting `ptrdiff_t` for `js_sendbin()`'s `wr` was propagating the hack; the description now says `ssize_t`, and notes that `js_socket_sendsocket()` and `js_socket_sendfilesocket()` carry the same `ptrdiff_t`/`off_t` residue if you want them converted in the same pass.

    — *Authored by Claude (Claude Code), on behalf of @rswindell*
    --- SBBSecho 3.37-Linux
    * Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)