fix: bound the size a compressed payload may expand to - #3502
Open
pjfanning wants to merge 2 commits into
Open
Conversation
Motivation: Five serializers gzip their payload and decompress it on the way back in, each with the same unbounded loop: read the whole GZIPInputStream into a ByteArrayOutputStream. gzip expands by up to about three orders of magnitude, so neither the size of the compressed bytes nor the transport's frame limit bounds the buffer the decompressed bytes are read into. Modification: Add Decompression (@internalapi) with a gunzip that stops once the decompressed size passes pekko.serialization.max-decompressed-size (default 256 MiB) and reports it as a NotSerializableException, and route all twelve call sites through it. The Jackson serializers already bound decompression and keep their own pekko.serialization.jackson.compression.max-decompressed-size. Result: An over-expanding payload is rejected as an ordinary serialization failure. No behaviour change for payloads within the limit.
Motivation: A bounded default could reject a payload an existing cluster legitimately exchanges, so a patch release carrying a 256 MiB default could break running clusters on upgrade. The bound should be opt-in. Modification: Default pekko.serialization.max-decompressed-size to -1, meaning no limit and matching the behaviour of earlier releases. A negative maximum skips the size check in gunzip. Config's getBytes refuses negative numbers, so the setting is read as a plain long first and as a memory size only when that is not a negative number. Result: Decompression is unbounded by default; configuring a size such as 256 MiB bounds it. Tests: - sbt "actor-tests/testOnly org.apache.pekko.serialization.DecompressionSpec" - 8 passed - sbt "cluster/testOnly org.apache.pekko.cluster.protobuf.ClusterMessageSerializerDecompressionSpec" - 4 passed - sbt "distributed-data/testOnly org.apache.pekko.cluster.ddata.protobuf.SerializationSupportDecompressionSpec" - 3 passed - sbt "actor/scalafmtCheckAll" "actor-tests/scalafmtCheckAll" - clean References: Refs apache#3502
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Five serializers gzip their payload and decompress it on the way back in, each with the
same unbounded loop: read the whole
GZIPInputStreaminto aByteArrayOutputStreamandreturn
toByteArray. gzip expands by up to about three orders of magnitude, so neitherthe size of the compressed bytes nor the transport's frame limit bounds the buffer the
decompressed bytes are read into — a payload well inside
maximum-frame-sizecan expandto hundreds of megabytes.
Two of the twelve call sites are a little further from where a failure would normally be
contained.
ClusterMessageSerializerdefersGossipEnvelope's decompression into athunk, so it runs when the gossip is read rather than on a deserialization thread; and
Welcomeis deserialized while the node is stilluninitialized.The Jackson serializers already bound this —
JacksonSerializer.gunzip, added in #3491 —but nothing else does.
Modification
Add
org.apache.pekko.serialization.Decompression(@InternalApi), whosegunzipstops as soon as the decompressed size passes
pekko.serialization.max-decompressed-sizeand reports it as a
NotSerializableException. Route all twelve call sites through it:ClusterMessageSerializer.decompressSerializationSupport.decompressDistributedPubSubMessageSerializer.decompressClusterShardingMessageSerializer.decompressMessageSerializer.decompressThe setting defaults to
-1, meaning no limit, so the bound is opt-in: a patch releasemust not start rejecting payloads an existing cluster legitimately exchanges, and there
is no default that is provably above every deployment's largest ddata or gossip payload.
Operators who want the protection set a size such as
256 MiB, larger than anythingtheir cluster legitimately sends. A negative maximum skips the size check in
gunzip;config's
getBytesrefuses negative numbers, so the setting is read as a plain longfirst and as a memory size only when that is not a negative number.
The four class-based serializers read the maximum once into a
val.SerializationSupportis a public trait whose implementors takesystemas aconstructor parameter, so it reads the maximum per call instead — a field there would
add abstract accessors to the trait and break binary compatibility, and the lookup is
negligible next to the decompression it guards.
The Jackson serializers are unchanged: they are already bounded and keep their own
pekko.serialization.jackson.compression.max-decompressed-size.decompresssignatures are unchanged, so there is no binary-compatibility impact.Result
By default, behaviour is unchanged. With a maximum configured, a payload that expands
beyond it is rejected as an ordinary serialization failure naming the setting.
Tests
sbt "actor-tests/testOnly org.apache.pekko.serialization.DecompressionSpec"— 8 passed (round trip, the limit boundary, one byte over, the setting named in the message, a highly compressible payload rejected without inflating, no limit applied for a negative maximum, the unlimited default, and a configured size read correctly)sbt "cluster/testOnly org.apache.pekko.cluster.protobuf.*"— 13 passed, including a newClusterMessageSerializerDecompressionSpeccovering both the eagerWelcomepath and the deferredGossipEnvelopepathsbt "distributed-data/testOnly org.apache.pekko.cluster.ddata.protobuf.*"— 30 passed, including a newSerializationSupportDecompressionSpeccovering the per-call lookup in the traitsbt "cluster-tools/testOnly org.apache.pekko.cluster.pubsub.protobuf.*"— 1 passedsbt "cluster-sharding/testOnly org.apache.pekko.cluster.sharding.protobuf.*"— 14 passedsbt "cluster-metrics/testOnly org.apache.pekko.cluster.metrics.protobuf.*"— 9 passedsbt "actor/mimaReportBinaryIssues" "cluster/mimaReportBinaryIssues" "cluster-metrics/mimaReportBinaryIssues" "cluster-sharding/mimaReportBinaryIssues" "cluster-tools/mimaReportBinaryIssues" "distributed-data/mimaReportBinaryIssues"— no issuessbt scalafmtAll headerCreateAll— no changesReferences
Extends #3491, which bounded decompression in the Jackson serializer only.
Decompression.scalaconsolidates thedecompressbodies of the five Akka-derivedserializers listed above together with
JacksonSerializer.gunzip, so it carries thederived-from-Akka header and the Lightbend copyright. The three new specs are new code
and carry the standard ASF header.