Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid holding onto the buffer when parsing unknown length-delimited fields #863

Merged
merged 5 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion protobuf/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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

Expand Down
6 changes: 2 additions & 4 deletions protobuf/lib/src/protobuf/coded_buffer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down
14 changes: 11 additions & 3 deletions protobuf/lib/src/protobuf/coded_buffer_reader.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -172,7 +180,7 @@ class CodedBufferReader {
readFixed64();
return true;
case WIRETYPE_LENGTH_DELIMITED:
readBytes();
readBytesAsView();
return true;
case WIRETYPE_FIXED32:
readFixed32();
Expand Down
12 changes: 7 additions & 5 deletions protobuf/lib/src/protobuf/message_set.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -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) ??
Expand Down
2 changes: 1 addition & 1 deletion protobuf/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
4 changes: 2 additions & 2 deletions protobuf/test/coded_buffer_reader_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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', () {
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion protoc_plugin/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 27 additions & 0 deletions protoc_plugin/test/unknown_field_set_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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);
});
}