================================================================================================ PROPOSAL - [MINOR] functional - p2j.socket.js collectChunk The MAX_CHUNK_BYTES abandon path poisons the transfer it abandons ================================================================================================ REVIEW FINDING (abridged) "When the MAX_CHUNK_BYTES (64 MiB) guard trips mid-transfer, resetChunkTransfers() drops the partial data and zeroes chunkBytes, but nothing records the poisoned transferId and the sender is never told. Later MSG_CHUNK pieces for that id fall into if (!parts) [...] and reassembly silently resumes mid-stream [...] Because chunkBytes is an aggregate released only on completion, any transfer stranded by a non-IOException failure in the body read [...] also accumulates permanently and lowers the threshold." FILES src/com/goldencode/p2j/ui/client/driver/web/res/p2j.socket.js src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java ------------------------------------------------------------------------------------------------ 1. ROOT CAUSE ------------------------------------------------------------------------------------------------ Three separate defects meet in the same guard (p2j.socket.js:3212). (a) Resuming a poisoned transfer. resetChunkTransfers() empties the table and returns null, but the sender knows nothing and keeps emitting pieces for the same id. They land on the if (!parts) branch, start a fresh array, and the piece with flags bit 0 assembles a message from an arbitrary tail - messageHandler then runs whole[0], a raw pixel component, as a message type. (b) Collateral damage. The cap is global and the reset is global, so one oversized transfer discards every unrelated in-flight transfer as well. (c) An accounting leak that makes (a) and (b) reachable with ordinary-sized images. chunkBytes is only ever decremented on successful completion (:3239). sendBinaryMessageStreamed catches only IOException (WebClientProtocol.java:981), while RawImageInputStream.read can raise ArrayIndexOutOfBoundsException on an unguarded pixels[p] when the requested region exceeds the screen array. That escapes without the terminating queueChunk(..., true), so the peer holds those pieces forever and the aggregate creeps toward the cap. Direct reachability of the cap itself is real but extreme: bodyLen = (long) width * height * 4 is uncapped, and the virtual screen is sized from window.innerWidth/innerHeight in CSS pixels, so a display zoomed out to 25% reports ~7680x4320 and a maximized STRETCH-TO-FIT image reaches 132 MB. (c) is the cheaper route to the same place. ------------------------------------------------------------------------------------------------ 2. PROPOSED FIX ------------------------------------------------------------------------------------------------ 2.1 Make the cap per-transfer and abandon only the offender ------------------------------------------------------------- + /** The most one transfer may buffer before it is abandoned. */ + var MAX_TRANSFER_BYTES = 64 * 1024 * 1024; function collectChunk(message) { ... parts.push(piece); chunkBytes += piece.length; + parts.bytes = (parts.bytes || 0) + piece.length; - if (chunkBytes > MAX_CHUNK_BYTES) - { - p2j.logger.error("Abandoning incomplete message transfers, buffered " + chunkBytes + - " bytes"); - resetChunkTransfers(); - return null; - } + if (parts.bytes > MAX_TRANSFER_BYTES) + { + p2j.logger.error("Abandoning message transfer " + id + ", buffered " + parts.bytes + + " bytes"); + dropTransfer(id); + return null; + } + + if (chunkBytes > MAX_CHUNK_BYTES) + { + // aggregate backstop: individually legal transfers that are never completed + p2j.logger.error("Abandoning all incomplete message transfers, buffered " + chunkBytes + + " bytes"); + resetChunkTransfers(); + return null; + } with + /** + * Discard one incomplete transfer and release its accounting. + * + * @param {number} id + * The transfer id to drop. + */ + function dropTransfer(id) + { + var parts = chunkTransfers[id]; + if (parts) + { + chunkBytes -= (parts.bytes || 0); + delete chunkTransfers[id]; + } + } The aggregate cap is kept as a backstop but is now the unusual case rather than the first line of defence, so the collateral wipe in (b) becomes rare instead of routine. 2.2 Do not resume a dropped transfer -------------------------------------- The first-piece flag from proposal-issue-critical-functional-1 section 2.2 already does this: after dropTransfer the id has no entry, and its subsequent pieces do not carry CHUNK_FLAG_FIRST, so they are discarded with a log line instead of starting a new reassembly. No extra bookkeeping - no "poisoned ids" set - is required, which is why that flag is worth taking first. If for some reason the flag is not taken, this proposal needs its own poisoned-id set with an eviction policy, which is strictly worse; prefer the flag. 2.3 Terminate a transfer stranded by a runtime exception ---------------------------------------------------------- In WebClientProtocol.sendBinaryMessageStreamed, widen the catch so any failure to read the body still closes the transfer at the peer: - catch (IOException e) + catch (IOException | RuntimeException e) { queueChunk(generation, transferId, new byte[CHUNK_HEADER_SIZE], 0, false, true); LOG.log(Level.WARNING, "Failed to stream binary message body", e); } RuntimeException rather than Throwable: an Error should continue to propagate. This also fixes the underlying report honestly - the terminator now runs for the ArrayIndexOutOfBoundsException case that RawImageInputStream can raise - without masking the bug, since the WARNING still carries the stack trace. Optionally, guard the source instead: RawImageInputStream could clamp its region to the screen bounds and report a short read. That is the better long-term fix but touches image rendering, so it is out of scope here; the catch widening is the containment. ------------------------------------------------------------------------------------------------ 3. RELATIONSHIP TO OTHER PROPOSALS ------------------------------------------------------------------------------------------------ proposal-issue-critical-functional-1 supplies CHUNK_FLAG_FIRST, which is what makes 2.2 free. Its queueChunk signature change is assumed by 2.3. proposal-issue-major-functional-1 removes the leak that is the main practical route to this cap being hit at all. ------------------------------------------------------------------------------------------------ 4. RISK AND VERIFICATION ------------------------------------------------------------------------------------------------ Risk: low. The per-transfer cap is strictly tighter than the aggregate one for a single transfer and strictly looser for many small ones, which is the intended change. Hanging a bytes property off the parts array is idiomatic enough here but a small {pieces: [], bytes: 0} object would read better if the surrounding style prefers it. Verification 1. Temporarily lower MAX_TRANSFER_BYTES to a few hundred KB and push one large image. Expected: one "Abandoning message transfer " line, chunkBytes returns to 0, subsequent pieces log "Discarding orphaned message piece", and no garbage dispatch or canvas corruption. 2. With the lowered cap, run two transfers concurrently (a font and an image) and confirm only the offender is dropped - the other still completes and renders. 3. Force the stranded-transfer case: temporarily make RawImageInputStream.read throw ArrayIndexOutOfBoundsException partway, and confirm the peer receives a terminating piece and chunkBytes returns to 0 rather than leaking. 4. Confirm normal large-image and custom-font rendering is unaffected.