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.

Sign up for GitHub

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

Jump to bottom

Add option for truncated stream detection #75671

Merged
merged 10 commits into from
Nov 2, 2022
Merged

Add option for truncated stream detection #75671

merged 10 commits into from
Nov 2, 2022

Conversation

mfkl
Copy link
Contributor

@mfkl mfkl commented Sep 15, 2022

Having another go at #47563 (previous #61768)

This time using an opt-in flag to avoid breakage like this #72726.

/cc @jozkee

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 15, 2022
@ghost
Copy link

ghost commented Sep 15, 2022

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

Having another go at #47563 (previous #61768)

This time using an opt-in flag to avoid breakage like this #72726.

/cc @jozkee

Author: mfkl
Assignees: -
Labels:

area-System.IO.Compression
Milestone: -

stephentoub
stephentoub reviewed Sep 15, 2022
View reviewed changes
src/libraries/Common/tests/System/IO/Compression/CompressionStreamUnitTestBase.cs Outdated
@@ -479,6 +478,8 @@ async Task<long> GetLengthAsync(CompressionLevel compressionLevel)
[InlineData(TestScenario.ReadByteAsync)]
public async Task StreamTruncation_IsDetected(TestScenario scenario)
{
AppContext.SetSwitch("System.IO.Compression.UseStrictValidation", true);
Copy link
Member

Choose a reason for hiding this comment

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

This affects all tests process-wide. Any test that changes such a process-wide switch needs to use RemoteExecutor to move that code out of process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done.

Copy link
Member

Choose a reason for hiding this comment

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

Another option is to create a second test project that enables the appcontext switch and runs all tests of the "normal" project with the switch on.

Copy link
Member

@jozkee jozkee Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

And a third option could be to turn the switch on for all the tests, we already know our tests will pass with the "strict validation" in place.

I do prefer this option over the others but let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a third option could be to turn the switch on for all the tests, we already know our tests will pass with the "strict validation" in place.

Not anymore, this one will fail

runtime/src/libraries/System.IO.Compression/tests/CompressionStreamUnitTests.Deflate.cs

Line 78 in eead714

public void CompressorNotClosed_DecompressorStillSuccessful(bool closeCompressorBeforeDecompression)

Another option is to create a second test project that enables the appcontext switch and runs all tests of the "normal" project with the switch on.

I prefer the current way (with the RemoteExecutor) but no strong feelings. Let me know.

mfkl added 4 commits September 16, 2022 14:34
@mfkl
Use RemoteExecutor
fa4b613 
move the test to concrete classes as abstracted classes are not supported by RemoteExecutor
@mfkl
review feedback
c7be15d
@mfkl
use same error text message
ab41e46
@mfkl
cache appcontext getswitch
1d5bc2a
stephentoub
stephentoub reviewed Sep 19, 2022
View reviewed changes
src/libraries/Common/tests/System/IO/Compression/ZipTestHelper.cs Outdated
break;

ac = await ast.ReadAtLeastAsync(ad, 4096);
bc = await bst.ReadAtLeastAsync(bd, 4096);
Copy link
Member

Choose a reason for hiding this comment

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

Presumably you want to pass throwOnEndOfStream:false for both of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.. thanks.

stephentoub
stephentoub reviewed Sep 19, 2022
View reviewed changes
src/libraries/Common/tests/System/IO/Compression/ZipTestHelper.cs Outdated
bd = NormalizeLineEndings(bd);
}

AssertExtensions.SequenceEqual(ad, bd);
Copy link
Member

Choose a reason for hiding this comment

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

And these should be sliced, e.g. ad.AsSpan(0, ac), bd.AsSpan(0, bc).

If this is just copying what's already there for the sync implementation, then maybe inputs just aren't triggering the faulty behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sync implementation is also not triggering any exception (when enabling strict validation) in the current version of the code. If I recall correctly I added this async version to cover the async code path while putting together the previous PR.

But since it appears to not trigger the truncated exception (anymore), it may be outside the scope of this PR to improve the test coverage for the async code path.

stephentoub
stephentoub reviewed Sep 19, 2022
View reviewed changes
...raries/System.IO.Compression.Brotli/src/System/IO/Compression/dec/BrotliStream.Decompress.cs Outdated
@@ -227,9 +234,15 @@ private bool TryDecompress(Span<byte> destination, out int bytesWritten)
return false;
}

private static readonly bool UseStrictValidation =
Copy link
Member

Choose a reason for hiding this comment

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

UseStrictValidation => s_useStrictValidation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@mfkl
Copy link
Contributor Author

mfkl commented Sep 29, 2022

CI is fixed.

Feel free to let me know what you prefer regarding #75671 (comment) and if anyone has any other feedback regarding this PR.

@jozkee jozkee added the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 10, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 13, 2022
@mfkl
Copy link
Contributor Author

mfkl commented Oct 31, 2022

I believe I've addressed most concerns. Do you have any more feedback on this PR?

jozkee
jozkee approved these changes Oct 31, 2022
View reviewed changes
Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

CI issue looks unrelated to this PR.

@jozkee jozkee merged commit f43d5e3 into dotnet:main Nov 2, 2022
@adamsitnik adamsitnik added this to the 8.0.0 milestone Nov 2, 2022
@mfkl mfkl deleted the strict-validation branch November 7, 2022 07:46
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@jozkee jozkee jozkee approved these changes

Assignees
No one assigned
Labels
area-System.IO.Compression community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mfkl @stephentoub @jozkee @adamsitnik