serializer-utils: bound frame size in length-prefixed streaming serializers by bryce-anderson · Pull Request #3563 · apple/servicetalk

bryce-anderson · 2026-07-09T18:21:14Z

Motivation

FixedLengthStreamingSerializer and VarIntLengthStreamingSerializer read an
attacker-controlled length prefix and validated only that it was non-negative, so
a single frame could declare up to Integer.MAX_VALUE bytes and drive the process
toward heap exhaustion before any object is deserialized. The HTTP aggregation
limit does not cover this: it only bounds aggregated messages, not the streaming
deserialization path these serializers use.

Modifications

Add a maxMessageSize checked as soon as the length prefix is decoded, before any
readBytes/accumulation. Every frame passes through the decode, so this covers
both single-buffer and split delivery. Oversized frames throw
MaxMessageSizeExceededException (a new SerializationException subtype in
serializer-api); malformed-length frames keep throwing SerializationException.
Default the limit to 4 MiB, resolved once from the temporary
io.servicetalk.serializer.utils.temporaryDefaultMaxMessageSize system property.
Configure it per instance via a 3-arg serializer constructor or a new
ProtobufSerializerFactory(int) constructor: 0 disables the limit, -1 enables
warn-only mode (logs but does not reject, as a rollout aid), and other negative
values are rejected.
Map MaxMessageSizeExceededException to HTTP 413 Payload Too Large in
HttpExceptionMapperServiceFilter, checked before the generic
SerializationException -> 415 branch. The type is transport-neutral so gRPC can
later map it to RESOURCE_EXHAUSTED.

Result

Streaming length-prefixed frames larger than 4 MiB are rejected up front with a
413 rather than buffering toward an OOM. This is a behavior change for the HTTP
streaming string content types and protobuf-over-HTTP: callers that legitimately
stream larger frames must raise, disable, or warn-only the limit via the 3-arg
constructor, a configured ProtobufSerializerFactory, or the system property.

#### Motivation The length-prefixed deframer validated only that the attacker-controlled 4-byte length prefix was non-negative, so a single frame could declare up to Integer.MAX_VALUE bytes and drive the process toward heap exhaustion before any object is deserialized. #### Modifications - Add an optional maxMessageSize, checked immediately after readInt() before any readBytes/accumulation. Every frame passes through readInt(), so this covers both single-buffer and split delivery. Oversized frames are rejected with a SerializationException. - Add a 3-arg constructor carrying maxMessageSize; the existing 2-arg constructor delegates with 0 (unlimited), leaving existing callers unchanged. - Add tests for rejection above the limit (single-buffer and split delivery), acceptance at the limit, and maxMessageSize = 0 disabling the check. #### Result Callers can cap the maximum frame size and reject oversized frames up front. This is complementary to the HTTP aggregation limit, which does not apply to the streaming deserialization path.

idelpivnitskiy · 2026-07-09T18:34:35Z

     */
    public FixedLengthStreamingSerializer(final SerializerDeserializer<T> serializer,
                                          final ToIntFunction<T> bytesEstimator) {
+        this(serializer, bytesEstimator, 0);


Imho we should set a sane default around 4Mb for each object, like grpc-java does. WDYT?

This is tough because we have a lot of serializers in HttpSerializers that would be affected. We could fix them all now if you like by adding another constructor and deprecating the existing one, but should we follow the same default pattern?

Not a lot, there are just 2 for fixed-length and 2 for var-length which are effectively the same and used only for String. This is kind of rare use-case to use Strings with length-prefixed encoding. I'm not sure anyone actually uses those. As long as we have a system property (can we reuse io.servicetalk.http.netty.temporaryDefaultMaxAggregatedPayloadSize or do we need a new one?) and new API to configure those - we should be good, the blast radius should be small. The only place that needs more API is grpc (servicetalk-data-protobuf) that uses VarIntLengthStreamingSerializer, it will need some API at the builders to propagate that setting, similar to grpc-java

#### Motivation Like FixedLengthStreamingSerializer, the VarIntLengthStreamingSerializer deframer accepted an attacker-controlled length prefix up to Integer.MAX_VALUE with no upper bound, so a single frame could drive the process toward heap exhaustion before any object is deserialized. #### Modifications - Add an optional maxMessageSize, checked as soon as the VarInt length prefix is decoded and before any readBytes/accumulation. Every frame passes through the decode, so this covers both single-buffer and split delivery. Oversized frames are rejected with a SerializationException. - Add a 3-arg constructor carrying maxMessageSize; the existing 2-arg constructor delegates with 0 (unlimited), leaving existing callers unchanged. - Add tests for rejection above the limit (single-buffer and split delivery), acceptance at the limit, and maxMessageSize = 0 disabling the check. #### Result Callers of VarIntLengthStreamingSerializer can cap the maximum frame size and reject oversized frames up front, matching the guard added to FixedLengthStreamingSerializer.

…d serializers #### Motivation The maxMessageSize guard added to FixedLengthStreamingSerializer and VarIntLengthStreamingSerializer accepts a frame whose declared length is at or below the limit, including a zero-length frame. That boundary was not covered by a test. #### Modifications Add a test to each serializer asserting that a frame declaring length 0 deserializes to an empty result when a positive maxMessageSize is configured. #### Result The at-limit boundary now includes explicit coverage of the empty-frame case.

…age-size limit #### Motivation The maxMessageSize guard added to FixedLengthStreamingSerializer and VarIntLengthStreamingSerializer was opt-in (the 2-arg constructor defaulted to unlimited), so the common callers that use these via the 2-arg constructor -- the HTTP streaming string content types and the protobuf-over-HTTP streaming serializer -- remained unprotected. These APIs are not heavily used, so a safe default is preferable to leaving them unbounded. #### Modifications - The 2-arg constructors now default maxMessageSize to 4 MiB, resolved once from the io.servicetalk.serializer.utils.temporaryDefaultMaxMessageSize system property (a value <= 0 disables the limit) via the new StreamingSerializerDefaults holder, mirroring HttpConfig's aggregation-limit default. Setting the property logs a warning that it is temporary. - The HTTP string serializers inherit the default via the 2-arg constructor. For the protobuf-over-HTTP serializer, add streamingSerializerDeserializer overloads to ProtobufSerializerFactory that accept a maxMessageSize so callers can raise or disable the limit without the JVM-global property; the existing methods keep the default. The 3-arg serializer constructor remains the direct override. - Add tests asserting the default constructor rejects a frame above the default and that the ProtobufSerializerFactory limit is honored. The protobuf VarInt boundary-encoding tests, which round-trip a 256 MiB message, disable the limit since they exercise prefix encoding rather than the cap. #### Result Frames larger than 4 MiB on these streaming content types are now rejected by default. This is a behavior change: callers that legitimately stream larger frames must raise or disable the limit via the 3-arg constructor, the ProtobufSerializerFactory overloads, or the system property. Take care during upgrade.

#### Motivation The maxMessageSize guard threw a plain SerializationException, which HttpExceptionMapperServiceFilter maps to 415 Unsupported Media Type. An oversized message should map to 413 Payload Too Large. The serializer layer cannot throw the http-api PayloadTooLargeException (wrong dependency direction), and the mapper matches SerializationException before PayloadTooLargeException regardless. #### Modifications - Add MaxMessageSizeExceededException (extends SerializationException) in serializer-api, a transport-agnostic layer visible to both the serializers and the HTTP mapper. FixedLength/VarInt serializers throw it for the size-limit case; the malformed-length case stays a plain SerializationException (415). - HttpExceptionMapperServiceFilter maps MaxMessageSizeExceededException to PAYLOAD_TOO_LARGE, checked before the SerializationException branch since it is a subtype. - Tests for both mappings and for the specific exception type from the serializers. #### Result Servers reject oversized streaming frames with 413 rather than 415. The type is transport-neutral, so the separate gRPC work can map it to RESOURCE_EXHAUSTED.

Co-authored-by: Idel Pivnitskiy <ipivnitskiy@apple.com>

idelpivnitskiy

Approving with one question:

idelpivnitskiy · 2026-07-11T00:20:43Z

+     * negative values are rejected. This applies only to streaming deserialization; single-message serialization is
+     * not length-prefixed and is unaffected.
+     */
+    public ProtobufSerializerFactory(final int maxMessageSize) {


Wonder if users may find a use-case when they need different size limits for different classes (proto messages). If we think it's rare/unlikely, I guess users can still have a few instances of the factory for different limits, right?

Thinking about it more, method overloads were without caching which made them not much better than creating separate instances of ProtobufSerializerFactory with different limits in case users really need tighter control

I also think it's rare: most people would just want a greatest value and use it across all instances. As you noted, if they did want separate caps they can use multiple factories and presumably they'd be chosen based on class so they could each efficiently cache whichever instances they're interested in.

bryce-anderson requested review from daschl and idelpivnitskiy July 9, 2026 18:21

idelpivnitskiy reviewed Jul 9, 2026

View reviewed changes

bryce-anderson added 4 commits July 9, 2026 12:51

bryce-anderson requested a review from idelpivnitskiy July 9, 2026 23:00

bryce-anderson changed the title ~~serializer-utils: bound frame size in FixedLengthStreamingSerializer~~ serializer-utils: bound frame size in length-prefixed streaming serializers Jul 9, 2026

idelpivnitskiy reviewed Jul 10, 2026

View reviewed changes

idelpivnitskiy mentioned this pull request Jul 10, 2026

grpc: add configurable maxInboundMessageSize #3564

Merged

bryce-anderson and others added 3 commits July 10, 2026 09:49

Apply suggestions from code review

d3e2092

Co-authored-by: Idel Pivnitskiy <ipivnitskiy@apple.com>

More feedback

65f46ff

Warn mode

cd044c6

bryce-anderson requested a review from idelpivnitskiy July 10, 2026 17:35

dont use method overloads, make ProtobufSerializerFactory constructable

6085adf

idelpivnitskiy approved these changes Jul 11, 2026

View reviewed changes

bryce-anderson merged commit 13b8bef into apple:main Jul 13, 2026
19 checks passed

bryce-anderson deleted the bl_anderson/fixed-length-limits branch July 13, 2026 16:30

Uh oh!

Conversation

bryce-anderson commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Result

Uh oh!

Uh oh!

idelpivnitskiy Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

bryce-anderson Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

idelpivnitskiy Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

idelpivnitskiy left a comment

Choose a reason for hiding this comment

Uh oh!

idelpivnitskiy Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

idelpivnitskiy Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

bryce-anderson Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bryce-anderson commented Jul 9, 2026 •

edited

Loading