Avoid holding onto the buffer when parsing unknown length-delimited f…

…ields (#863)

Currently unknown field set parser stores length-delimited fields as
views of the input buffer. This has a few issues:

- It's inconsistent with the known-field parsing where we copy `bytes`
fields.

- A single unknown length-delimited field can cause keeping a large
input buffer alive.

- Because the caller is free to modify the input buffer, this
implementation does not allow freezing an unknown field set.

(This can also be fixed by copying the length-delimited fields when
freezing.)

- Even when the parsed message is not frozen, this aliasing can cause
bugs as it's not documented. Even if we document it, it would probably
be a footgun.

This can cause segfaults or worse when the input buffer is passed from
e.g. C++ or C as a `Uint8List`, and the caller frees the buffer after
parsing while the message is still in use.

This PR makes `readBytes` copy the bytes before returning to avoid
aliasing. A new member `readBytesAsView` added with the previous
behavior. Unknown length-delimited field parsing fixed with the new
`readBytes`.

Sync CL: cl/552077275

protobuf/CHANGELOG.md

@@ -1,4 +1,13 @@
-## 3.0.1-dev
+## 3.1.0-dev
+
+* `CodedBufferReader` `readBytes` now copies the returned bytes to avoid
+  accidental sharing of the input buffer with the returned `Uint8List`. New
+  member `readBytesAsView` added with the old behavior. ([#863])
+
+* Avoid sharing the input buffer in unknown length-delimited fields using the
+  new `readBytes`. ([#863])
+
+[#863]: https://github.com/google/protobuf.dart/pull/863
 
 ## 3.0.0
 

protobuf/lib/src/protobuf/coded_buffer.dart

@@ -57,7 +57,7 @@ void _mergeFromCodedBufferReader(BuilderInfo meta, _FieldSet fs,
         fs._setFieldUnchecked(meta, fi, input.readBool());
         break;
       case PbFieldType._OPTIONAL_BYTES:
-        fs._setFieldUnchecked(meta, fi, Uint8List.fromList(input.readBytes()));
+        fs._setFieldUnchecked(meta, fi, input.readBytes());
         break;
       case PbFieldType._OPTIONAL_STRING:
         fs._setFieldUnchecked(meta, fi, input.readString());
@@ -131,9 +131,7 @@ void _mergeFromCodedBufferReader(BuilderInfo meta, _FieldSet fs,
         _readPackable(meta, fs, input, wireType, fi, input.readBool);
         break;
       case PbFieldType._REPEATED_BYTES:
-        fs
-            ._ensureRepeatedField(meta, fi)
-            .add(Uint8List.fromList(input.readBytes()));
+        fs._ensureRepeatedField(meta, fi).add(input.readBytes());
         break;
       case PbFieldType._REPEATED_STRING:
         fs._ensureRepeatedField(meta, fi).add(input.readString());

protobuf/lib/src/protobuf/coded_buffer_reader.dart

@@ -133,14 +133,22 @@ class CodedBufferReader {
   }
 
   bool readBool() => _readRawVarint32(true) != 0;
-  List<int> readBytes() {
+
+  /// Read a length-delimited field as bytes.
+  Uint8List readBytes() => Uint8List.fromList(readBytesAsView());
+
+  /// Read a length-delimited field as a view of the [CodedBufferReader]'s
+  /// buffer. When storing the returned value directly (instead of e.g. parsing
+  /// it as a UTF-8 string and copying) use [readBytes] instead to avoid
+  /// holding on to the whole message, or copy the returned view.
+  Uint8List readBytesAsView() {
     final length = readInt32();
     _checkLimit(length);
     return Uint8List.view(
         _buffer.buffer, _buffer.offsetInBytes + _bufferPos - length, length);
   }
 
-  String readString() => _utf8.decode(readBytes());
+  String readString() => _utf8.decode(readBytesAsView());
   double readFloat() => _readByteData(4).getFloat32(0, Endian.little);
   double readDouble() => _readByteData(8).getFloat64(0, Endian.little);
 
@@ -172,7 +180,7 @@ class CodedBufferReader {
         readFixed64();
         return true;
       case WIRETYPE_LENGTH_DELIMITED:
-        readBytes();
+        readBytesAsView();
         return true;
       case WIRETYPE_FIXED32:
         readFixed32();

protobuf/lib/src/protobuf/message_set.dart

@@ -80,7 +80,7 @@ abstract class $_MessageSet extends GeneratedMessage {
       //
       // We can see the fields in any order, so loop until parsing both fields.
       int? typeId;
-      List<int>? message;
+      Uint8List? message;
       while (true) {
         final tag = input.readTag();
         final tagNumber = getTagFieldNumber(tag);
@@ -97,7 +97,7 @@ abstract class $_MessageSet extends GeneratedMessage {
             message = null;
           }
         } else if (tagNumber == _messageSetItemMessageTag) {
-          message = input.readBytes();
+          message = input.readBytesAsView();
           if (typeId != null) {
             _parseExtension(typeId, message, extensionRegistry);
             typeId = null;
@@ -121,15 +121,17 @@ abstract class $_MessageSet extends GeneratedMessage {
   }
 
   void _parseExtension(
-      int typeId, List<int> message, ExtensionRegistry extensionRegistry) {
+      int typeId, Uint8List message, ExtensionRegistry extensionRegistry) {
     final ext =
         extensionRegistry.getExtension(info_.qualifiedMessageName, typeId);
     if (ext == null) {
       final messageItem = UnknownFieldSet();
       messageItem.addField(_messageSetItemTypeIdTag,
           UnknownFieldSetField()..varints.add(Int64(typeId)));
-      messageItem.addField(_messageSetItemMessageTag,
-          UnknownFieldSetField()..lengthDelimited.add(message));
+      messageItem.addField(
+          _messageSetItemMessageTag,
+          UnknownFieldSetField()
+            ..lengthDelimited.add(Uint8List.fromList(message)));
 
       final itemListField =
           _fieldSet._ensureUnknownFields().getField(_messageSetItemsTag) ??

protobuf/pubspec.yaml

@@ -1,5 +1,5 @@
 name: protobuf
-version: 3.0.1-dev
+version: 3.1.0-dev
 description: >-
   Runtime library for protocol buffers support. Use with package:protoc_plugin
   to generate dart code for your '.proto' files.

protobuf/test/coded_buffer_reader_test.dart

@@ -76,7 +76,7 @@ void main() {
       expect(cis.readString(), 'optional_string');
 
       expect(cis.readTag(), makeTag(115, WIRETYPE_LENGTH_DELIMITED));
-      expect(cis.readBytes(), 'optional_bytes'.codeUnits);
+      expect(cis.readBytesAsView(), 'optional_bytes'.codeUnits);
     }
 
     test('normal-list', () {
@@ -113,7 +113,7 @@ void main() {
     final input = CodedBufferReader(output.toBuffer());
     expect(input.readTag(), tag);
 
-    expect(input.readBytes, throwsInvalidProtocolBufferException);
+    expect(input.readBytesAsView, throwsInvalidProtocolBufferException);
   });
 
   /// Tests that if we read a string that contains invalid UTF-8, no exception

protoc_plugin/pubspec.yaml

@@ -9,7 +9,7 @@ environment:
 dependencies:
   fixnum: ^1.0.0
   path: ^1.8.0
-  protobuf: ^3.0.0
+  protobuf: ^3.1.0
 
 dev_dependencies:
   collection: ^1.15.0

protoc_plugin/test/unknown_field_set_test.dart

@@ -2,6 +2,8 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'dart:typed_data';
+
 import 'package:protobuf/protobuf.dart';
 import 'package:test/test.dart';
 
@@ -326,4 +328,29 @@ void main() {
     final m2 = TestAllExtensions()..unknownFields;
     expect(m.hashCode, m2.hashCode);
   });
+
+  test('Copy length delimited fields', () {
+    // Length-delimited fields should be copied before adding to the unknown
+    // field set to avoid aliasing.
+    final originalBytes = [1, 2, 3, 4, 5, 6];
+    final bytes = Uint8List.fromList([
+      10, // tag = 1, type = length delimited
+      originalBytes.length,
+      ...originalBytes
+    ]);
+
+    final parsed = UnknownFieldSet()
+      ..mergeFromCodedBufferReader(CodedBufferReader(bytes));
+
+    expect(parsed.getField(1)?.lengthDelimited, [originalBytes]);
+
+    // Modify the message. Input buffer should not be updated.
+    final newBytes = [9, 8, 7, 6, 5, 4];
+    parsed.getField(1)!.lengthDelimited[0].setRange(0, 6, newBytes);
+    expect(bytes.sublist(2), originalBytes);
+
+    // Modify the input buffer. Message should not be updated.
+    bytes.setRange(2, 8, [10, 11, 12, 13, 14, 15]);
+    expect(parsed.getField(1)!.lengthDelimited[0], newBytes);
+  });
 }