Skip to content

Conversation

@gfoidl
Copy link
Owner

@gfoidl gfoidl commented Dec 20, 2022

Introduced

public enum WhitespaceMode
{
/// <summary>
/// Treats whitspace as invalid input.
/// </summary>
None = 0,
/// <summary>
/// Treats whitespace as valid input -- i.e. it's ignored / skipped over.
/// </summary>
Allow = 1
}
for decoding APIs.


  • Update / validate if fuzzing is still OK

@gfoidl
Copy link
Owner Author

gfoidl commented Dec 22, 2022

/azp run Base64-Fuzz

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gfoidl gfoidl mentioned this pull request Dec 22, 2022
@gfoidl
Copy link
Owner Author

gfoidl commented Dec 22, 2022

Fuzz ran for over an hour w/o finding something. I'm going to merge, so that regular fuzz-run can detect something (or hopefully not).

@gfoidl gfoidl merged commit 063507b into master Dec 22, 2022
@gfoidl gfoidl deleted the allow_whitespace branch December 22, 2022 21:49
}
}
//-------------------------------------------------------------------------
private static OperationStatus DecodeWithWithespaceBlockwise<T, TOperation>(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace is misspelled here

@heathbm
Copy link

heathbm commented Dec 23, 2022

I ran this code against the existing tests in: https://github.com/dotnet/runtime/blob/8d3070f90c557b920b0e53489f617ec55b186eed/src/libraries/System.Memory/tests/Base64/Base64DecoderUnitTests.cs

The DecodingInvalidBytesPadding test is failing here: https://github.com/dotnet/runtime/blob/8d3070f90c557b920b0e53489f617ec55b186eed/src/libraries/System.Memory/tests/Base64/Base64DecoderUnitTests.cs#L373

I believe there may be an issue with the way consumed bytes are being handled, according to the definition of consumed: "The number of input bytes consumed during the operation. This can be used to slice the input for subsequent calls, if necessary."
Take the simplest example, an empty string " ", we would expect consumed = 0, however, we are getting 1.
Also, any whitespace after valid bytes should not be included in consumed, such that, "AQ== " consumed would be 4 and not 5.

@gfoidl
Copy link
Owner Author

gfoidl commented Dec 23, 2022

"The number of input bytes consumed during the operation. This can be used to slice the input for subsequent calls, if necessary."
...
any whitespace after valid bytes should not be included in consumed, such that, "AQ== " consumed would be 4 and not 5.

Looks like we have different definitions for "consumed". For me in the case of "AQ== " it should be 5. Rationale is that the decoding operation actually consumed it and to accord for "used to slice the input for subsequent calls".
If it would be 4, then after slicing " " would remain, so input != Empty and another call may be done, with " ".
When decoding is called with isFinalBlock: true and whitespace is allowed, then the whole input should be consumed, so that input == Empty if the operation succeeded. If there's anything left in the input, then it's strange -- think of Debug.Assert(consumed == base64Input.Length).

For the empty string "" it's 0, as there's nothing to do.
For the single-space string " " (which is not empty) it's consumed 1, and written 0.

If whitespace is to be tolerated, then it should account as consumed too.

@gfoidl
Copy link
Owner Author

gfoidl commented Dec 23, 2022

If whitespace is to be tolerated, then it should account as consumed too.

To be strict maybe there should be two counters: consumed and consumedBase64Bytes.
When whitespace isn't allowed both have the same value. With allowed whitespace it's Debug.Assert(consumed >= consumedBase64Bytes).

At the moment I don't see need to expose that additional counter. For usage all that's important is how many bytes/chars are consumed from the base64-input, and how many bytes are written to the data-output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants