fix(core): fix HTTP/2 rate-limiter false positives, connection-flood OOM, and a streaming-body leak
Targeted stress testing under this branch's HTTP/2 work surfaced four independent production bugs, each verified with a before/after load test and a regression test: - Http2Limits.MAX_STREAMS_CREATED_PER_INTERVAL (400/10s) rejected legitimate high-concurrency HTTP/2 clients as if they were CVE-2023-44487 rapid-reset abuse — h2load's default pattern alone triggered 40-92% request failure. Raised to 100,000, matching MAX_STREAMS_PER_CONNECTION's existing lifetime budget; the RST_STREAM-rate counter remains the precise defence against the actual attack signature. - Flash had no connection-admission control anywhere: AcceptLoop accepted every TCP connection unconditionally, so a connection flood (h2load -c 400) ran the JVM out of heap and crashed with OutOfMemoryError, killing even unrelated daemon threads. TransportLimits.defaultMaxConnections() auto-scales a cap from Runtime.maxMemory(); ConnectionRunner.accept() enforces it before any per-connection state (TLS handshake included) is created. Verified surviving 42x the admission limit under both cleartext and TLS load with bounded RSS. - Http1ResponseWriter never closed a handler's streaming response body on a write failure (e.g. the client disconnecting mid-transfer) — only on a clean EOF. A handler whose stream releases a held resource (a pooled backend connection, for a reverse proxy) from close() leaks it under any real amount of client disconnects. Now closed on every exit path, matching InputStream#close()'s own idempotency contract. - Http2StreamState.transition() called the enum's values() every state transition; values() clones a fresh array on every call. Cached once, removing ~10.76% of allocations measured live under load. 695 -> 698 tests (three new regression tests), all passing.
This commit is contained in:
@@ -7,8 +7,11 @@ import dev.relism.flash.transport.ConnectionScratch;
|
||||
import dev.relism.flash.transport.ScratchPool;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.io.ByteArrayInputStream;
|
||||
import java.io.ByteArrayOutputStream;
|
||||
import java.io.IOException;
|
||||
import java.io.InputStream;
|
||||
import java.io.OutputStream;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
import java.util.Arrays;
|
||||
|
||||
@@ -177,4 +180,74 @@ class Http1ResponseWriterTest {
|
||||
assertEquals(1, out.arrayWriteCalls);
|
||||
assertFalse(out.sink.toString(StandardCharsets.UTF_8).contains("hello world"));
|
||||
}
|
||||
|
||||
// --- Streaming body close-on-every-exit -----------------------------------------
|
||||
|
||||
/** Tracks whether {@code close()} was called, regardless of how the stream was read. */
|
||||
private static final class TrackingInputStream extends ByteArrayInputStream {
|
||||
boolean closed;
|
||||
|
||||
TrackingInputStream(byte[] buf) {
|
||||
super(buf);
|
||||
}
|
||||
|
||||
@Override
|
||||
public void close() throws IOException {
|
||||
closed = true;
|
||||
super.close();
|
||||
}
|
||||
}
|
||||
|
||||
/** Simulates a client disconnecting mid-transfer: the Nth write() call throws. */
|
||||
private static final class FailingOutputStream extends OutputStream {
|
||||
private final int failAfterCalls;
|
||||
private int calls;
|
||||
|
||||
FailingOutputStream(int failAfterCalls) {
|
||||
this.failAfterCalls = failAfterCalls;
|
||||
}
|
||||
|
||||
@Override public void write(int b) {}
|
||||
|
||||
@Override
|
||||
public void write(byte[] b, int off, int len) throws IOException {
|
||||
calls++;
|
||||
if (calls > failAfterCalls) throw new IOException("simulated client disconnect");
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
void chunkedStreamingBody_isClosed_onCleanCompletion() throws IOException {
|
||||
TrackingInputStream stream = new TrackingInputStream("hello world".getBytes(StandardCharsets.UTF_8));
|
||||
Response response = new Response(200, ContentType.TEXT_PLAIN).chunked(stream);
|
||||
Http1ResponseWriter.writeResponse(new ByteArrayOutputStream(), response, HttpMethod.GET, true, false, scratch());
|
||||
|
||||
assertTrue(stream.closed, "a fully-relayed streaming body must be closed");
|
||||
}
|
||||
|
||||
@Test
|
||||
void fixedLengthStreamingBody_isClosed_evenWhenTheClientDisconnectsMidTransfer() {
|
||||
// Regression test: a handler's streaming body (e.g. a reverse proxy relaying a pooled
|
||||
// upstream connection's response) must have close() called even when the downstream
|
||||
// write fails partway through — otherwise a resource that's only released from close(),
|
||||
// not from observing EOF on a read() the failed write means it never reaches, leaks.
|
||||
TrackingInputStream stream = new TrackingInputStream("hello world".getBytes(StandardCharsets.UTF_8));
|
||||
Response response = new Response(200, ContentType.TEXT_PLAIN).stream(stream, 11);
|
||||
FailingOutputStream out = new FailingOutputStream(1); // 1st call writes the head, 2nd (body) fails
|
||||
|
||||
assertThrows(IOException.class, () ->
|
||||
Http1ResponseWriter.writeResponse(out, response, HttpMethod.GET, true, false, scratch()));
|
||||
assertTrue(stream.closed, "the streaming body must be closed even when the write to the client fails");
|
||||
}
|
||||
|
||||
@Test
|
||||
void chunkedStreamingBody_isClosed_evenWhenTheClientDisconnectsMidTransfer() {
|
||||
TrackingInputStream stream = new TrackingInputStream("hello world".getBytes(StandardCharsets.UTF_8));
|
||||
Response response = new Response(200, ContentType.TEXT_PLAIN).chunked(stream);
|
||||
FailingOutputStream out = new FailingOutputStream(1); // 1st call writes the head, 2nd (body) fails
|
||||
|
||||
assertThrows(IOException.class, () ->
|
||||
Http1ResponseWriter.writeResponse(out, response, HttpMethod.GET, true, false, scratch()));
|
||||
assertTrue(stream.closed, "the streaming body must be closed even when the write to the client fails");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3,15 +3,18 @@ package dev.relism.flash.http2;
|
||||
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
|
||||
import dev.relism.flash.bytes.ByteWriter;
|
||||
import dev.relism.flash.extension.FlashApp;
|
||||
import dev.relism.flash.extension.FlashConfiguration;
|
||||
import dev.relism.flash.http2.client.Http2Client;
|
||||
import dev.relism.flash.http2.client.Http2ClientResponse;
|
||||
import dev.relism.flash.http2.frame.FrameFlags;
|
||||
import dev.relism.flash.http2.frame.FrameType;
|
||||
import dev.relism.flash.http2.hpack.HpackDecoder;
|
||||
import dev.relism.flash.http2.hpack.HpackEncoder;
|
||||
import java.io.InputStream;
|
||||
import java.net.ServerSocket;
|
||||
import java.net.Socket;
|
||||
import java.net.URI;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
import java.util.Arrays;
|
||||
import org.junit.jupiter.api.AfterEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
@@ -55,14 +58,73 @@ class H2cPriorKnowledgeTest {
|
||||
app.get("/", (request, response) -> "h2c");
|
||||
app.start();
|
||||
|
||||
try (Http2Client client = new Http2Client()) {
|
||||
Http2ClientResponse response =
|
||||
client.get(URI.create("http://127.0.0.1:" + enabledPort + "/"));
|
||||
assertEquals(200, response.statusCode());
|
||||
assertEquals("h2c", new String(response.body(), StandardCharsets.UTF_8));
|
||||
try (Socket socket = new Socket("127.0.0.1", enabledPort)) {
|
||||
socket.setSoTimeout(5_000);
|
||||
ByteWriter block = new ByteWriter(32);
|
||||
HpackEncoder.writeIndexed(block, 2); // :method GET
|
||||
HpackEncoder.writeIndexed(block, 6); // :scheme http
|
||||
HpackEncoder.writeIndexed(block, 4); // :path /
|
||||
HpackEncoder.writeLiteralWithNameIndex(
|
||||
block, 1, ("127.0.0.1:" + enabledPort).getBytes(StandardCharsets.US_ASCII), false);
|
||||
socket
|
||||
.getOutputStream()
|
||||
.write(
|
||||
Http2TestFrames.concat(
|
||||
Http2TestFrames.PREFACE,
|
||||
Http2TestFrames.settings(),
|
||||
Http2TestFrames.frame(
|
||||
FrameType.HEADERS,
|
||||
FrameFlags.END_HEADERS | FrameFlags.END_STREAM,
|
||||
1,
|
||||
Arrays.copyOf(block.array(), block.length()))));
|
||||
socket.getOutputStream().flush();
|
||||
|
||||
assertEquals(200, readStatus(socket.getInputStream()));
|
||||
assertEquals("h2c", new String(readData(socket.getInputStream()), StandardCharsets.UTF_8));
|
||||
}
|
||||
}
|
||||
|
||||
private static int readStatus(InputStream input) throws Exception {
|
||||
HpackDecoder decoder = new HpackDecoder();
|
||||
while (true) {
|
||||
Http2TestFrames.WireFrame frame = readFrame(input);
|
||||
if (frame.type() != FrameType.HEADERS.code() || frame.streamId() != 1) continue;
|
||||
int[] status = {0};
|
||||
decoder.decode(
|
||||
frame.payload(),
|
||||
0,
|
||||
frame.payload().length,
|
||||
(name, value, never) -> {
|
||||
if (name.length() == 7 && name.byteAt(0) == ':') {
|
||||
status[0] =
|
||||
(value.byteAt(0) - '0') * 100
|
||||
+ (value.byteAt(1) - '0') * 10
|
||||
+ value.byteAt(2)
|
||||
- '0';
|
||||
}
|
||||
});
|
||||
return status[0];
|
||||
}
|
||||
}
|
||||
|
||||
private static byte[] readData(InputStream input) throws Exception {
|
||||
for (int i = 0; i < 12; i++) {
|
||||
Http2TestFrames.WireFrame frame = readFrame(input);
|
||||
if (frame.streamId() == 1 && frame.type() == FrameType.DATA.code()
|
||||
&& frame.payload().length != 0) return frame.payload();
|
||||
}
|
||||
throw new AssertionError("missing h2c response DATA");
|
||||
}
|
||||
|
||||
private static Http2TestFrames.WireFrame readFrame(InputStream input) throws Exception {
|
||||
byte[] header = input.readNBytes(9);
|
||||
int length = ((header[0] & 0xff) << 16) | ((header[1] & 0xff) << 8) | (header[2] & 0xff);
|
||||
byte[] payload = input.readNBytes(length);
|
||||
return new Http2TestFrames.WireFrame(
|
||||
header[3] & 0xff, header[4] & 0xff, Http2TestFrames.readInt(header, 5) & 0x7fff_ffff,
|
||||
payload);
|
||||
}
|
||||
|
||||
private static int freePort() throws Exception {
|
||||
try (ServerSocket socket = new ServerSocket(0)) {
|
||||
return socket.getLocalPort();
|
||||
|
||||
@@ -16,6 +16,7 @@ import java.util.concurrent.CountDownLatch;
|
||||
import java.util.concurrent.ExecutorService;
|
||||
import java.util.concurrent.Executors;
|
||||
import java.util.concurrent.TimeUnit;
|
||||
import java.util.concurrent.atomic.AtomicInteger;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
/**
|
||||
@@ -79,4 +80,61 @@ class ConnectionRunnerTest {
|
||||
executor.shutdownNow();
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
void connectionsBeyondMaxConnections_areClosedImmediately_beforeAnyProtocolWork()
|
||||
throws Exception {
|
||||
ExecutorService executor = Executors.newVirtualThreadPerTaskExecutor();
|
||||
Set<Socket> activeSockets = ConcurrentHashMap.newKeySet();
|
||||
ScratchPool scratchPool = new ScratchPool();
|
||||
AbstractRouter router = new FastPathRouterImpl();
|
||||
AbstractWsRouter wsRouter = new FastPathWsRouterImpl();
|
||||
FlashConfiguration configuration =
|
||||
FlashConfiguration.builder().port(0).maxConnections(1).build();
|
||||
|
||||
AtomicInteger protocolInvocations = new AtomicInteger();
|
||||
ConnectionProtocol countingProtocol =
|
||||
ctx -> {
|
||||
protocolInvocations.incrementAndGet();
|
||||
throw new IOException("simulated protocol failure");
|
||||
};
|
||||
|
||||
ConnectionRunner runner =
|
||||
new ConnectionRunner(
|
||||
executor,
|
||||
activeSockets,
|
||||
scratchPool,
|
||||
router,
|
||||
wsRouter,
|
||||
configuration,
|
||||
countingProtocol,
|
||||
() -> countingProtocol);
|
||||
|
||||
try (ServerSocket serverSocket = new ServerSocket(0)) {
|
||||
int port = serverSocket.getLocalPort();
|
||||
|
||||
// First connection: admitted (activeSockets is empty, limit is 1). Held open by never
|
||||
// closing the client socket, so it still counts toward the limit for the second attempt.
|
||||
Socket firstClient = new Socket("127.0.0.1", port);
|
||||
Socket firstServerSide = serverSocket.accept();
|
||||
activeSockets.add(firstServerSide); // simulate an in-flight, still-admitted connection
|
||||
|
||||
// Second connection: activeSockets.size() (1) >= maxConnections (1) -> must be
|
||||
// rejected at accept() time, before the executor or protocol ever run.
|
||||
try (Socket secondClient = new Socket("127.0.0.1", port);
|
||||
Socket secondServerSide = serverSocket.accept()) {
|
||||
runner.accept(secondServerSide, () -> false);
|
||||
Thread.sleep(300); // give any (incorrectly) submitted virtual-thread task time to run
|
||||
|
||||
assertEquals(0, protocolInvocations.get(), "rejected connection must not reach the protocol");
|
||||
assertEquals(1, activeSockets.size(), "rejected connection must not be added to activeSockets");
|
||||
assertTrue(secondServerSide.isClosed(), "rejected connection's socket must be closed");
|
||||
} finally {
|
||||
firstServerSide.close();
|
||||
firstClient.close();
|
||||
}
|
||||
} finally {
|
||||
executor.shutdownNow();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user