From 338b8591f73884f9657c6dfb061c3182b080606e Mon Sep 17 00:00:00 2001 From: Adwait Kumar Singh Date: Sun, 16 Aug 2026 14:07:16 +0530 Subject: [PATCH] Fix four out-of-bounds defects in the Query serializer --- .../aws-client-awsquery/build.gradle.kts | 1 + .../QueryFormSerializerBenchmark.java | 61 +++ .../client/awsquery/QueryFormSerializer.java | 81 +++- .../awsquery/QueryFormSerializerTest.java | 407 ++++++++++++++++-- 4 files changed, 517 insertions(+), 33 deletions(-) create mode 100644 aws/client/aws-client-awsquery/src/jmh/java/software/amazon/smithy/java/aws/client/awsquery/QueryFormSerializerBenchmark.java diff --git a/aws/client/aws-client-awsquery/build.gradle.kts b/aws/client/aws-client-awsquery/build.gradle.kts index 1ff2a0f70f..b182c8ffdc 100644 --- a/aws/client/aws-client-awsquery/build.gradle.kts +++ b/aws/client/aws-client-awsquery/build.gradle.kts @@ -1,5 +1,6 @@ plugins { id("smithy-java.module-conventions") + id("smithy-java.jmh-conventions") id("smithy-java.protocol-testing-conventions") } diff --git a/aws/client/aws-client-awsquery/src/jmh/java/software/amazon/smithy/java/aws/client/awsquery/QueryFormSerializerBenchmark.java b/aws/client/aws-client-awsquery/src/jmh/java/software/amazon/smithy/java/aws/client/awsquery/QueryFormSerializerBenchmark.java new file mode 100644 index 0000000000..4f921cde22 --- /dev/null +++ b/aws/client/aws-client-awsquery/src/jmh/java/software/amazon/smithy/java/aws/client/awsquery/QueryFormSerializerBenchmark.java @@ -0,0 +1,61 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +package software.amazon.smithy.java.aws.client.awsquery; + +import java.nio.ByteBuffer; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import software.amazon.smithy.java.core.schema.PreludeSchemas; +import software.amazon.smithy.java.core.schema.Schema; +import software.amazon.smithy.model.shapes.ShapeId; + +@State(Scope.Thread) +public class QueryFormSerializerBenchmark { + + @Param({ + "ascii_128", + "unicode_first_128", + "unicode_last_128", + "cjk_128", + "ascii_8192", + "unicode_last_8192", + }) + public String testCaseId; + + private Schema member; + private String value; + + @Setup + public void setup() { + int separator = testCaseId.lastIndexOf('_'); + int length = Integer.parseInt(testCaseId.substring(separator + 1)); + value = switch (testCaseId.substring(0, separator)) { + case "ascii" -> "a".repeat(length); + case "unicode_first" -> "日" + "a".repeat(length - 1); + case "unicode_last" -> "a".repeat(length - 1) + "日"; + case "cjk" -> "日".repeat(length); + default -> throw new IllegalArgumentException("Unknown test case: " + testCaseId); + }; + + Schema struct = Schema.structureBuilder(ShapeId.from("smithy.benchmark#Input")) + .putMember("value", PreludeSchemas.STRING) + .build(); + member = struct.member("value"); + } + + @Benchmark + public ByteBuffer serializeString() { + QueryFormSerializer serializer = QueryFormSerializer.acquire( + QueryFormSerializer.QueryVariant.AWS_QUERY, + "Benchmark", + "2020-01-01"); + serializer.writeString(member, value); + return serializer.finish(); + } +} diff --git a/aws/client/aws-client-awsquery/src/main/java/software/amazon/smithy/java/aws/client/awsquery/QueryFormSerializer.java b/aws/client/aws-client-awsquery/src/main/java/software/amazon/smithy/java/aws/client/awsquery/QueryFormSerializer.java index 3d736f589f..980efba931 100644 --- a/aws/client/aws-client-awsquery/src/main/java/software/amazon/smithy/java/aws/client/awsquery/QueryFormSerializer.java +++ b/aws/client/aws-client-awsquery/src/main/java/software/amazon/smithy/java/aws/client/awsquery/QueryFormSerializer.java @@ -73,6 +73,8 @@ enum QueryVariant { private static final int DEFAULT_BUF_SIZE = 1024; private static final int MAX_CACHEABLE_BUF = DEFAULT_BUF_SIZE * 4; + private static final int MAX_BYTES_PER_CHAR = 9; + record AcquireContext(QueryVariant variant, String action, String version) {} private static final StripedPool POOL = @@ -139,7 +141,14 @@ static QueryFormSerializer acquire(QueryVariant variant, String action, String v } ByteBuffer finish() { - ByteBuffer result = ByteBuffer.wrap(buf, 0, pos); + ByteBuffer result; + if (buf.length > MAX_CACHEABLE_BUF) { + byte[] resultBuf = buf; + buf = new byte[DEFAULT_BUF_SIZE]; + result = ByteBuffer.wrap(resultBuf, 0, pos).slice(); + } else { + result = ByteBuffer.wrap(Arrays.copyOf(buf, pos)); + } POOL.release(this); return result; } @@ -223,7 +232,32 @@ private void writeUrlEncodedAsciiBytes(byte[] data, int dataLen) { private void writeUrlEncoded(String s) { int len = s.length(); + int next = pos; for (int i = 0; i < len; i++) { + char c = s.charAt(i); + if (c >= 0x80) { + ensureCapacity(next - pos + (len - i) * MAX_BYTES_PER_CHAR); + // Read buf after ensureCapacity: it may have replaced the array. + pos = writeUrlEncodedRemainder(buf, next, s, i); + return; + } + if (UNRESERVED[c]) { + buf[next++] = (byte) c; + } else { + int off = c * 3; + buf[next] = PERCENT_ENCODED[off]; + buf[next + 1] = PERCENT_ENCODED[off + 1]; + buf[next + 2] = PERCENT_ENCODED[off + 2]; + next += 3; + } + } + pos = next; + } + + // Encodes the remaining arbitrary characters. The caller must reserve nine bytes per char. + private static int writeUrlEncodedRemainder(byte[] buf, int pos, String s, int start) { + int len = s.length(); + for (int i = start; i < len; i++) { char c = s.charAt(i); if (c < 0x80) { if (UNRESERVED[c]) { @@ -269,6 +303,16 @@ private void writeUrlEncoded(String s) { pos += 3; } } + return pos; + } + + /** + * Upper bound on the base-10 ASCII bytes {@link NumberCodec#writeBigInteger} writes to the form body. + */ + static int maxBigIntegerLength(BigInteger value) { + // log10(2) is just over 0.301; the larger factor plus constants cover rounding and a sign. + int digits = (int) (value.bitLength() * 0.302) + 2; + return 1 + digits; } /** @@ -431,9 +475,18 @@ private void writeEc2List(Schema schema, T listState, int size, BiConsumer void writeList(Schema schema, T listState, int size, BiConsumer void writeMap(Schema schema, T mapState, int size, BiConsumer ser.writeList((Schema) m, null, 2, (st, outer) -> { + for (int i = 1; i <= 2; i++) { + int n = i; + outer.writeMap(outerMember, + null, + 1, + (s1, entries) -> entries.writeEntry(mapKey, + "k" + n, + null, + (t, vs) -> vs.writeList(mapValue, null, 2, (s2, items) -> { + items.writeInteger(intMember, n * 10); + items.writeInteger(intMember, n * 10 + 1); + }))); + } + })); + + // The doubled ".member" is the outer element prefix plus the map member's own name. + assertThat(out, containsString("ListOfMaps.member.1.member.entry.1.key=k1")); + assertThat(out, containsString("ListOfMaps.member.1.member.entry.1.value.member.1=10")); + assertThat(out, containsString("ListOfMaps.member.1.member.entry.1.value.member.2=11")); + assertThat(out, containsString("ListOfMaps.member.2.member.entry.1.key=k2")); + assertThat(out, containsString("ListOfMaps.member.2.member.entry.1.value.member.1=20")); + assertThat(out, containsString("ListOfMaps.member.2.member.entry.1.value.member.2=21")); + // Without the fix the inner list leaves the shared index at 3, so the second map lands on + // "member.3" and the two outer elements are no longer consecutive. + assertThat(out, not(containsString("ListOfMaps.member.3"))); + } + + /** A map value that is itself a map, reached without a struct in between. */ + @Test + void mapOfMapsKeepsOuterEntryIndex() { + Schema innerMap = Schema.mapBuilder(ShapeId.from("smithy.test#Inner")) + .putMember("key", PreludeSchemas.STRING) + .putMember("value", PreludeSchemas.STRING) + .build(); + Schema outerMap = Schema.mapBuilder(ShapeId.from("smithy.test#MapOfMaps")) + .putMember("key", PreludeSchemas.STRING) + .putMember("value", innerMap) + .build(); + + Schema outerKey = outerMap.member("key"); + Schema outerValue = outerMap.member("value"); + Schema innerKey = innerMap.member("key"); + Schema innerValue = innerMap.member("value"); + + String out = serialize(outerMap, (m, ser) -> ser.writeMap((Schema) m, null, 2, (st, outer) -> { + for (int i = 1; i <= 2; i++) { + int n = i; + outer.writeEntry(outerKey, + "o" + n, + null, + (s1, ov) -> ov.writeMap(outerValue, + null, + 1, + (s2, inner) -> inner + .writeEntry(innerKey, + "i" + n, + null, + (t, vs) -> vs.writeString( + innerValue, + "v" + n)))); + } + })); + + assertThat(out, containsString("MapOfMaps.entry.1.key=o1")); + assertThat(out, containsString("MapOfMaps.entry.1.value.entry.1.value=v1")); + assertThat(out, containsString("MapOfMaps.entry.2.key=o2")); + assertThat(out, containsString("MapOfMaps.entry.2.value.entry.1.value=v2")); + // Without the fix the inner map leaves the shared index at 2, so the second entry is "entry.3". + assertThat(out, not(containsString("MapOfMaps.entry.3"))); + } + + /** EC2 Query has its own list writer, which shares the same list serializer instance. */ + @Test + void ec2ListOfListsKeepsOuterIndex() { + Schema member = outerList.member("member"); + Schema innerMember = innerList.member("member"); + + String out = QueryFormSerializerTest.serialize(QueryFormSerializer.QueryVariant.EC2_QUERY, + outerList, + (m, ser) -> ser.writeList((Schema) m, null, 2, (st, outer) -> { + outer.writeList(member, null, 2, (s1, inner1) -> { + inner1.writeInteger(innerMember, 10); + inner1.writeInteger(innerMember, 20); + }); + outer.writeList(member, null, 2, (s2, inner2) -> { + inner2.writeInteger(innerMember, 30); + inner2.writeInteger(innerMember, 40); + }); + })); + + // EC2 lists are always flattened, so the element index follows the prefix directly; the + // capitalized "Member" is the inner list member's name under EC2 naming. + assertThat(out, containsString("OuterList.1.Member.1=10")); + assertThat(out, containsString("OuterList.1.Member.2=20")); + assertThat(out, containsString("OuterList.2.Member.1=30")); + assertThat(out, containsString("OuterList.2.Member.2=40")); + assertThat(out, not(containsString("OuterList.3"))); + } + @Test void flatListIsUnaffected() { Schema member = innerList.member("member"); @@ -212,36 +339,9 @@ void flatListIsUnaffected() { assertThat(out, containsString("InnerList.member.3=9")); } - // Serialize a single-member struct via writeMember and return the query string. private String serialize(Schema memberSchema, BiConsumer writeMember) { - Schema structSchema = Schema.structureBuilder(ShapeId.from("smithy.test#Outer")) - .putMember(memberSchema.id().getName(), memberSchema) - .build(); - Schema member = structSchema.member(memberSchema.id().getName()); - - SerializableStruct struct = new SerializableStruct() { - @Override - public Schema schema() { - return structSchema; - } - - @Override - public void serializeMembers(ShapeSerializer serializer) { - writeMember.accept(member, serializer); - } - - @Override - public T getMemberValue(Schema m) { - return null; - } - }; - - QueryFormSerializer s = QueryFormSerializer.acquire( - QueryFormSerializer.QueryVariant.AWS_QUERY, - "TestAction", - "2020-01-01"); - s.writeStruct(structSchema, struct); - return StandardCharsets.UTF_8.decode(s.finish()).toString(); + return QueryFormSerializerTest + .serialize(QueryFormSerializer.QueryVariant.AWS_QUERY, memberSchema, writeMember); } // A struct {inner: {i: }} that serializes its map member. @@ -269,6 +369,255 @@ public T getMemberValue(Schema m) { } } + /** + * Buffer bounds. + * + *

Every parameter is written by reserving a computed upper bound and then encoding into the + * reserved space with no further checks, so a bound that under-counts is an out-of-bounds array + * write rather than a wrong answer. These are the cases where the bound is not simply the value's + * length: a character can encode to nine bytes, and a {@code BigInteger} has no fixed length at all. + */ + @Nested + class BufferBounds { + + /** Three UTF-8 bytes, each percent-encoding to three: nine bytes for one {@code char}. */ + private static final String CJK = "日"; + private static final String CJK_ENCODED = "%E6%97%A5"; + + /** Long enough that the encoded form overruns both the reservation and the initial buffer. */ + private static final int LONG = 400; + + @Test + void longNonAsciiStringIsWrittenInFull() { + String out = serializeString(CJK.repeat(LONG)); + assertThat(out, equalTo(header() + "&String=" + CJK_ENCODED.repeat(LONG))); + } + + @Test + void asciiPrefixIsWrittenOnceWhenTheTailIsNotAscii() { + String out = serializeString("a".repeat(LONG) + CJK.repeat(LONG)); + assertThat(out, equalTo(header() + "&String=" + "a".repeat(LONG) + CJK_ENCODED.repeat(LONG))); + } + + @Test + void longSurrogatePairStringIsWrittenInFull() { + String out = serializeString("🎉".repeat(LONG)); + assertThat(out, equalTo(header() + "&String=" + "%F0%9F%8E%89".repeat(LONG))); + } + + @Test + void longNonAsciiListElementIsWrittenInFull() { + Schema list = Schema.listBuilder(ShapeId.from("smithy.test#StringList")) + .putMember("member", PreludeSchemas.STRING) + .build(); + Schema element = list.member("member"); + String out = serialize(QueryFormSerializer.QueryVariant.AWS_QUERY, + list, + (m, ser) -> ser.writeList((Schema) m, + null, + 1, + (st, items) -> items.writeString(element, CJK.repeat(LONG)))); + assertThat(out, equalTo(header() + "&StringList.member.1=" + CJK_ENCODED.repeat(LONG))); + } + + @Test + void longNonAsciiMapKeyAndValueAreWrittenInFull() { + Schema map = Schema.mapBuilder(ShapeId.from("smithy.test#StringMap")) + .putMember("key", PreludeSchemas.STRING) + .putMember("value", PreludeSchemas.STRING) + .build(); + Schema key = map.member("key"); + Schema value = map.member("value"); + String big = CJK.repeat(LONG); + String encoded = CJK_ENCODED.repeat(LONG); + String out = serialize(QueryFormSerializer.QueryVariant.AWS_QUERY, + map, + (m, ser) -> ser.writeMap((Schema) m, + null, + 1, + (st, entries) -> entries + .writeEntry(key, big, null, (t, vs) -> vs.writeString(value, big)))); + assertThat(out, + equalTo(header() + "&StringMap.entry.1.key=" + encoded + + "&StringMap.entry.1.value=" + encoded)); + } + + @Test + void arbitrarilyLongBigIntegerIsWrittenInFull() { + BigInteger value = BigInteger.TEN.pow(2000); + String out = serialize(QueryFormSerializer.QueryVariant.AWS_QUERY, + PreludeSchemas.BIG_INTEGER, + (m, ser) -> ser.writeBigInteger((Schema) m, value)); + assertThat(out, equalTo(header() + "&BigInteger=" + value)); + } + + @Test + void arbitrarilyLongBigIntegerListElementIsWrittenInFull() { + Schema list = Schema.listBuilder(ShapeId.from("smithy.test#BigList")) + .putMember("member", PreludeSchemas.BIG_INTEGER) + .build(); + Schema element = list.member("member"); + BigInteger value = BigInteger.TEN.pow(2000); + String out = serialize(QueryFormSerializer.QueryVariant.AWS_QUERY, + list, + (m, ser) -> ser.writeList((Schema) m, + null, + 1, + (st, items) -> items.writeBigInteger(element, value))); + assertThat(out, equalTo(header() + "&BigList.member.1=" + value)); + } + + @Test + void arbitrarilyLongBigIntegerMapValueIsWrittenInFull() { + Schema map = Schema.mapBuilder(ShapeId.from("smithy.test#BigMap")) + .putMember("key", PreludeSchemas.STRING) + .putMember("value", PreludeSchemas.BIG_INTEGER) + .build(); + Schema key = map.member("key"); + Schema value = map.member("value"); + BigInteger big = BigInteger.TEN.pow(2000); + String out = serialize(QueryFormSerializer.QueryVariant.AWS_QUERY, + map, + (m, ser) -> ser.writeMap((Schema) m, + null, + 1, + (st, entries) -> entries + .writeEntry(key, "k", null, (t, vs) -> vs.writeBigInteger(value, big)))); + assertThat(out, equalTo(header() + "&BigMap.entry.1.key=k&BigMap.entry.1.value=" + big)); + } + + @Test + void maxBigIntegerLengthBoundsDecimalEncoding() { + BigInteger[] boundaries = { + BigInteger.ZERO, + BigInteger.ONE, + BigInteger.ONE.negate(), + BigInteger.TEN.pow(18), + BigInteger.TEN.pow(18).negate(), + BigInteger.TEN.pow(2000), + BigInteger.TEN.pow(2000).negate(), + }; + for (BigInteger value : boundaries) { + assertBigIntegerFits(value); + } + + for (int bits = 1; bits <= 4096; bits += 31) { + BigInteger powerOfTwo = BigInteger.ONE.shiftLeft(bits); + BigInteger belowPowerOfTwo = powerOfTwo.subtract(BigInteger.ONE); + assertBigIntegerFits(powerOfTwo); + assertBigIntegerFits(powerOfTwo.negate()); + assertBigIntegerFits(belowPowerOfTwo); + assertBigIntegerFits(belowPowerOfTwo.negate()); + } + } + + /** + * The serializer is pooled, so a body wrapping the pooled array would be rewritten in place by + * the next request on the same thread. + */ + @Test + void finishDoesNotAliasThePooledBuffer() { + ByteBuffer first = QueryFormSerializer + .acquire(QueryFormSerializer.QueryVariant.AWS_QUERY, "A1", "V1") + .finish(); + ByteBuffer second = QueryFormSerializer + .acquire(QueryFormSerializer.QueryVariant.AWS_QUERY, "A2", "V2") + .finish(); + assertThat(first.array(), not(sameInstance(second.array()))); + } + + @Test + void finishedBodyIsUnaffectedByTheNextSerializer() { + ByteBuffer first = QueryFormSerializer + .acquire(QueryFormSerializer.QueryVariant.AWS_QUERY, "A1", "V1") + .finish(); + QueryFormSerializer + .acquire(QueryFormSerializer.QueryVariant.AWS_QUERY, "A2", "V2") + .finish(); + assertThat(StandardCharsets.UTF_8.decode(first).toString(), equalTo("Action=A1&Version=V1")); + } + + @Test + void finishTransfersOversizedBuffer() { + String value = "a".repeat(8192); + ByteBuffer body = serializeStringBuffer(value); + + // The returned view has an exact limit, but keeps the oversized backing array rather than + // copying it immediately before the pool would discard it. + assertThat(body.array().length > body.remaining(), equalTo(true)); + QueryFormSerializer.acquire(QueryFormSerializer.QueryVariant.AWS_QUERY, "A2", "V2").finish(); + assertThat(StandardCharsets.UTF_8.decode(body).toString(), + equalTo(header() + "&String=" + value)); + } + + private void assertBigIntegerFits(BigInteger value) { + int maxLength = QueryFormSerializer.maxBigIntegerLength(value); + byte[] bytes = new byte[maxLength]; + int end = NumberCodec.writeBigInteger(bytes, 0, value); + assertThat(end, lessThanOrEqualTo(maxLength)); + assertThat(new String(bytes, 0, end, StandardCharsets.US_ASCII), equalTo(value.toString())); + } + + private String serializeString(String value) { + return StandardCharsets.UTF_8.decode(serializeStringBuffer(value)).toString(); + } + + private ByteBuffer serializeStringBuffer(String value) { + Schema struct = Schema.structureBuilder(ShapeId.from("smithy.test#Outer")) + .putMember("String", PreludeSchemas.STRING) + .build(); + QueryFormSerializer serializer = QueryFormSerializer.acquire( + QueryFormSerializer.QueryVariant.AWS_QUERY, + "TestAction", + "2020-01-01"); + serializer.writeString(struct.member("String"), value); + return serializer.finish(); + } + + private String header() { + return "Action=TestAction&Version=2020-01-01"; + } + } + + /** + * Wraps {@code memberSchema} in a single-member structure, serializes it through the serializer, and + * returns the query string. + * + *

{@code writeMember} receives the member schema of the wrapper struct, not {@code memberSchema} + * itself, because that is what a generated or hand-written shape would pass. + */ + static String serialize( + QueryFormSerializer.QueryVariant variant, + Schema memberSchema, + BiConsumer writeMember + ) { + Schema structSchema = Schema.structureBuilder(ShapeId.from("smithy.test#Outer")) + .putMember(memberSchema.id().getName(), memberSchema) + .build(); + Schema member = structSchema.member(memberSchema.id().getName()); + + SerializableStruct struct = new SerializableStruct() { + @Override + public Schema schema() { + return structSchema; + } + + @Override + public void serializeMembers(ShapeSerializer serializer) { + writeMember.accept(member, serializer); + } + + @Override + public T getMemberValue(Schema m) { + return null; + } + }; + + QueryFormSerializer s = QueryFormSerializer.acquire(variant, "TestAction", "2020-01-01"); + s.writeStruct(structSchema, struct); + return StandardCharsets.UTF_8.decode(s.finish()).toString(); + } + static Stream reservedCharactersProvider() { return Stream.of( Arguments.of(" ", "%20"),