This is an automated email from the ASF dual-hosted git repository. anmolnar pushed a commit to branch branch-3.9 in repository https://gitbox.apache.org/repos/asf/zookeeper.git
commit 1f169e149f78fe8fcfa31b7db0cf9b625cdb978d Author: Andor Molnar <[email protected]> AuthorDate: Wed Sep 30 10:40:32 2026 -0500 Add multiRead response throttling to guard resource allocation --- .../src/main/resources/markdown/zookeeperAdmin.md | 24 +++++ .../zookeeper/server/FinalRequestProcessor.java | 49 ++++++++- .../apache/zookeeper/server/ZooKeeperServer.java | 45 ++++++++ .../apache/zookeeper/test/MultiOperationTest.java | 115 +++++++++++++++++++++ 4 files changed, 231 insertions(+), 2 deletions(-) diff --git a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md index e122bde3d..c53bc0a4d 100644 --- a/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md +++ b/zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md @@ -1156,6 +1156,30 @@ property, when available, is noted below. **New in 3.6.0:** The size threshold after which a request is considered a large request. If it is -1, then all requests are considered small, effectively turning off large request throttling. The default is -1. +* *multiRead.maxOps* : + (Java system property: **zookeeper.multiRead.maxOps**) + **New in 3.9.7:** + The maximum number of read operations (getData / getChildren) permitted in a single + multiRead request. A multiRead can amplify a request that is small on the wire into a + very large in-memory response, because every sub-operation result is materialized and + held in memory before the response is serialized. This limit, together with + *multiRead.maxResponseBytes*, bounds that amplification. A multiRead exceeding this + count is rejected with a BadArguments (Code = BADARGUMENTS) error and is not executed. + Note that neither *jute.maxbuffer* nor *largeRequestMaxBytes* protects against this, + as both bound only the inbound request, not the response it generates. Set to 0 (or a + negative value) to disable the check. The default is 1000. + +* *multiRead.maxResponseBytes* : + (Java system property: **zookeeper.multiRead.maxResponseBytes**) + **New in 3.9.7:** + The maximum cumulative size, in bytes, of the data materialized while serving a single + multiRead request. This is the primary guard against a multiRead response-amplification + denial of service. The server accumulates the size of each sub-operation result as the + request is processed and rejects the request with a BadArguments (Code = BADARGUMENTS) + error as soon as the running total exceeds this value, before the full response is built + in heap. Set to 0 (or a negative value) to disable the check. The default is 67108864 + (64 * 1024 * 1024, i.e. 64 MB). + * *outstandingHandshake.limit* (Java system property only: **zookeeper.netty.server.outstandingHandshake.limit**) The maximum in-flight TLS handshake connections could have in ZooKeeper, diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java index 911583f9f..cd360c231 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/FinalRequestProcessor.java @@ -270,20 +270,41 @@ public void processRequest(Request request) { case OpCode.multiRead: { lastOp = "MLTR"; MultiOperationRecord multiReadRecord = request.readRequestRecord(MultiOperationRecord::new); + + // Guard against multiRead response-amplification DoS. A multiRead request + // can be tiny on the wire (and thus pass jute.maxbuffer and the large-request + // throttle, which only bound the inbound request) yet force the server to + // materialize an unbounded response in heap: every sub-op result is copied and + // held in the MultiResponse before serialization. Cap the operation count and + // the cumulative response size so the response stays bounded. + final int maxOps = zks.getMultiReadMaxOps(); + if (maxOps > 0 && multiReadRecord.size() > maxOps) { + throw new KeeperException.BadArgumentsException( + "multiRead operation count " + multiReadRecord.size() + + " exceeds the configured maximum of " + maxOps); + } + final long maxResponseBytes = zks.getMultiReadMaxResponseBytes(); + long responseBytes = 0; + rsp = new MultiResponse(); OpResult subResult; for (Op readOp : multiReadRecord) { + long opBytes = 0; try { Record rec; switch (readOp.getType()) { case OpCode.getChildren: rec = handleGetChildrenRequest(readOp.toRequestRecord(), cnxn, request.authInfo); - subResult = new GetChildrenResult(((GetChildrenResponse) rec).getChildren()); + List<String> children = ((GetChildrenResponse) rec).getChildren(); + opBytes = childrenSizeInBytes(children); + subResult = new GetChildrenResult(children); break; case OpCode.getData: rec = handleGetDataRequest(readOp.toRequestRecord(), cnxn, request.authInfo); GetDataResponse gdr = (GetDataResponse) rec; - subResult = new GetDataResult(gdr.getData(), gdr.getStat()); + byte[] data = gdr.getData(); + opBytes = data == null ? 0 : data.length; + subResult = new GetDataResult(data, gdr.getStat()); break; default: throw new IOException("Invalid type of readOp"); @@ -291,6 +312,14 @@ public void processRequest(Request request) { } catch (KeeperException e) { subResult = new ErrorResult(e.code().intValue()); } + if (maxResponseBytes > 0) { + responseBytes += opBytes; + if (responseBytes > maxResponseBytes) { + throw new KeeperException.BadArgumentsException( + "multiRead response size exceeds the configured maximum of " + + maxResponseBytes + " bytes"); + } + } ((MultiResponse) rsp).add(subResult); } break; @@ -626,6 +655,22 @@ public void processRequest(Request request) { } } + /** + * Estimates the serialized size of a getChildren result so that multiRead can bound + * its cumulative response size. Each child name is counted as its length in bytes + * (chars are a safe lower bound) plus the 4-byte length prefix jute writes per string. + */ + private static long childrenSizeInBytes(List<String> children) { + if (children == null) { + return 0; + } + long bytes = 0; + for (String child : children) { + bytes += 4L + (child == null ? 0 : child.length()); + } + return bytes; + } + private Record handleGetChildrenRequest(Record request, ServerCnxn cnxn, List<Id> authInfo) throws KeeperException, IOException { GetChildrenRequest getChildrenRequest = (GetChildrenRequest) request; String path = getChildrenRequest.getPath(); diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java index f9a60da1f..7395509d8 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java @@ -327,6 +327,25 @@ protected enum State { private final AtomicInteger currentLargeRequestBytes = new AtomicInteger(0); + /** + * Maximum number of read operations allowed in a single multiRead request. + * A multiRead can amplify a tiny request into a very large in-memory response + * (each sub-op result is fully materialized and held in the MultiResponse before + * serialization), so this bounds the operation fan-out. A value of 0 or less + * disables the check. + */ + private volatile int multiReadMaxOps = 1000; + + /** + * Maximum cumulative size, in bytes, of the data materialized while serving a + * single multiRead request. This is the primary guard against a response- + * amplification denial of service: without it, a request that is small on the wire + * (and therefore not caught by jute.maxbuffer or the large-request throttle, both + * of which only bound the inbound request) can force the server to allocate + * arbitrarily large amounts of heap. A value of 0 or less disables the check. + */ + private volatile long multiReadMaxResponseBytes = 64L * 1024 * 1024; + private final AuthenticationHelper authHelper = new AuthenticationHelper(); void removeCnxn(ServerCnxn cnxn) { @@ -384,6 +403,8 @@ public ZooKeeperServer(FileTxnSnapLog txnLogFactory, int tickTime, int minSessio this.initLargeRequestThrottlingSettings(); + this.initMultiReadThrottlingSettings(); + LOG.info( "Created server with" + " tickTime {} ms" @@ -1609,6 +1630,30 @@ private void initLargeRequestThrottlingSettings() { setLargeRequestThreshold(Integer.getInteger("zookeeper.largeRequestThreshold", -1)); } + private void initMultiReadThrottlingSettings() { + multiReadMaxOps = Integer.getInteger("zookeeper.multiRead.maxOps", multiReadMaxOps); + multiReadMaxResponseBytes = Long.getLong("zookeeper.multiRead.maxResponseBytes", multiReadMaxResponseBytes); + LOG.info("multiRead limits: maxOps={}, maxResponseBytes={}", multiReadMaxOps, multiReadMaxResponseBytes); + } + + public int getMultiReadMaxOps() { + return multiReadMaxOps; + } + + public void setMultiReadMaxOps(int maxOps) { + this.multiReadMaxOps = maxOps; + LOG.info("multiRead maxOps set to {}", maxOps); + } + + public long getMultiReadMaxResponseBytes() { + return multiReadMaxResponseBytes; + } + + public void setMultiReadMaxResponseBytes(long maxBytes) { + this.multiReadMaxResponseBytes = maxBytes; + LOG.info("multiRead maxResponseBytes set to {}", maxBytes); + } + public int getLargeRequestMaxBytes() { return largeRequestMaxBytes; } diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java index fd7b8a02f..31c3f04db 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java @@ -25,6 +25,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; import java.util.ArrayList; @@ -60,7 +61,9 @@ import org.apache.zookeeper.data.Id; import org.apache.zookeeper.data.Stat; import org.apache.zookeeper.server.SyncRequestProcessor; +import org.apache.zookeeper.server.ZooKeeperServer; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -960,6 +963,118 @@ public void testMultiRead(boolean useAsync) throws Exception { assertEquals(0, stat.getNumChildren()); } + /** + * A multiRead whose operation count exceeds multiRead.maxOps must be rejected + * outright (BADARGUMENTS) rather than executed. This is the operation-count + * dimension of the multiRead response-amplification DoS guard. + */ + @Test + public void testMultiReadOpCountLimitRejected() throws Exception { + ZooKeeperServer zks = serverFactory.getZooKeeperServer(); + zks.setMultiReadMaxOps(5); + + zk.create("/rlimit", "data".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); + + List<Op> ops = new ArrayList<>(); + for (int i = 0; i < 6; i++) { + ops.add(Op.getData("/rlimit")); + } + assertThrows(KeeperException.BadArgumentsException.class, () -> multi(zk, ops, false)); + } + + /** + * A multiRead at exactly multiRead.maxOps operations is allowed. + */ + @Test + public void testMultiReadOpCountLimitAtBoundaryAllowed() throws Exception { + ZooKeeperServer zks = serverFactory.getZooKeeperServer(); + zks.setMultiReadMaxOps(5); + + zk.create("/rlimit", "data".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); + + List<Op> ops = new ArrayList<>(); + for (int i = 0; i < 5; i++) { + ops.add(Op.getData("/rlimit")); + } + List<OpResult> results = multi(zk, ops, false); + assertEquals(5, results.size()); + } + + /** + * Setting both limits to 0 disables the guards and restores the historical + * (unbounded) behaviour, confirming the checks are the only thing gating it. + */ + @Test + public void testMultiReadLimitsDisabled() throws Exception { + ZooKeeperServer zks = serverFactory.getZooKeeperServer(); + zks.setMultiReadMaxOps(0); + zks.setMultiReadMaxResponseBytes(0); + + zk.create("/rlimit", "data".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); + + List<Op> ops = new ArrayList<>(); + for (int i = 0; i < 2000; i++) { // well over the default cap of 1000 + ops.add(Op.getData("/rlimit")); + } + List<OpResult> results = multi(zk, ops, false); + assertEquals(2000, results.size()); + } + + /** + * A multiRead with a small operation count whose cumulative response size + * exceeds multiRead.maxResponseBytes must be rejected. This is the byte + * dimension of the guard and is the shape of the reported DoS (a few ops that + * each pull a large znode). + */ + @Test + public void testMultiReadResponseByteLimitRejected() throws Exception { + ZooKeeperServer zks = serverFactory.getZooKeeperServer(); + zks.setMultiReadMaxOps(0); // isolate the byte cap + zks.setMultiReadMaxResponseBytes(100_000); + + byte[] data = new byte[40_000]; + Arrays.fill(data, (byte) 'x'); + zk.create("/big", data, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); + + // 3 * 40_000 = 120_000 > 100_000 -> rejected while processing the third op + List<Op> ops = Arrays.asList(Op.getData("/big"), Op.getData("/big"), Op.getData("/big")); + assertThrows(KeeperException.BadArgumentsException.class, () -> multi(zk, ops, false)); + } + + /** + * A multiRead whose cumulative response size stays under + * multiRead.maxResponseBytes is served normally. + */ + @Test + public void testMultiReadResponseByteLimitUnderBudgetAllowed() throws Exception { + ZooKeeperServer zks = serverFactory.getZooKeeperServer(); + zks.setMultiReadMaxOps(0); + zks.setMultiReadMaxResponseBytes(100_000); + + byte[] data = new byte[40_000]; + Arrays.fill(data, (byte) 'x'); + zk.create("/big", data, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); + + // 2 * 40_000 = 80_000 < 100_000 -> allowed + List<Op> ops = Arrays.asList(Op.getData("/big"), Op.getData("/big")); + List<OpResult> results = multi(zk, ops, false); + assertEquals(2, results.size()); + assertArrayEquals(data, ((OpResult.GetDataResult) results.get(0)).getData()); + } + + /** + * A normal, small multiRead must not be affected by the new guards under + * their default limits (no false positives). + */ + @Test + public void testMultiReadWithinDefaultLimitsSucceeds() throws Exception { + zk.create("/n1", "d1".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); + zk.create("/n2", "d2".getBytes(), Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); + List<OpResult> results = multi(zk, Arrays.asList( + Op.getData("/n1"), Op.getChildren("/n1"), Op.getData("/n2")), false); + assertEquals(3, results.size()); + } + @ParameterizedTest @ValueSource(booleans = {true, false}) public void testMixedReadAndTransaction(boolean useAsync) throws Exception {
