Fuzz Json Deserializing #117956
Fuzz Json Deserializing #117956
Conversation
BrennanConroy
requested review from
PranavSenthilnathan,
Copilot and
eiriktsarpalis
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new fuzzer called Utf8JsonReaderFuzzer to the .NET runtime fuzzing infrastructure. The fuzzer is designed to comprehensively test JSON deserialization functionality by comparing behavior between span-based and sequence-based Utf8JsonReader instances.
Key changes:
- Implements a comprehensive JSON reader fuzzer that tests various data types and scenarios
- Adds pipeline configuration to deploy the fuzzer to OneFuzz testing infrastructure
- Includes testing for both primitive types and complex POCOs with dynamically generated types
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/Utf8JsonReaderFuzzer.cs |
Main fuzzer implementation with comprehensive JSON deserialization testing |
src/libraries/Fuzzing/DotnetFuzzing/DotnetFuzzing.csproj |
Project file updated to include the new fuzzer |
eng/pipelines/libraries/fuzzing/deploy-to-onefuzz.yml |
Pipeline configuration to deploy the fuzzer to OneFuzz |
| } | ||
| catch (InvalidOperationException ex) when (ex.Message.Contains("surrogate")) | ||
| { | ||
| // If the string is not a valid DateTime, we expect an exception |
There was a problem hiding this comment.
Comment is incorrect - the code is testing GetString() method, not DateTime parsing. The comment should reflect that this handles invalid surrogate characters in strings.
| // If the string is not a valid DateTime, we expect an exception | |
| // If the string contains invalid surrogate characters, we expect an exception |
| } | ||
| catch (InvalidOperationException ex) when (ex.Message.Contains("surrogate")) | ||
| { | ||
| // If the string is not a valid DateTime, we expect an exception |
There was a problem hiding this comment.
Comment is incorrect - the code is testing GetString() method, not DateTime parsing. The comment should reflect that this handles invalid surrogate characters in strings.
| // If the string is not a valid DateTime, we expect an exception | |
| // If the string contains invalid surrogate characters, we expect an exception |
| if (spanArgumentEx?.Message.Equals(seqArgumentEx?.Message) is not true) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"Span and Sequence readers diverged in exponent exception:{Environment.NewLine}" + |
There was a problem hiding this comment.
Error message incorrectly refers to 'exponent exception' when handling 'same key' ArgumentException. Should be 'same key exception' or similar.
| $"Span and Sequence readers diverged in exponent exception:{Environment.NewLine}" + | |
| $"Span and Sequence readers diverged in same key exception:{Environment.NewLine}" + |
| if (CompareExceptions(ex, ex)) | ||
| { | ||
| return default!; | ||
| } |
There was a problem hiding this comment.
Comparing the same exception with itself (ex, ex) will always return true for matching exceptions. This should likely compare different exceptions or handle the case differently.
| if (CompareExceptions(ex, ex)) | |
| { | |
| return default!; | |
| } | |
| // Handle the exception or log it as needed | |
| // For now, rethrowing the exception to ensure proper handling | |
| throw new InvalidOperationException("An error occurred during token comparison.", ex); |
| //Debugger.Launch(); | ||
|
|
There was a problem hiding this comment.
| //Debugger.Launch(); |
| // Create a random seed from the last 4 bytes of the input | ||
| int len = bytes.Length; | ||
| int randomSeed = bytes[len - 1] + (bytes[len - 2] << 8) + (bytes[len - 3] << 16) + (bytes[len - 4] << 24); | ||
| s_random = new Random(randomSeed); |
There was a problem hiding this comment.
Interesting technique. I wonder if passing inputs through our own pseudorandom generator makes it more difficult for the fuzzing engine to guide code coverage. @MihaZupan is this considered good practice?
There was a problem hiding this comment.
I won't pretend to understand how exactly it'd affect input generation, but I would expect it to struggle more, especially since the random generator itself isn't being instrumented.
AFAICT, you're only using a couple of random bytes per invocation, so it might be viable to interpret the first N bytes as the random data instead of as a seed.
| static readonly Type[] groundTypes = [ | ||
| typeof(int), | ||
| typeof(string), | ||
| typeof(decimal), |
There was a problem hiding this comment.
Consider adding the following types for better variety: double, long, Int128, char.
| ]; | ||
|
|
||
| void TestRandomPoco(ReadOnlySpan<byte> bytes, ReadOnlySequence<byte> sequence, JsonSerializerOptions options) | ||
| { |
There was a problem hiding this comment.
Consider constructing random types recursively: each node can be randomly chosen to either be a ground type, a record, a poco with setters, a dictionary or list type. This should ensure maximal coverage for all built-in converters and JSON shapes. You can avoid extremely deep type graphs or stack overflows by imposing a maximum depth or randomly generated size.
3 participants
No description provided.