Skip to content
New issue

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

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

Already on GitHub? # to your account

Skip encoding mixing test in fuzzer when SkipValidation=true #111596

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PranavSenthilnathan
Copy link
Member

Mixing encodings will not throw when SkipValidation = true, so there's no need to validate that it does.

Closes #111588

@MihaZupan
Copy link
Member

@MihuBot fuzz JsonWriter

@eiriktsarpalis
Copy link
Member

Maybe I'm misunderstanding what this fix is doing, but the error in #111588 suggests to me that it's a misconfiguration issue, i.e. passing a non-expandable MemoryStream to a Utf8JsonWriter which is incorrect regardless of what inputs are being passed to the writer.

@PranavSenthilnathan
Copy link
Member Author

The test is writing 2 segments of different encodings and asserting it throws. But in addition, it only provides a memory stream large enough only for the first segment, to validate that none of the second segment is written. In the fuzzer error, because validation is skipped, the second segment will be written anyway which exceeds the memory stream length.

@eiriktsarpalis
Copy link
Member

Couldn't the fuzzer check that the total length is below the target threshold after writing has completed? The error message being surfaced right now seems a bit misleading to me.

@PranavSenthilnathan
Copy link
Member Author

passing a non-expandable MemoryStream to a Utf8JsonWriter which is incorrect regardless of what inputs are being passed to the writer.

My understanding is that if the serialized output length is less than or equal to the non-expandable stream capacity then serialization should not fail. Is that not a guarantee we offer? This is a stricter condition than checks on BytesCommitted/Pending - for example, if the writer reports the expected BytesCommitted value but still writes more than it should then the test won't catch it.

@eiriktsarpalis
Copy link
Member

I wouldn't disagree, but it just feels like an inappropriate way to enforce it. If we keep it like this I would prefer it if we inherited from MemoryStream and made sure a more appropriate error message was being surfaced.

@MihaZupan
Copy link
Member

Should this one be merged?

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fuzzing] Utf8JsonWriterFuzzer: Memory stream is not expandable.
3 participants