Prasath Jayasundar created ZOOKEEPER-5097:
---------------------------------------------
Summary: Add configurable limits to snapshot deserialization
(shared by file load, AdminServer restore, and learner sync)
Key: ZOOKEEPER-5097
URL: https://issues.apache.org/jira/browse/ZOOKEEPER-5097
Project: ZooKeeper
Issue Type: Bug
Affects Versions: 3.9.6
Reporter: Prasath Jayasundar
*Summary:* Add configurable limits to snapshot deserialization to prevent
unbounded resource consumption
*Type:* Improvement
*Component:* server
*Affects:* 3.9.6 and master
----
Follow-up to a snapshot deserialization robustness discussion on
[email protected]. This was assessed as *not a vulnerability* under
ZooKeeper's security model and is tracked here as hardening work.
h2. Background
The snapshot deserializers allocate based on values read from the input stream,
with no aggregate byte, node, or entry budget. {{jute.maxbuffer}} bounds
individual records but provides no cumulative limit, so a snapshot whose
contents exceed the configured heap causes an {{OutOfMemoryError}} during load.
This affects legitimate oversized snapshots as much as malformed ones — the
parser cannot distinguish them without an operator-configured limit.
Three loops are unbounded:
* {{ReferenceCountedACLCache.deserialize}} — declared ACL map count,
accumulated into a {{{}LinkedHashMap{}}}, no validation of the count
* {{DataTree.deserialize}} — sentinel-terminated ({{{}while
(!"/".equals(path)){}}}), reads until the {{/}} sentinel or EOF
* {{SerializeUtils.deserializeSnapshot}} — declared session count, unvalidated
Relevant failure detail: {{FileSnap.deserialize}} iterates candidate snapshots
inside {{{}try \{ ... } catch (IOException e){}}}, so a malformed file is
logged and skipped, but {{OutOfMemoryError}} is an {{Error}} rather than an
{{IOException}} and propagates past that handler. With the JVM flags in the
distribution's {{zkServer.sh}} ({{{}-XX:OnOutOfMemoryError='kill -9 %p'{}}})
the process is killed.
h2. Proposed work
# A configurable size limit on the AdminServer restore request body.
# Optional limits on entries during deserialization, failing with an
{{IOException}} so that {{FileSnap.deserialize}} falls through to the next
candidate snapshot rather than exhausting the heap.
h2. Where the limits belong
During scoping, a third call site was identified. {{Learner.syncWithLeader()}}
deserializes the SNAP payload received from the leader through the same
{{{}ZKDatabase{}}}/{{{}SerializeUtils{}}} path used by file load and
AdminServer restore ({{{}Learner.java:601{}}}), with the sanity-check marker
read only after deserialization completes ({{{}Learner.java:609-612{}}}).
There are therefore three entry points into the same deserializers:
||Entry point||Path||
|Snapshot file load|{{FileSnap.deserialize}}|
|AdminServer restore|{{ZooKeeperServer.restoreFromSnapshot}}|
|Learner sync|{{Learner.syncWithLeader}}|
Any limit should be implemented in the shared deserialization path rather than
in {{SnapStream}} or on the restore request body, so that a single change
covers all three. A bound placed in {{SnapStream}} would miss both restore and
learner sync; a bound on the restore body would miss the other two.
h2. Design consideration: the fallback asymmetry
The rationale for failing with an {{IOException}} is that
{{FileSnap.deserialize}} then skips to the next candidate snapshot. *That
fallback does not exist on the learner sync path.* A learner that refuses the
leader's snapshot will reconnect, be offered the same snapshot again, and be
unable to rejoin the ensemble.
So the same exception produces graceful degradation on file load and an
unjoinable learner on sync. Consequently:
* Any new limit must default to {*}disabled, or to a generous value{*}.
* When exceeded, it should fail with a clear error message {*}naming the
configuration property involved{*}, so that an ensemble whose legitimate data
has outgrown the limit fails in an obvious, fixable way rather than looping
with an opaque failure.
----
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
