Skip to content

Commit 531c644

Browse files
Avoid potential exceptions on serialize in the face of malformed lazy extensions.
The behavior before this change was that when a corrupted lazy extension was seen, the first time parse is reached it actually discarded and 0-length byte is stored. This can led to exceptions in a case of - Parse some parent message with a messageset with an extension that has malformed bytes - Call getSerializedLength() on parent, which will see the lazy fields byte length and cache it - Call .get() on the bad extension, which sees the corruption, this resets only the inner cached size to -1 not any parents - Serialize the message, serialized length doesn't match the cached serialized length which will throw that the wrong number of bytes were written. After this change, we instead round-trip the original bytes in the face of corrupted data. This is consistent with cases like wrong-tag cases where we stuff the data into unknown fields instead and don't discard it. PiperOrigin-RevId: 844814654
1 parent b0a9ecc commit 531c644

File tree

4 files changed

+26
-44
lines changed

4 files changed

+26
-44
lines changed

java/core/src/main/java/com/google/protobuf/GeneratedMessage.java

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1087,13 +1087,6 @@ public final <T> T getExtension(final ExtensionLite<? extends MessageT, T> exten
10871087
} else {
10881088
result = (T) extension.fromReflectionType(value);
10891089
}
1090-
1091-
// If the lazy field is corrupted, we need to invalidate the memoized size in case the
1092-
// corrupted message data was replaced with an empty ByteString and yet a previous serialized
1093-
// size was memoized.
1094-
if (extensions.lazyFieldCorrupted(descriptor)) {
1095-
setMemoizedSerializedSize(-1);
1096-
}
10971090
return result;
10981091
}
10991092

java/core/src/main/java/com/google/protobuf/LazyFieldLite.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -336,10 +336,10 @@ public void setByteString(ByteString bytes, ExtensionRegistryLite extensionRegis
336336
public int getSerializedSize() {
337337
// We *must* return delayed bytes size if it was ever set because the dependent messages may
338338
// have memoized serialized size based off of it.
339-
if (memoizedBytes != null) {
340-
return memoizedBytes.size();
341-
} else if (delayedBytes != null) {
339+
if (delayedBytes != null) {
342340
return delayedBytes.size();
341+
} else if (memoizedBytes != null) {
342+
return memoizedBytes.size();
343343
} else if (value != null) {
344344
return value.getSerializedSize();
345345
} else {
@@ -349,14 +349,14 @@ public int getSerializedSize() {
349349

350350
/** Returns a BytesString for this field in a thread-safe way. */
351351
public ByteString toByteString() {
352-
if (memoizedBytes != null) {
353-
return memoizedBytes;
354-
}
355352
// We *must* return delayed bytes if it was set because the dependent messages may have
356353
// memoized serialized size based off of it.
357354
if (delayedBytes != null) {
358355
return delayedBytes;
359356
}
357+
if (memoizedBytes != null) {
358+
return memoizedBytes;
359+
}
360360
synchronized (this) {
361361
if (memoizedBytes != null) {
362362
return memoizedBytes;

java/core/src/test/java/com/google/protobuf/LazilyParsedMessageSetTest.java

Lines changed: 15 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import proto2_unittest.UnittestMset.TestMessageSetExtension2;
1515
import proto2_unittest.UnittestMset.TestMessageSetExtension3;
1616
import proto2_wireformat_unittest.UnittestMsetWireFormat.TestMessageSet;
17+
import proto2_wireformat_unittest.UnittestMsetWireFormat.TestMessageSetWireFormatContainer;
1718
import org.junit.Before;
1819
import org.junit.Test;
1920
import org.junit.runner.RunWith;
@@ -147,17 +148,10 @@ public void testLoadCorruptedLazyField_getsReplacedWithEmptyMessage() throws Exc
147148
// Serialize. The first extension should be serialized as an empty message.
148149
ByteString outputData = messageSet.toByteString();
149150

150-
// Re-parse as RawMessageSet
151+
// Round trip and confirm the corrupted payload is preserved.
151152
RawMessageSet actualRaw =
152153
RawMessageSet.parseFrom(outputData, ExtensionRegistry.getEmptyRegistry());
153-
154-
RawMessageSet expectedRaw =
155-
RawMessageSet.newBuilder()
156-
.addItem(
157-
RawMessageSet.Item.newBuilder().setTypeId(TYPE_ID_1).setMessage(ByteString.empty()))
158-
.build();
159-
160-
assertThat(actualRaw).isEqualTo(expectedRaw);
154+
assertThat(actualRaw).isEqualTo(inputRaw);
161155
}
162156

163157
@Test
@@ -174,27 +168,21 @@ public void testLoadCorruptedLazyField_getSerializedSize() throws Exception {
174168
ByteString inputData = inputRaw.toByteString();
175169
TestMessageSet messageSet = TestMessageSet.parseFrom(inputData, extensionRegistry);
176170

171+
TestMessageSetWireFormatContainer container =
172+
TestMessageSetWireFormatContainer.newBuilder().setMessageSet(messageSet).build();
173+
177174
// Effectively cache the serialized size of the message set.
178-
assertThat(messageSet.getSerializedSize()).isEqualTo(9);
175+
assertThat(container.getSerializedSize()).isEqualTo(11);
179176

180-
// getExtension should mark the memoized size as "dirty" (i.e. -1).
181-
assertThat(messageSet.getExtension(TestMessageSetExtension1.messageSetExtension))
177+
// getExtension will notice that the extension is corrupted and replace it with the default
178+
// empty message.
179+
assertThat(container.getMessageSet().getExtension(TestMessageSetExtension1.messageSetExtension))
182180
.isEqualTo(TestMessageSetExtension1.getDefaultInstance());
183181

184-
// toByteString calls getSerializedSize() which should re-compute the serialized size as the
185-
// message contains lazy fields.
186-
ByteString outputData = messageSet.toByteString();
187-
188-
// Re-parse as RawMessageSet
189-
RawMessageSet actualRaw =
190-
RawMessageSet.parseFrom(outputData, ExtensionRegistry.getEmptyRegistry());
191-
192-
RawMessageSet expectedRaw =
193-
RawMessageSet.newBuilder()
194-
.addItem(
195-
RawMessageSet.Item.newBuilder().setTypeId(TYPE_ID_1).setMessage(ByteString.empty()))
196-
.build();
197-
198-
assertThat(actualRaw).isEqualTo(expectedRaw);
182+
// Make sure that toByteString() works even though the total size has been cached, and round
183+
// tripping should keep equals().
184+
ByteString bytes = container.toByteString();
185+
assertThat(container)
186+
.isEqualTo(TestMessageSetWireFormatContainer.parseFrom(bytes, extensionRegistry));
199187
}
200188
}

java/core/src/test/java/com/google/protobuf/LazyFieldLiteTest.java

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,13 +118,14 @@ public void testEmptyLazyField() throws Exception {
118118
@Test
119119
public void testInvalidProto() throws Exception {
120120
// Silently fails and uses the default instance.
121-
LazyFieldLite field =
122-
new LazyFieldLite(TestUtil.getExtensionRegistry(), ByteString.copyFromUtf8("invalid"));
121+
ByteString invalid = ByteString.copyFromUtf8("invalid");
122+
LazyFieldLite field = new LazyFieldLite(TestUtil.getExtensionRegistry(), invalid);
123+
assertThat(field.getSerializedSize()).isEqualTo(7);
123124
assertThat(
124125
field.getValue(TestAllTypes.getDefaultInstance()))
125126
.isEqualTo(TestAllTypes.getDefaultInstance());
126-
assertThat(field.getSerializedSize()).isEqualTo(0);
127-
assertThat(field.toByteString()).isEqualTo(ByteString.EMPTY);
127+
assertThat(field.getSerializedSize()).isEqualTo(7);
128+
assertThat(field.toByteString()).isEqualTo(invalid);
128129
}
129130

130131
@Test

0 commit comments

Comments
 (0)