feat(dart): add configurable deserialization size guardrails #3434
feat(dart): add configurable deserialization size guardrails #3434chaokunyang merged 5 commits intoapache:mainfrom
Conversation
| } | ||
| } | ||
|
|
||
| class DeserializationSizeException extends DeserializationException { |
There was a problem hiding this comment.
We dont' need so many special exception. Could you create a InvalidDataException in dart/packages/fory/lib/src/exception/fory_exception.dart?
Adn open anoterh PR to merge all excpetions into one file name fory_exception.dart under dart/packages/fory/lib/src/ dir? Current fory dart is too java style
| @override | ||
| T read(ByteReader br, int refId, DeserializationContext pack) { | ||
| int remaining = br.readVarUint32Small7(); | ||
| final int? maxCollectionSize = pack.config.maxCollectionSize; |
There was a problem hiding this comment.
you should set a default value for maxCollectionSize and maxBinary Size instead use null to span across whole repo
There was a problem hiding this comment.
Set maxCollectionSize to 1000_000, and maxBinarySize default to 64MB
|
Thanks for the reviews, I have made the changes and will also put up a PR for the exceptions. |
| 'Collection', remaining, maxCollectionSize); | ||
| if (remaining > pack.config.maxCollectionSize) { | ||
| throw InvalidDataException( | ||
| 'Collection size $remaining exceeds limit ${pack.config.maxCollectionSize}'); |
There was a problem hiding this comment.
Please refine thev error message, to tell user either the inout data is malicouse , or user need to increase maxCollectionSize or maxBinarySize when creating Fory. ditto for other InvalidDataException construction
yash-agarwa-l
force-pushed
the
dart_size_limits
branch
from
b803c75 to
a74b2aa
Compare
|
changes made, PTAL. |
|
Please rename |
| }); | ||
| }); | ||
|
|
||
| group('combined guardrails', () { |
There was a problem hiding this comment.
rename all guardrails to guard check
dart/packages/fory-test/test/config_test/size_guardrail_test.dart
Outdated
Show resolved
Hide resolved
|
done! |
|
You added two same tests file...:
|
yash-agarwa-l
force-pushed
the
dart_size_limits
branch
from
1a0b9f0 to
d9c6495
Compare
|
My bad, I missed to add that change that time |
…on.dart (#3436) ## Why? The current Dart exception hierarchy spreads runtime exceptions across four separate files under `dart/packages/fory/lib/src/exception/`, which is overly Java-style for idiomatic Dart. ## What needs to be done? - Merge all runtime exception classes from: - `exception/**` - `codegen/exception/**` into a single file: `dart/packages/fory/lib/src/fory_exception.dart` - Update all imports across serializers, resolvers, context, and codegen base to point to the new file. - Delete the `exception/` subdirectory and the `codegen/exception/` subdirectory. ## Does this introduce any user-facing change? - [x] Does this introduce any public API change? (import paths change) - [ ] Does this introduce any binary protocol compatibility change? ## Related - Requested in #3434 review: #3434 (comment)
2 participants
Why?
Untrusted payloads can encode arbitrarily large collection/binary lengths,
causing pre-allocation of huge buffers and OOM crashes before any
elements are even deserialized.
What does this PR do?
maxBinarySizeandmaxCollectionSizeto
ForyConfig(defaults to 1000000 Collection Size and 64MB for BinarySize).ForyConfigintoDeserializationContextso serializers canaccess limits at read time.
InvalidDataExceptionfollowing existing exception conventions.maxCollectionSizeinListSerializer,SetSerializer, andMapSerializerbefore pre-allocation.maxBinarySizeinNumericArraySerializerforObjType.BINARYbefore buffer copy.
and default scenarios for lists, sets, maps, and binary.
Related issues
Closes #3415.
Does this PR introduce any user-facing change?
Benchmark
N/A — guard checks are a single null-check + comparison per
collection/binary read; no measurable performance impact.