Skip to content

Commit a38eb3c

Browse files
markt-asfmarkt-asf
committed
Fix BZ 70126. permessage-deflate may drop bytes of inflated frame
Fix written by GPT-5.5 Test case written by Hironori Ichimiya
1 parent 1484a54 commit a38eb3c

2 files changed

Lines changed: 123 additions & 16 deletions

File tree

‎java/org/apache/tomcat/websocket/PerMessageDeflate.java‎

Lines changed: 77 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,14 @@ public Transformation build(List<List<Parameter>> preferences, boolean isServer)
6868
private final boolean isServer;
6969
private final Inflater inflater = new Inflater(true);
7070
private final ByteBuffer readBuffer = ByteBuffer.allocate(Constants.DEFAULT_BUFFER_SIZE);
71+
private final byte[] eomOverflowBuffer = new byte[1];
7172
private final Deflater deflater = new Deflater(Deflater.DEFAULT_COMPRESSION, true);
7273
private final byte[] EOM_BUFFER = new byte[EOM_BYTES.length + 1];
7374

7475
private volatile Transformation next;
7576
private volatile boolean skipDecompression = false;
77+
private volatile boolean eomBytesInserted = false;
78+
private volatile boolean eomOverflowWritten = false;
7679
private volatile ByteBuffer writeBuffer = ByteBuffer.allocate(Constants.DEFAULT_BUFFER_SIZE);
7780
private volatile boolean firstCompressedFrameWritten = false;
7881
// Flag to track if a message is completely empty
@@ -211,10 +214,23 @@ public TransformationResult getMoreData(byte opCode, boolean fin, int rsv, ByteB
211214
return next.getMoreData(opCode, fin, rsv, dest);
212215
}
213216

217+
if (eomOverflowWritten) {
218+
if (!dest.hasRemaining()) {
219+
return TransformationResult.OVERFLOW;
220+
}
221+
dest.put(eomOverflowBuffer[0]);
222+
eomOverflowWritten = false;
223+
if (!dest.hasRemaining()) {
224+
if (inflateEomBytes()) {
225+
return TransformationResult.OVERFLOW;
226+
}
227+
return endFrame(fin);
228+
}
229+
}
230+
214231
int written;
215-
boolean usedEomBytes = false;
216232

217-
while (dest.remaining() > 0 || usedEomBytes) {
233+
while (dest.hasRemaining()) {
218234
// Space available in destination. Try and fill it.
219235
try {
220236
written = inflater.inflate(dest.array(), dest.arrayOffset() + dest.position(), dest.remaining());
@@ -226,7 +242,7 @@ public TransformationResult getMoreData(byte opCode, boolean fin, int rsv, ByteB
226242
}
227243
dest.position(dest.position() + written);
228244

229-
if (inflater.needsInput() && !usedEomBytes) {
245+
if (inflater.needsInput() && !eomBytesInserted) {
230246
readBuffer.clear();
231247
TransformationResult nextResult = next.getMoreData(opCode, fin, (rsv ^ RSV_BITMASK), readBuffer);
232248
inflater.setInput(readBuffer.array(), readBuffer.arrayOffset(), readBuffer.position());
@@ -236,33 +252,78 @@ public TransformationResult getMoreData(byte opCode, boolean fin, int rsv, ByteB
236252
} else if (TransformationResult.END_OF_FRAME.equals(nextResult) && readBuffer.position() == 0) {
237253
if (fin) {
238254
inflater.setInput(EOM_BYTES);
239-
usedEomBytes = true;
255+
eomBytesInserted = true;
240256
} else {
241-
return TransformationResult.END_OF_FRAME;
257+
return endFrame(fin);
242258
}
243259
}
244260
} else if (readBuffer.position() > 0) {
245261
return TransformationResult.OVERFLOW;
246-
} else if (fin) {
247-
inflater.setInput(EOM_BYTES);
248-
usedEomBytes = true;
249-
}
250-
} else if (written == 0) {
251-
if (fin && (isServer && !clientContextTakeover || !isServer && !serverContextTakeover)) {
252-
try {
253-
inflater.reset();
254-
} catch (NullPointerException e) {
255-
throw new IOException(sm.getString("perMessageDeflate.alreadyClosed"), e);
262+
} else if (TransformationResult.END_OF_FRAME.equals(nextResult)) {
263+
if (fin) {
264+
if (inflateEomBytes()) {
265+
return TransformationResult.OVERFLOW;
266+
}
256267
}
268+
return endFrame(fin);
269+
} else if (TransformationResult.UNDERFLOW.equals(nextResult)) {
270+
return nextResult;
257271
}
258-
return TransformationResult.END_OF_FRAME;
272+
} else if (written == 0) {
273+
return endFrame(fin);
274+
}
275+
}
276+
277+
if (eomBytesInserted) {
278+
if (inflateEomBytes()) {
279+
return TransformationResult.OVERFLOW;
259280
}
281+
return endFrame(fin);
260282
}
261283

262284
return TransformationResult.OVERFLOW;
263285
}
264286

265287

288+
private boolean inflateEomBytes() throws IOException {
289+
if (!eomBytesInserted) {
290+
inflater.setInput(EOM_BYTES);
291+
eomBytesInserted = true;
292+
}
293+
294+
int written;
295+
try {
296+
written = inflater.inflate(eomOverflowBuffer, 0, eomOverflowBuffer.length);
297+
} catch (DataFormatException e) {
298+
throw new IOException(sm.getString("perMessageDeflate.deflateFailed"), e);
299+
} catch (IllegalStateException | NullPointerException e) {
300+
// As of Java 25, the JRE throws an ISE rather than an NPE
301+
throw new IOException(sm.getString("perMessageDeflate.alreadyClosed"), e);
302+
}
303+
304+
if (written > 0) {
305+
eomOverflowWritten = true;
306+
return true;
307+
}
308+
309+
return false;
310+
}
311+
312+
313+
private TransformationResult endFrame(boolean fin) throws IOException {
314+
eomBytesInserted = false;
315+
eomOverflowWritten = false;
316+
if (fin && (isServer && !clientContextTakeover || !isServer && !serverContextTakeover)) {
317+
try {
318+
inflater.reset();
319+
} catch (NullPointerException e) {
320+
throw new IOException(sm.getString("perMessageDeflate.alreadyClosed"), e);
321+
}
322+
}
323+
return TransformationResult.END_OF_FRAME;
324+
}
325+
326+
266327
@Override
267328
public boolean validateRsv(int rsv, byte opCode) {
268329
if (Util.isControl(opCode)) {

‎test/org/apache/tomcat/websocket/TestPerMessageDeflate.java‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import java.nio.ByteBuffer;
2121
import java.nio.charset.StandardCharsets;
2222
import java.util.ArrayList;
23+
import java.util.Arrays;
2324
import java.util.Collections;
2425
import java.util.List;
2526

@@ -101,6 +102,51 @@ public void testMessagePartThatFillsBuffer() throws IOException {
101102
}
102103

103104

105+
@Test
106+
public void testMessagePartThatOverfillsBuffer() throws IOException {
107+
108+
List<Parameter> parameters = Collections.emptyList();
109+
List<List<Parameter>> preferences = new ArrayList<>();
110+
preferences.add(parameters);
111+
112+
List<String> truncated = new ArrayList<>();
113+
114+
for (int size = 8192 + 1; size <= 8192 + 512; size++) {
115+
116+
// Compress `size` identical (highly compressible) bytes as one binary message.
117+
PerMessageDeflate perMessageDeflateTx = PerMessageDeflate.build(preferences, true);
118+
perMessageDeflateTx.setNext(new TesterTransformation());
119+
byte[] data = new byte[size];
120+
Arrays.fill(data, (byte) 0x80);
121+
List<MessagePart> uncompressedParts = new ArrayList<>();
122+
uncompressedParts.add(new MessagePart(true, 0, Constants.OPCODE_BINARY,
123+
ByteBuffer.wrap(data), null, null, -1));
124+
MessagePart compressedPart = perMessageDeflateTx.sendMessagePart(uncompressedParts).get(0);
125+
126+
// Decompress the way WsFrameBase.processDataBinary does: fill an 8192
127+
// byte buffer, loop on OVERFLOW, stop on END_OF_FRAME.
128+
PerMessageDeflate perMessageDeflateRx = PerMessageDeflate.build(preferences, true);
129+
perMessageDeflateRx.setNext(new TesterTransformation(compressedPart.getPayload()));
130+
131+
int total = 0;
132+
ByteBuffer received = ByteBuffer.allocate(8192);
133+
TransformationResult tr;
134+
do {
135+
tr = perMessageDeflateRx.getMoreData(compressedPart.getOpCode(), compressedPart.isFin(),
136+
compressedPart.getRsv(), received);
137+
total += received.position();
138+
received.clear();
139+
} while (tr == TransformationResult.OVERFLOW);
140+
141+
if (total != size) {
142+
truncated.add("size=" + size + " recovered=" + total);
143+
}
144+
}
145+
146+
Assert.assertTrue("permessage-deflate dropped trailing bytes for: " + truncated, truncated.isEmpty());
147+
}
148+
149+
104150
/*
105151
* https://bz.apache.org/bugzilla/show_bug.cgi?id=66681
106152
*/

0 commit comments

Comments
 (0)