This is an automated email from the ASF dual-hosted git repository. markt-asf pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 0697b66bd823daa23e91c3e3c63fc4765cebc66d Author: Mark Thomas <[email protected]> AuthorDate: Tue Sep 8 19:32:47 2026 +0100 Fix per-message-deflate with non-final blocks that have BFINAL set --- .../apache/tomcat/websocket/PerMessageDeflate.java | 69 +++++- .../tomcat/websocket/TestPerMessageDeflate.java | 253 +++++++++++++++++++-- webapps/docs/changelog.xml | 5 + 3 files changed, 308 insertions(+), 19 deletions(-) diff --git a/java/org/apache/tomcat/websocket/PerMessageDeflate.java b/java/org/apache/tomcat/websocket/PerMessageDeflate.java index 10ffa1dbbc..502b5d0b91 100644 --- a/java/org/apache/tomcat/websocket/PerMessageDeflate.java +++ b/java/org/apache/tomcat/websocket/PerMessageDeflate.java @@ -76,6 +76,16 @@ public class PerMessageDeflate implements Transformation { private volatile boolean skipDecompression = false; private volatile boolean eomBytesInserted = false; private volatile boolean eomOverflowWritten = false; + /* + * Offset and length, within readBuffer's backing array, of the compressed bytes most recently passed to + * inflater.setInput(). Used to work out where the unconsumed tail starts if inflater.finished() becomes true before + * all of those bytes have been consumed (see getMoreData()). Both fields must be kept in sync with whatever the + * most recent setInput() call actually used - lastInputOffset is not always readBuffer.arrayOffset(): after the + * first such recovery, the next input segment starts wherever the previous one left off, not at the start of + * readBuffer's backing array. + */ + private volatile int lastInputOffset; + private volatile int lastInputLength; private volatile ByteBuffer writeBuffer = ByteBuffer.allocate(Constants.DEFAULT_BUFFER_SIZE); private volatile boolean firstCompressedFrameWritten = false; // Flag to track if a message is completely empty @@ -238,6 +248,8 @@ public class PerMessageDeflate implements Transformation { if (inflater.needsInput() && !eomBytesInserted) { readBuffer.clear(); TransformationResult nextResult = next.getMoreData(opCode, fin, (rsv ^ RSV_BITMASK), readBuffer); + lastInputOffset = readBuffer.arrayOffset(); + lastInputLength = readBuffer.position(); inflater.setInput(readBuffer.array(), readBuffer.arrayOffset(), readBuffer.position()); if (dest.hasRemaining()) { if (TransformationResult.UNDERFLOW.equals(nextResult)) { @@ -267,7 +279,37 @@ public class PerMessageDeflate implements Transformation { sm.getString("perMessageDeflate.next.ise", next.getClass().getName())); } } else if (written == 0) { - return endFrame(fin); + if (!eomBytesInserted && inflater.finished() && inflater.getRemaining() > 0) { + /* + * RFC 7692 section 7.2.1 permits an endpoint to compress a single message using multiple DEFLATE + * blocks with any mix of BFINAL values, including a block with BFINAL=1 that is not the last block + * of the message. + * + * If inflater is finished without EOM bytes being inserted and with data still to process this + * indicates there is at least one more block to process. Inflater has no API to continue once it + * has finished. From this point on, it silently ignores any further input. The only way to process + * the remaining, still-unconsumed bytes belonging to this same message is to reset() the Inflater + * (clearing the finished state) and feed it just the unconsumed tail. + */ + int remaining = inflater.getRemaining(); + /* + * The unconsumed tail starts wherever the *current* input segment started, not necessarily at + * readBuffer.arrayOffset(): if this is the second (or later) recovery for the same message, the + * current segment already starts partway into readBuffer. + */ + int newOffset = lastInputOffset + lastInputLength - remaining; + try { + inflater.reset(); + } catch (NullPointerException e) { + throw new IOException(sm.getString("perMessageDeflate.alreadyClosed"), e); + } + inflater.setInput(readBuffer.array(), newOffset, remaining); + lastInputOffset = newOffset; + lastInputLength = remaining; + // Continue decompression loop + } else { + return endFrame(fin); + } } } @@ -313,11 +355,26 @@ public class PerMessageDeflate implements Transformation { private TransformationResult endFrame(boolean fin) throws IOException { eomBytesInserted = false; eomOverflowWritten = false; - if (fin && (isServer && !clientContextTakeover || !isServer && !serverContextTakeover)) { - try { - inflater.reset(); - } catch (NullPointerException e) { - throw new IOException(sm.getString("perMessageDeflate.alreadyClosed"), e); + if (fin) { + boolean contextTakeover = isServer ? clientContextTakeover : serverContextTakeover; + /* + * If the message's final block was itself an independently BFINAL=1 terminated block (see the recovery + * in getMoreData()), the EOM_BYTES appended to complete the message per RFC 7692 section 7.2.2 were fed + * to an already-finished Inflater and were never consumed: inflater.finished() stays true with those 4 + * bytes still reported by getRemaining(). Left in that state, the next message would compute its first + * recovery offset from this stale, unrelated leftover count, which can go negative. There is no way to + * continue decompressing past a finished Inflater in place, so it has to be reset here too - even though + * context takeover is enabled - at the cost of losing the window for the *next* message in this, + * otherwise rare, case. + */ + if (!contextTakeover || inflater.finished()) { + try { + inflater.reset(); + } catch (NullPointerException e) { + throw new IOException(sm.getString("perMessageDeflate.alreadyClosed"), e); + } + lastInputOffset = 0; + lastInputLength = 0; } } return TransformationResult.END_OF_FRAME; diff --git a/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java b/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java index 74a77d112e..93a7ede4a3 100644 --- a/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java +++ b/test/org/apache/tomcat/websocket/TestPerMessageDeflate.java @@ -16,6 +16,7 @@ */ package org.apache.tomcat.websocket; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; @@ -23,6 +24,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.zip.Deflater; import jakarta.websocket.Extension; import jakarta.websocket.Extension.Parameter; @@ -47,16 +49,14 @@ public class TestPerMessageDeflate { perMessageDeflate.setNext(new TesterTransformation()); ByteBuffer bb1 = ByteBuffer.wrap("A".getBytes(StandardCharsets.UTF_8)); - MessagePart mp1 = - new MessagePart(true, 0, Constants.OPCODE_TEXT, bb1, null, null, false, Long.MAX_VALUE); + MessagePart mp1 = new MessagePart(true, 0, Constants.OPCODE_TEXT, bb1, null, null, false, Long.MAX_VALUE); List<MessagePart> uncompressedParts1 = new ArrayList<>(); uncompressedParts1.add(mp1); perMessageDeflate.sendMessagePart(uncompressedParts1); ByteBuffer bb2 = ByteBuffer.wrap("".getBytes(StandardCharsets.UTF_8)); - MessagePart mp2 = - new MessagePart(true, 0, Constants.OPCODE_TEXT, bb2, null, null, false, Long.MAX_VALUE); + MessagePart mp2 = new MessagePart(true, 0, Constants.OPCODE_TEXT, bb2, null, null, false, Long.MAX_VALUE); List<MessagePart> uncompressedParts2 = new ArrayList<>(); uncompressedParts2.add(mp2); @@ -83,8 +83,7 @@ public class TestPerMessageDeflate { ByteBuffer bb = ByteBuffer.wrap(data); long writeTimeoutExpiry = System.currentTimeMillis() + 10000; - MessagePart mp = - new MessagePart(true, 0, Constants.OPCODE_BINARY, bb, null, null, true, writeTimeoutExpiry); + MessagePart mp = new MessagePart(true, 0, Constants.OPCODE_BINARY, bb, null, null, true, writeTimeoutExpiry); List<MessagePart> uncompressedParts = new ArrayList<>(); uncompressedParts.add(mp); @@ -125,8 +124,8 @@ public class TestPerMessageDeflate { byte[] data = new byte[size]; Arrays.fill(data, (byte) 0x80); List<MessagePart> uncompressedParts = new ArrayList<>(); - uncompressedParts.add(new MessagePart(true, 0, Constants.OPCODE_BINARY, - ByteBuffer.wrap(data), null, null, false, Long.MAX_VALUE)); + uncompressedParts.add(new MessagePart(true, 0, Constants.OPCODE_BINARY, ByteBuffer.wrap(data), null, null, + false, Long.MAX_VALUE)); MessagePart compressedPart = perMessageDeflateTx.sendMessagePart(uncompressedParts).get(0); // Decompress the way WsFrameBase.processDataBinary does: fill an 8192 @@ -172,18 +171,17 @@ public class TestPerMessageDeflate { // First message part byte[] data = new byte[1024]; ByteBuffer bb = ByteBuffer.wrap(data); - MessagePart mp1 = - new MessagePart(true, 0, Constants.OPCODE_BINARY, bb, null, null, false, Long.MAX_VALUE); + MessagePart mp1 = new MessagePart(true, 0, Constants.OPCODE_BINARY, bb, null, null, false, Long.MAX_VALUE); uncompressedParts.add(mp1); // Flush message (replicates result of calling flushBatch() - MessagePart mp2 = new MessagePart(true, 0, Constants.INTERNAL_OPCODE_FLUSH, null, null, null, false, - Long.MAX_VALUE); + MessagePart mp2 = + new MessagePart(true, 0, Constants.INTERNAL_OPCODE_FLUSH, null, null, null, false, Long.MAX_VALUE); uncompressedParts.add(mp2); List<MessagePart> compressedParts = perMessageDeflateTx.sendMessagePart(uncompressedParts); - Assert.assertEquals(2, compressedParts.size()); + Assert.assertEquals(2, compressedParts.size()); // Check the first compressed part MessagePart compressedPart1 = compressedParts.get(0); @@ -205,6 +203,181 @@ public class TestPerMessageDeflate { } + /* + * RFC 7692 section 7.2.1 explicitly permits an endpoint to compress a single message using multiple DEFLATE blocks, + * of any type, with any mix of BFINAL values - including multiple, independently terminated (BFINAL=1) DEFLATE + * blocks/streams concatenated together. This is not something PerMessageDeflate's own compressor produces (it + * always uses a single, sync-flush terminated stream), so the payload has to be constructed by hand to reproduce + * it. A compliant receiver must still recover the full, correctly ordered, decompressed content of the message. + */ + @Test + public void testMultipleFinalDeflateBlocksInOneMessage() throws IOException { + + byte[] part1 = "Hello, this is the FIRST part of the message.".getBytes(StandardCharsets.UTF_8); + byte[] part2 = "And this is the SECOND, independently terminated part.".getBytes(StandardCharsets.UTF_8); + + // Two separate raw DEFLATE streams, each terminated with BFINAL=1, concatenated together. + byte[] compressedPayload = concat(rawDeflateFinished(part1), rawDeflateFinished(part2)); + + List<Parameter> parameters = Collections.emptyList(); + List<List<Parameter>> preferences = new ArrayList<>(); + preferences.add(parameters); + + PerMessageDeflate perMessageDeflateRx = PerMessageDeflate.build(preferences, true); + perMessageDeflateRx.setNext(new TesterTransformation(ByteBuffer.wrap(compressedPayload))); + + // RSV1 (the permessage-deflate compression bit) set, RSV2/RSV3 clear. + int rsv = 0b100; + + ByteArrayOutputStream received = new ByteArrayOutputStream(); + ByteBuffer buf = ByteBuffer.allocate(8192); + TransformationResult tr; + do { + buf.clear(); + tr = perMessageDeflateRx.getMoreData(Constants.OPCODE_BINARY, true, rsv, buf); + received.write(buf.array(), 0, buf.position()); + } while (tr == TransformationResult.OVERFLOW); + + Assert.assertEquals(TransformationResult.END_OF_FRAME, tr); + + byte[] expected = concat(part1, part2); + + Assert.assertArrayEquals("Expected the concatenation of both DEFLATE blocks' decompressed content, got: " + + received.size() + " bytes: [" + received.toString("UTF-8") + "]", expected, received.toByteArray()); + } + + + /* + * As testMultipleFinalDeflateBlocksInOneMessage, but with three independently terminated (BFINAL=1) DEFLATE blocks + * concatenated together. This requires two recoveries in getMoreData() back to back, with no intervening + * readBuffer.clear()-based fetch between them - which is what exposes an offset bug that a single-recovery + * (two-block) message cannot: computing the second recovery's offset from readBuffer.arrayOffset() (fixed) rather + * than from where the first recovery's input segment actually started. + */ + @Test + public void testThreeFinalDeflateBlocksInOneMessage() throws IOException { + + byte[] part1 = "First part.".getBytes(StandardCharsets.UTF_8); + byte[] part2 = "Second, somewhat longer part.".getBytes(StandardCharsets.UTF_8); + byte[] part3 = "Third and final part.".getBytes(StandardCharsets.UTF_8); + + byte[] compressedPayload = + concat(concat(rawDeflateFinished(part1), rawDeflateFinished(part2)), rawDeflateFinished(part3)); + + List<Parameter> parameters = Collections.emptyList(); + List<List<Parameter>> preferences = new ArrayList<>(); + preferences.add(parameters); + + PerMessageDeflate perMessageDeflateRx = PerMessageDeflate.build(preferences, true); + perMessageDeflateRx.setNext(new TesterTransformation(ByteBuffer.wrap(compressedPayload))); + + int rsv = 0b100; + + ByteArrayOutputStream received = new ByteArrayOutputStream(); + ByteBuffer buf = ByteBuffer.allocate(8192); + TransformationResult tr; + do { + buf.clear(); + tr = perMessageDeflateRx.getMoreData(Constants.OPCODE_BINARY, true, rsv, buf); + received.write(buf.array(), 0, buf.position()); + } while (tr == TransformationResult.OVERFLOW); + + Assert.assertEquals(TransformationResult.END_OF_FRAME, tr); + + byte[] expected = concat(concat(part1, part2), part3); + + Assert.assertArrayEquals( + "Expected the concatenation of all three DEFLATE blocks' decompressed content, " + "got: " + + received.size() + " bytes: [" + received.toString("UTF-8") + "]", + expected, received.toByteArray()); + } + + + /* + * A message whose real content is a single, exactly-fitting, independently BFINAL=1 terminated block (nothing + * else) finishes cleanly via the normal needsInput()==true path and, as part of that, has the RFC 7692 section + * 7.2.2 EOM_BYTES fed to an already-finished Inflater - which silently ignores them, leaving finished()==true + * with those 4 bytes stuck in getRemaining(). With context takeover enabled, endFrame() must not leave the + * Inflater in that state: otherwise the next message's recovery logic computes its first offset from this + * stale, unrelated leftover count. + */ + @Test + public void testMessageEndingInCleanBfinalBlockDoesNotPoisonNextMessage() throws IOException { + byte[] message1 = "First message, a single clean BFINAL block.".getBytes(StandardCharsets.UTF_8); + byte[] compressed1 = rawDeflateFinished(message1); + + List<Parameter> parameters = Collections.emptyList(); + List<List<Parameter>> preferences = new ArrayList<>(); + preferences.add(parameters); + + /* + * Context takeover is enabled by default (no *_no_context_takeover parameter) - the same PerMessageDeflate + * instance, and therefore the same Inflater, must be reused across messages, exactly as it would be for a + * real connection. setNext() on PerMessageDeflate delegates to the existing next's setNext() once next is + * already set, so a single mutable source (rather than two separate TesterTransformation instances) is used + * to supply both messages' bytes in turn. + */ + PerMessageDeflate perMessageDeflateRx = PerMessageDeflate.build(preferences, true); + MutableTesterTransformation source = new MutableTesterTransformation(ByteBuffer.wrap(compressed1)); + perMessageDeflateRx.setNext(source); + int rsv = 0b100; + + ByteArrayOutputStream received1 = new ByteArrayOutputStream(); + ByteBuffer buf = ByteBuffer.allocate(8192); + TransformationResult tr; + do { + buf.clear(); + tr = perMessageDeflateRx.getMoreData(Constants.OPCODE_BINARY, true, rsv, buf); + received1.write(buf.array(), 0, buf.position()); + } while (tr == TransformationResult.OVERFLOW); + Assert.assertEquals(TransformationResult.END_OF_FRAME, tr); + Assert.assertArrayEquals(message1, received1.toByteArray()); + + // A completely separate, ordinary message on the same connection/Transformation instance. + byte[] message2 = "Second message on the same connection.".getBytes(StandardCharsets.UTF_8); + byte[] compressed2 = rawDeflateFinished(message2); + source.data = ByteBuffer.wrap(compressed2); + source.delivered = false; + + ByteArrayOutputStream received2 = new ByteArrayOutputStream(); + do { + buf.clear(); + tr = perMessageDeflateRx.getMoreData(Constants.OPCODE_BINARY, true, rsv, buf); + received2.write(buf.array(), 0, buf.position()); + } while (tr == TransformationResult.OVERFLOW); + Assert.assertEquals(TransformationResult.END_OF_FRAME, tr); + Assert.assertArrayEquals("Second message must decompress correctly; the first message's clean BFINAL " + + "ending must not poison the shared Inflater's state", message2, received2.toByteArray()); + } + + + private static byte[] rawDeflateFinished(byte[] data) { + @SuppressWarnings("resource") // False positive + Deflater deflater = new Deflater(Deflater.DEFAULT_COMPRESSION, true); + try { + deflater.setInput(data); + deflater.finish(); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + byte[] buf = new byte[4096]; + while (!deflater.finished()) { + int n = deflater.deflate(buf); + baos.write(buf, 0, n); + } + return baos.toByteArray(); + } finally { + deflater.end(); + } + } + + + private static byte[] concat(byte[] a, byte[] b) { + byte[] result = new byte[a.length + b.length]; + System.arraycopy(a, 0, result, 0, a.length); + System.arraycopy(b, 0, result, a.length, b.length); + return result; + } + + /* * RFC 7692 section 5.1 requires a permessage-deflate offer that contains an invalid extension parameter to be * declined so the handshake continues without compression. The offer must not fail the handshake. @@ -281,4 +454,58 @@ public class TestPerMessageDeflate { public void close() { } } + + + /* + * Like TesterTransformation, but the source ByteBuffer can be swapped out between messages, to exercise reuse of + * a single PerMessageDeflate instance (and therefore its Inflater) across multiple messages, as happens on a + * real connection with context takeover enabled. + */ + private static class MutableTesterTransformation implements Transformation { + + ByteBuffer data; + boolean delivered; + + MutableTesterTransformation(ByteBuffer data) { + this.data = data; + } + + @Override + public boolean validateRsvBits(int i) { + return false; + } + + @Override + public boolean validateRsv(int rsv, byte opCode) { + return false; + } + + @Override + public void setNext(Transformation t) { + } + + @Override + public List<MessagePart> sendMessagePart(List<MessagePart> messageParts) { + return messageParts; + } + + @Override + public TransformationResult getMoreData(byte opCode, boolean fin, int rsv, ByteBuffer dest) { + if (delivered) { + return TransformationResult.END_OF_FRAME; + } + dest.put(data); + delivered = true; + return TransformationResult.END_OF_FRAME; + } + + @Override + public Extension getExtensionResponse() { + return null; + } + + @Override + public void close() { + } + } } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 99b5522233..d08d579cd7 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -548,6 +548,11 @@ <code>{</code> and <code>}</code> characters from being mapped to WebSocket end points. (markt) </fix> + <fix> + Fix handling of WebSocket messages with compressed payloads using + per-message-deflate that have one or more non-final blocks where the + <code>BFINAL</code> bit is set. (markt) + </fix> </changelog> </subsection> <subsection name="Web applications"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
