|
1
|
================================================================================================
|
|
2
|
PROPOSAL - [MINOR] functional - p2j.socket.js collectChunk
|
|
3
|
The MAX_CHUNK_BYTES abandon path poisons the transfer it abandons
|
|
4
|
================================================================================================
|
|
5
|
|
|
6
|
REVIEW FINDING (abridged)
|
|
7
|
"When the MAX_CHUNK_BYTES (64 MiB) guard trips mid-transfer, resetChunkTransfers() drops the
|
|
8
|
partial data and zeroes chunkBytes, but nothing records the poisoned transferId and the sender is
|
|
9
|
never told. Later MSG_CHUNK pieces for that id fall into if (!parts) [...] and reassembly
|
|
10
|
silently resumes mid-stream [...] Because chunkBytes is an aggregate released only on completion,
|
|
11
|
any transfer stranded by a non-IOException failure in the body read [...] also accumulates
|
|
12
|
permanently and lowers the threshold."
|
|
13
|
|
|
14
|
FILES
|
|
15
|
src/com/goldencode/p2j/ui/client/driver/web/res/p2j.socket.js
|
|
16
|
src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java
|
|
17
|
|
|
18
|
------------------------------------------------------------------------------------------------
|
|
19
|
1. ROOT CAUSE
|
|
20
|
------------------------------------------------------------------------------------------------
|
|
21
|
|
|
22
|
Three separate defects meet in the same guard (p2j.socket.js:3212).
|
|
23
|
|
|
24
|
(a) Resuming a poisoned transfer. resetChunkTransfers() empties the table and returns null, but
|
|
25
|
the sender knows nothing and keeps emitting pieces for the same id. They land on the
|
|
26
|
if (!parts) branch, start a fresh array, and the piece with flags bit 0 assembles a message
|
|
27
|
from an arbitrary tail - messageHandler then runs whole[0], a raw pixel component, as a
|
|
28
|
message type.
|
|
29
|
|
|
30
|
(b) Collateral damage. The cap is global and the reset is global, so one oversized transfer
|
|
31
|
discards every unrelated in-flight transfer as well.
|
|
32
|
|
|
33
|
(c) An accounting leak that makes (a) and (b) reachable with ordinary-sized images. chunkBytes is
|
|
34
|
only ever decremented on successful completion (:3239). sendBinaryMessageStreamed catches
|
|
35
|
only IOException (WebClientProtocol.java:981), while RawImageInputStream.read can raise
|
|
36
|
ArrayIndexOutOfBoundsException on an unguarded pixels[p] when the requested region exceeds
|
|
37
|
the screen array. That escapes without the terminating queueChunk(..., true), so the peer
|
|
38
|
holds those pieces forever and the aggregate creeps toward the cap.
|
|
39
|
|
|
40
|
Direct reachability of the cap itself is real but extreme: bodyLen = (long) width * height * 4 is
|
|
41
|
uncapped, and the virtual screen is sized from window.innerWidth/innerHeight in CSS pixels, so a
|
|
42
|
display zoomed out to 25% reports ~7680x4320 and a maximized STRETCH-TO-FIT image reaches 132 MB.
|
|
43
|
(c) is the cheaper route to the same place.
|
|
44
|
|
|
45
|
------------------------------------------------------------------------------------------------
|
|
46
|
2. PROPOSED FIX
|
|
47
|
------------------------------------------------------------------------------------------------
|
|
48
|
|
|
49
|
2.1 Make the cap per-transfer and abandon only the offender
|
|
50
|
-------------------------------------------------------------
|
|
51
|
|
|
52
|
+ /** The most one transfer may buffer before it is abandoned. */
|
|
53
|
+ var MAX_TRANSFER_BYTES = 64 * 1024 * 1024;
|
|
54
|
|
|
55
|
function collectChunk(message)
|
|
56
|
{
|
|
57
|
...
|
|
58
|
parts.push(piece);
|
|
59
|
chunkBytes += piece.length;
|
|
60
|
+ parts.bytes = (parts.bytes || 0) + piece.length;
|
|
61
|
|
|
62
|
- if (chunkBytes > MAX_CHUNK_BYTES)
|
|
63
|
- {
|
|
64
|
- p2j.logger.error("Abandoning incomplete message transfers, buffered " + chunkBytes +
|
|
65
|
- " bytes");
|
|
66
|
- resetChunkTransfers();
|
|
67
|
- return null;
|
|
68
|
- }
|
|
69
|
+ if (parts.bytes > MAX_TRANSFER_BYTES)
|
|
70
|
+ {
|
|
71
|
+ p2j.logger.error("Abandoning message transfer " + id + ", buffered " + parts.bytes +
|
|
72
|
+ " bytes");
|
|
73
|
+ dropTransfer(id);
|
|
74
|
+ return null;
|
|
75
|
+ }
|
|
76
|
+
|
|
77
|
+ if (chunkBytes > MAX_CHUNK_BYTES)
|
|
78
|
+ {
|
|
79
|
+ // aggregate backstop: individually legal transfers that are never completed
|
|
80
|
+ p2j.logger.error("Abandoning all incomplete message transfers, buffered " + chunkBytes +
|
|
81
|
+ " bytes");
|
|
82
|
+ resetChunkTransfers();
|
|
83
|
+ return null;
|
|
84
|
+ }
|
|
85
|
|
|
86
|
with
|
|
87
|
|
|
88
|
+ /**
|
|
89
|
+ * Discard one incomplete transfer and release its accounting.
|
|
90
|
+ *
|
|
91
|
+ * @param {number} id
|
|
92
|
+ * The transfer id to drop.
|
|
93
|
+ */
|
|
94
|
+ function dropTransfer(id)
|
|
95
|
+ {
|
|
96
|
+ var parts = chunkTransfers[id];
|
|
97
|
+ if (parts)
|
|
98
|
+ {
|
|
99
|
+ chunkBytes -= (parts.bytes || 0);
|
|
100
|
+ delete chunkTransfers[id];
|
|
101
|
+ }
|
|
102
|
+ }
|
|
103
|
|
|
104
|
The aggregate cap is kept as a backstop but is now the unusual case rather than the first line of
|
|
105
|
defence, so the collateral wipe in (b) becomes rare instead of routine.
|
|
106
|
|
|
107
|
2.2 Do not resume a dropped transfer
|
|
108
|
--------------------------------------
|
|
109
|
|
|
110
|
The first-piece flag from proposal-issue-critical-functional-1 section 2.2 already does this: after
|
|
111
|
dropTransfer the id has no entry, and its subsequent pieces do not carry CHUNK_FLAG_FIRST, so they
|
|
112
|
are discarded with a log line instead of starting a new reassembly. No extra bookkeeping - no
|
|
113
|
"poisoned ids" set - is required, which is why that flag is worth taking first.
|
|
114
|
|
|
115
|
If for some reason the flag is not taken, this proposal needs its own poisoned-id set with an
|
|
116
|
eviction policy, which is strictly worse; prefer the flag.
|
|
117
|
|
|
118
|
2.3 Terminate a transfer stranded by a runtime exception
|
|
119
|
----------------------------------------------------------
|
|
120
|
|
|
121
|
In WebClientProtocol.sendBinaryMessageStreamed, widen the catch so any failure to read the body
|
|
122
|
still closes the transfer at the peer:
|
|
123
|
|
|
124
|
- catch (IOException e)
|
|
125
|
+ catch (IOException | RuntimeException e)
|
|
126
|
{
|
|
127
|
queueChunk(generation, transferId, new byte[CHUNK_HEADER_SIZE], 0, false, true);
|
|
128
|
LOG.log(Level.WARNING, "Failed to stream binary message body", e);
|
|
129
|
}
|
|
130
|
|
|
131
|
RuntimeException rather than Throwable: an Error should continue to propagate. This also fixes the
|
|
132
|
underlying report honestly - the terminator now runs for the ArrayIndexOutOfBoundsException case
|
|
133
|
that RawImageInputStream can raise - without masking the bug, since the WARNING still carries the
|
|
134
|
stack trace.
|
|
135
|
|
|
136
|
Optionally, guard the source instead: RawImageInputStream could clamp its region to the screen
|
|
137
|
bounds and report a short read. That is the better long-term fix but touches image rendering, so it
|
|
138
|
is out of scope here; the catch widening is the containment.
|
|
139
|
|
|
140
|
------------------------------------------------------------------------------------------------
|
|
141
|
3. RELATIONSHIP TO OTHER PROPOSALS
|
|
142
|
------------------------------------------------------------------------------------------------
|
|
143
|
|
|
144
|
proposal-issue-critical-functional-1 supplies CHUNK_FLAG_FIRST, which is what makes 2.2 free.
|
|
145
|
Its queueChunk signature change is assumed by 2.3.
|
|
146
|
proposal-issue-major-functional-1 removes the leak that is the main practical route to this
|
|
147
|
cap being hit at all.
|
|
148
|
|
|
149
|
------------------------------------------------------------------------------------------------
|
|
150
|
4. RISK AND VERIFICATION
|
|
151
|
------------------------------------------------------------------------------------------------
|
|
152
|
|
|
153
|
Risk: low. The per-transfer cap is strictly tighter than the aggregate one for a single transfer and
|
|
154
|
strictly looser for many small ones, which is the intended change. Hanging a bytes property off the
|
|
155
|
parts array is idiomatic enough here but a small {pieces: [], bytes: 0} object would read better if
|
|
156
|
the surrounding style prefers it.
|
|
157
|
|
|
158
|
Verification
|
|
159
|
1. Temporarily lower MAX_TRANSFER_BYTES to a few hundred KB and push one large image. Expected:
|
|
160
|
one "Abandoning message transfer <id>" line, chunkBytes returns to 0, subsequent pieces log
|
|
161
|
"Discarding orphaned message piece", and no garbage dispatch or canvas corruption.
|
|
162
|
2. With the lowered cap, run two transfers concurrently (a font and an image) and confirm only
|
|
163
|
the offender is dropped - the other still completes and renders.
|
|
164
|
3. Force the stranded-transfer case: temporarily make RawImageInputStream.read throw
|
|
165
|
ArrayIndexOutOfBoundsException partway, and confirm the peer receives a terminating piece and
|
|
166
|
chunkBytes returns to 0 rather than leaking.
|
|
167
|
4. Confirm normal large-image and custom-font rendering is unaffected.
|