Send every byte of a short write, from the right offset
send(2) may accept less than it was given and say so, which is not an error and not rare -- it is what a full socket buffer looks like. Both raw-socket writers here got the resumption wrong, in the two available ways. irc's send-all! shrank the length on each pass but passed the same address, so a short write re-sent the head of the line and dropped the tail: the peer got the right number of bytes, the wrong ones, and a stream that no longer framed. Reproduced with c-send stubbed to accept ten bytes first -- "PRIVMSG #chan :abc..." goes out as "PRIVMSG #cPRIVMSG #chan :abc" and the tail is never sent at all. This is the plain-TCP transport, which is what the phone gets. oauth's respond! had no loop: one c-send, return discarded. A short write there truncates the HTTP response under a Content-Length header promising more, on the one page the user is watching for the sign-in to land. So the loop lives in frq.wire now and both call it. The pointer advances with the length, and a non-positive return ends it -- recurring on an unchanged `sent` was an infinite loop that sent nothing, which is worse than the failure it hid. ring-chez's websocket writer, against this same c-send, has had it right all along; this is that loop, minus the EINTR retry, which needs an errno jolt.socket does not publish. Both byte counts are taken as UTF-8 explicitly, because that is what with-c-string writes. The platform default agrees on every machine frq has run on, so this changes nothing today -- but oauth measured a Content-Length in one encoding against a body laid down in another, and that is only ever one host away from being wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7bc4770 parent: fc6923b modified
src/frq/irc.clj +2 -16 | @@ -17,6 +17,7 @@ | ||
| 17 | 17 | (:require [clojure.string :as str] |
| 18 | 18 | [frq.atproto :as atproto] |
| 19 | 19 | [frq.msgsig :as msgsig] |
| 20 | + [frq.wire :as wire] | |
| 20 | 21 | [jolt.ffi :as ffi] |
| 21 | 22 | [jolt.host :as host] |
| 22 | 23 | [jolt.mvn-http :as tls] |
| @@ -26,10 +27,6 @@ | ||
| 26 | 27 | (def ^:private sock-stream 1) |
| 27 | 28 | (def ^:private buffer-size 8192) |
| 28 | 29 | |
| 29 | -;; MSG_NOSIGNAL. Writing to a socket the far end has closed raises SIGPIPE | |
| 30 | -;; otherwise, and nothing here handles signals — the process simply goes. | |
| 31 | -(def ^:private no-signal @#'socket/msg-nosignal) | |
| 32 | - | |
| 33 | 30 | ;; jolt's TLS sockets carry a 30-second receive timeout, so a quiet connection |
| 34 | 31 | ;; reads nothing without being closed. These decide how long that is allowed to |
| 35 | 32 | ;; go on: past `idle-ping-secs` we ask the server whether it is still there, |
| @@ -127,24 +124,13 @@ | ||
| 127 | 124 | |
| 128 | 125 | ;; ---------------------------------------------------------------- transport |
| 129 | 126 | |
| 130 | -(defn- send-all! | |
| 131 | - "send(2) until the whole string is gone — a short write is not an error." | |
| 132 | - [fd text] | |
| 133 | - (let [len (count (.getBytes text))] | |
| 134 | - (ffi/with-c-string [p text] | |
| 135 | - (loop [sent 0] | |
| 136 | - (when (< sent len) | |
| 137 | - (let [n (socket/c-send fd p (- len sent) no-signal)] | |
| 138 | - (when (neg? n) (throw (ex-info "send failed" {:fd fd}))) | |
| 139 | - (recur (+ sent n)))))))) | |
| 140 | - | |
| 141 | 127 | (defn- write! |
| 142 | 128 | "Bytes out, whichever transport this is. TLS callers go through the outbox |
| 143 | 129 | instead — see `send-line!`." |
| 144 | 130 | [conn text] |
| 145 | 131 | (if (= :tls (:kind conn)) |
| 146 | 132 | (tls/tls-write (:tls conn) (.getBytes text)) |
| 147 | - (send-all! (:fd conn) text))) | |
| 133 | + (wire/send-all! (:fd conn) text))) | |
| 148 | 134 | |
| 149 | 135 | (defn- read-chunk! |
| 150 | 136 | "Block for the next chunk as a string, or nil at end of stream." |
| @@ -17,6 +17,7 @@ | |||
| 17 | (:require [clojure.string :as str] | 17 | (:require [clojure.string :as str] |
| 18 | [frq.atproto :as atproto] | 18 | [frq.atproto :as atproto] |
| 19 | [frq.msgsig :as msgsig] | 19 | [frq.msgsig :as msgsig] |
| 20 | + [frq.wire :as wire] | ||
| 20 | [jolt.ffi :as ffi] | 21 | [jolt.ffi :as ffi] |
| 21 | [jolt.host :as host] | 22 | [jolt.host :as host] |
| 22 | [jolt.mvn-http :as tls] | 23 | [jolt.mvn-http :as tls] |
| @@ -26,10 +27,6 @@ | |||
| 26 | (def ^:private sock-stream 1) | 27 | (def ^:private sock-stream 1) |
| 27 | (def ^:private buffer-size 8192) | 28 | (def ^:private buffer-size 8192) |
| 28 | 29 | ||
| 29 | -;; MSG_NOSIGNAL. Writing to a socket the far end has closed raises SIGPIPE | ||
| 30 | -;; otherwise, and nothing here handles signals — the process simply goes. | ||
| 31 | -(def ^:private no-signal @#'socket/msg-nosignal) | ||
| 32 | - | ||
| 33 | ;; jolt's TLS sockets carry a 30-second receive timeout, so a quiet connection | 30 | ;; jolt's TLS sockets carry a 30-second receive timeout, so a quiet connection |
| 34 | ;; reads nothing without being closed. These decide how long that is allowed to | 31 | ;; reads nothing without being closed. These decide how long that is allowed to |
| 35 | ;; go on: past `idle-ping-secs` we ask the server whether it is still there, | 32 | ;; go on: past `idle-ping-secs` we ask the server whether it is still there, |
| @@ -127,24 +124,13 @@ | |||
| 127 | 124 | ||
| 128 | ;; ---------------------------------------------------------------- transport | 125 | ;; ---------------------------------------------------------------- transport |
| 129 | 126 | ||
| 130 | -(defn- send-all! | ||
| 131 | - "send(2) until the whole string is gone — a short write is not an error." | ||
| 132 | - [fd text] | ||
| 133 | - (let [len (count (.getBytes text))] | ||
| 134 | - (ffi/with-c-string [p text] | ||
| 135 | - (loop [sent 0] | ||
| 136 | - (when (< sent len) | ||
| 137 | - (let [n (socket/c-send fd p (- len sent) no-signal)] | ||
| 138 | - (when (neg? n) (throw (ex-info "send failed" {:fd fd}))) | ||
| 139 | - (recur (+ sent n)))))))) | ||
| 140 | - | ||
| 141 | (defn- write! | 127 | (defn- write! |
| 142 | "Bytes out, whichever transport this is. TLS callers go through the outbox | 128 | "Bytes out, whichever transport this is. TLS callers go through the outbox |
| 143 | instead — see `send-line!`." | 129 | instead — see `send-line!`." |
| 144 | [conn text] | 130 | [conn text] |
| 145 | (if (= :tls (:kind conn)) | 131 | (if (= :tls (:kind conn)) |
| 146 | (tls/tls-write (:tls conn) (.getBytes text)) | 132 | (tls/tls-write (:tls conn) (.getBytes text)) |
| 147 | - (send-all! (:fd conn) text))) | 133 | + (wire/send-all! (:fd conn) text))) |
| 148 | 134 | ||
| 149 | (defn- read-chunk! | 135 | (defn- read-chunk! |
| 150 | "Block for the next chunk as a string, or nil at end of stream." | 136 | "Block for the next chunk as a string, or nil at end of stream." |
modified
src/frq/oauth.clj +9 -5 | @@ -13,6 +13,7 @@ | ||
| 13 | 13 | (:require [clojure.string :as str] |
| 14 | 14 | [frq.atproto :as atproto] |
| 15 | 15 | [frq.platform :as platform] |
| 16 | + [frq.wire :as wire] | |
| 16 | 17 | [jolt.ffi :as ffi] |
| 17 | 18 | [jolt.host :as host] |
| 18 | 19 | [jolt.socket :as socket])) |
| @@ -87,12 +88,15 @@ | ||
| 87 | 88 | (defn- respond! [fd body content-type] |
| 88 | 89 | (let [head (str "HTTP/1.1 200 OK\r\nContent-Type: " content-type |
| 89 | 90 | "\r\nConnection: close\r\nContent-Length: " |
| 90 | - (count (.getBytes body)) "\r\n\r\n") | |
| 91 | + (count (.getBytes ^String body "UTF-8")) "\r\n\r\n") | |
| 91 | 92 | text (str head body)] |
| 92 | - (ffi/with-c-string [p text] | |
| 93 | - ;; A closed peer is ordinary here, so a failed write is not an error. | |
| 94 | - (try (socket/c-send fd p (count (.getBytes text)) no-signal) | |
| 95 | - (catch Exception _ -1))))) | |
| 93 | + ;; A closed peer is ordinary here, so a failed write is not an error. A | |
| 94 | + ;; SHORT write is not ordinary: one c-send used to be the whole of this, | |
| 95 | + ;; and the browser was promised a Content-Length the socket had not | |
| 96 | + ;; finished delivering — a hung tab on the one page the user is watching | |
| 97 | + ;; for the sign-in to land. | |
| 98 | + (try (wire/send-all! fd text) | |
| 99 | + (catch Exception _ -1)))) | |
| 96 | 100 | |
| 97 | 101 | (defn- read-request [fd] |
| 98 | 102 | (let [buf (ffi/alloc 16384) |
| @@ -13,6 +13,7 @@ | |||
| 13 | (:require [clojure.string :as str] | 13 | (:require [clojure.string :as str] |
| 14 | [frq.atproto :as atproto] | 14 | [frq.atproto :as atproto] |
| 15 | [frq.platform :as platform] | 15 | [frq.platform :as platform] |
| 16 | + [frq.wire :as wire] | ||
| 16 | [jolt.ffi :as ffi] | 17 | [jolt.ffi :as ffi] |
| 17 | [jolt.host :as host] | 18 | [jolt.host :as host] |
| 18 | [jolt.socket :as socket])) | 19 | [jolt.socket :as socket])) |
| @@ -87,12 +88,15 @@ | |||
| 87 | (defn- respond! [fd body content-type] | 88 | (defn- respond! [fd body content-type] |
| 88 | (let [head (str "HTTP/1.1 200 OK\r\nContent-Type: " content-type | 89 | (let [head (str "HTTP/1.1 200 OK\r\nContent-Type: " content-type |
| 89 | "\r\nConnection: close\r\nContent-Length: " | 90 | "\r\nConnection: close\r\nContent-Length: " |
| 90 | - (count (.getBytes body)) "\r\n\r\n") | 91 | + (count (.getBytes ^String body "UTF-8")) "\r\n\r\n") |
| 91 | text (str head body)] | 92 | text (str head body)] |
| 92 | - (ffi/with-c-string [p text] | 93 | + ;; A closed peer is ordinary here, so a failed write is not an error. A |
| 93 | - ;; A closed peer is ordinary here, so a failed write is not an error. | 94 | + ;; SHORT write is not ordinary: one c-send used to be the whole of this, |
| 94 | - (try (socket/c-send fd p (count (.getBytes text)) no-signal) | 95 | + ;; and the browser was promised a Content-Length the socket had not |
| 95 | - (catch Exception _ -1))))) | 96 | + ;; finished delivering — a hung tab on the one page the user is watching |
| 97 | + ;; for the sign-in to land. | ||
| 98 | + (try (wire/send-all! fd text) | ||
| 99 | + (catch Exception _ -1)))) | ||
| 96 | 100 | ||
| 97 | (defn- read-request [fd] | 101 | (defn- read-request [fd] |
| 98 | (let [buf (ffi/alloc 16384) | 102 | (let [buf (ffi/alloc 16384) |
added
src/frq/wire.clj +48 -0 | new file mode 100644 | ||
| @@ -0,0 +1,48 @@ | ||
| 1 | +(ns frq.wire | |
| 2 | + "Bytes onto a raw socket, all of them. | |
| 3 | + | |
| 4 | + `send(2)` is allowed to accept less than it was given and say so, which is | |
| 5 | + not an error and not rare — it is what a full socket buffer looks like. The | |
| 6 | + caller has to resume from where it stopped, and the only way to say that to | |
| 7 | + the kernel is a pointer further along the buffer: shrinking the length while | |
| 8 | + passing the same address re-sends the head of the line and loses the tail. | |
| 9 | + The peer then gets the right number of bytes, the wrong ones, and a stream | |
| 10 | + that no longer frames. | |
| 11 | + | |
| 12 | + Both raw-socket writers in frq had their own version of this loop and both | |
| 13 | + had it wrong, so there is one here instead. TLS does not come through — that | |
| 14 | + transport is `tls/tls-write`, which handles its own record boundaries." | |
| 15 | + (:require [jolt.ffi :as ffi] | |
| 16 | + [jolt.socket :as socket])) | |
| 17 | + | |
| 18 | +(def ^:private no-signal | |
| 19 | + "MSG_NOSIGNAL: a write to a closed peer returns EPIPE rather than killing | |
| 20 | + the process with SIGPIPE. Read from jolt rather than spelled here, as both | |
| 21 | + call sites already did — the value is the platform's, not frq's." | |
| 22 | + @#'socket/msg-nosignal) | |
| 23 | + | |
| 24 | +(defn send-all! | |
| 25 | + "Write `text` to `fd` until none is left. Throws if the socket does. | |
| 26 | + | |
| 27 | + The byte count is taken as UTF-8 explicitly, because that is what | |
| 28 | + `with-c-string` writes: the platform default agrees on every machine frq | |
| 29 | + has run on, but a machine where it did not would send a length measured in | |
| 30 | + one encoding against bytes laid down in another." | |
| 31 | + [fd text] | |
| 32 | + (let [len (count (.getBytes ^String text "UTF-8"))] | |
| 33 | + (ffi/with-c-string [p text] | |
| 34 | + (loop [sent 0] | |
| 35 | + (when (< sent len) | |
| 36 | + ;; The pointer advances with the length. `p` is an address, so this | |
| 37 | + ;; is ordinary arithmetic on it. | |
| 38 | + (let [n (socket/c-send fd (+ p sent) (- len sent) no-signal)] | |
| 39 | + ;; Anything not positive ends it. Zero especially: recurring on an | |
| 40 | + ;; unchanged `sent` is an infinite loop that sends nothing, which | |
| 41 | + ;; is worse than the failure it is hiding. | |
| 42 | + ;; ponytail: EINTR is thrown rather than retried — jolt.socket | |
| 43 | + ;; publishes no errno, so telling it from a real error would mean | |
| 44 | + ;; binding __errno_location here. Worth doing if signals ever | |
| 45 | + ;; start interrupting these writes in practice. | |
| 46 | + (when-not (pos? n) | |
| 47 | + (throw (ex-info "send failed" {:fd fd :sent sent :len len :ret n}))) | |
| 48 | + (recur (+ sent n)))))))) | |
| new file mode 100644 | |||
| @@ -0,0 +1,48 @@ | |||
| 1 | +(ns frq.wire | ||
| 2 | + "Bytes onto a raw socket, all of them. | ||
| 3 | + | ||
| 4 | + `send(2)` is allowed to accept less than it was given and say so, which is | ||
| 5 | + not an error and not rare — it is what a full socket buffer looks like. The | ||
| 6 | + caller has to resume from where it stopped, and the only way to say that to | ||
| 7 | + the kernel is a pointer further along the buffer: shrinking the length while | ||
| 8 | + passing the same address re-sends the head of the line and loses the tail. | ||
| 9 | + The peer then gets the right number of bytes, the wrong ones, and a stream | ||
| 10 | + that no longer frames. | ||
| 11 | + | ||
| 12 | + Both raw-socket writers in frq had their own version of this loop and both | ||
| 13 | + had it wrong, so there is one here instead. TLS does not come through — that | ||
| 14 | + transport is `tls/tls-write`, which handles its own record boundaries." | ||
| 15 | + (:require [jolt.ffi :as ffi] | ||
| 16 | + [jolt.socket :as socket])) | ||
| 17 | + | ||
| 18 | +(def ^:private no-signal | ||
| 19 | + "MSG_NOSIGNAL: a write to a closed peer returns EPIPE rather than killing | ||
| 20 | + the process with SIGPIPE. Read from jolt rather than spelled here, as both | ||
| 21 | + call sites already did — the value is the platform's, not frq's." | ||
| 22 | + @#'socket/msg-nosignal) | ||
| 23 | + | ||
| 24 | +(defn send-all! | ||
| 25 | + "Write `text` to `fd` until none is left. Throws if the socket does. | ||
| 26 | + | ||
| 27 | + The byte count is taken as UTF-8 explicitly, because that is what | ||
| 28 | + `with-c-string` writes: the platform default agrees on every machine frq | ||
| 29 | + has run on, but a machine where it did not would send a length measured in | ||
| 30 | + one encoding against bytes laid down in another." | ||
| 31 | + [fd text] | ||
| 32 | + (let [len (count (.getBytes ^String text "UTF-8"))] | ||
| 33 | + (ffi/with-c-string [p text] | ||
| 34 | + (loop [sent 0] | ||
| 35 | + (when (< sent len) | ||
| 36 | + ;; The pointer advances with the length. `p` is an address, so this | ||
| 37 | + ;; is ordinary arithmetic on it. | ||
| 38 | + (let [n (socket/c-send fd (+ p sent) (- len sent) no-signal)] | ||
| 39 | + ;; Anything not positive ends it. Zero especially: recurring on an | ||
| 40 | + ;; unchanged `sent` is an infinite loop that sends nothing, which | ||
| 41 | + ;; is worse than the failure it is hiding. | ||
| 42 | + ;; ponytail: EINTR is thrown rather than retried — jolt.socket | ||
| 43 | + ;; publishes no errno, so telling it from a real error would mean | ||
| 44 | + ;; binding __errno_location here. Worth doing if signals ever | ||
| 45 | + ;; start interrupting these writes in practice. | ||
| 46 | + (when-not (pos? n) | ||
| 47 | + (throw (ex-info "send failed" {:fd fd :sent sent :len len :ret n}))) | ||
| 48 | + (recur (+ sent n)))))))) | ||