Fix for ZipArchive depending on Position for Create streams by StephenCleary · Pull Request #12682 · dotnet/corefx

StephenCleary · 2016-10-15T14:57:11Z

This is a fix for #11497, using the stream wrapper technique suggested by @svick.

The stream wrapper is only applied when necessary (if the ZipArchive is opened in Create mode and the underlying stream does not support seeking).

The wrapper forwards all (non-reading) methods to its base stream, including asynchronous methods. This is important to prevent the wrapper from forcing synchrony.

The existing unit tests (once fixed) were sufficient to detect this bug.

…n for non-seekable streams. This causes test failure due to issue #11497.

dnfclas · 2016-10-15T14:57:14Z

Hi @StephenCleary, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

dnfclas · 2016-10-15T15:00:47Z

@StephenCleary, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

karelz · 2016-10-15T15:48:58Z

@ianhays can you please CR?

danmoseley · 2016-10-17T22:14:40Z

@ianhays PTAL

ianhays · 2016-10-17T22:51:13Z

@svick

Looks like goodness to me. I'm somewhat concerned by the overhead of creating the wrapper stream, but considering the old behavior I think it's admissible.

The existing unit tests (once fixed) were sufficient to detect this bug.

@StephenCleary what do you mean by this? We didn't have any tests failing because of this issue. It would be ideal to have a test added that fails with the old behavior but passes with the new behavior.

StephenCleary · 2016-10-17T23:53:56Z

I was also hesitant about the wrapper stream, but the alternative would be to have ZipArchive itself keep track of the position, which would require every helper method using Position to read the stream's position from ZipArchive instead of the stream. I rejected that approach because it's less maintainable - all future zip-related code would "just have to know" that reading stream position is done completely differently here. So I settled on the wrapper, keeping it small, and only using it when necessary.

The first checkin in this pull request fixes the unit tests so that they fail due to this bug. So there are now unit tests that will detect a regression.

danmoseley · 2016-10-18T02:47:42Z

@ianhays please hit merge if you're good..

ianhays · 2016-10-18T16:03:26Z

Thanks for the contribution @StephenCleary, I tend to agree on this being preferable to manually tracking the position. Merging.

stephentoub · 2016-10-18T16:35:21Z

src/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs

+
+        public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
+        {
+            _position += count;


What if the operation is canceled... won't the position then be incorrect?

Same comment/question would apply as well to exceptions in other operations.

Yes, but what would the correct position be in that case?

There's the same issue for when the synchronous Write flat out fails. I considered saying we should update the position after the underlying call, but we would have to do something special for the async Write for that.

but what would the correct position be in that case?

We can't know, but that also means we'd be reporting potentially incorrect results from Position, and I don't know enough about the codebase to know what that could mess up. I'm wondering if on cancellation/failure, we should set _position to a sentinel value (e.g. -1) and change get_Position to do if (_position < 0) throw ...; else return _position;. It may not be worth it if nothing bad would come of it, but we should understand it better.

I like that idea; it's the only approach that really makes sense.

It also takes this class a step closer to being a generic (read/write) position tracking stream wrapper, which may actually make sense to eventually expose (in a more general package).

stephentoub · 2016-10-18T16:37:25Z

src/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs

+
+namespace System.IO.Compression
+{
+    internal sealed class PositionPreservingWriteOnlyStreamWrapper : Stream


Nit: I'd drop the "Wrapper" suffix. There are a variety of streams that wrap other streams and that don't use a suffix like this, e.g. DeflateStream, BufferedStream, etc.

stephentoub · 2016-10-18T16:38:28Z

src/System.IO.Compression/src/System/IO/Compression/PositionPreservingWriteOnlyStreamWrapper.cs

+
+        public override void WriteByte(byte value)
+        {
+            _position += 1;


Nit: _position++

StephenCleary · 2016-10-25T20:25:51Z

Pardon my ignorance regarding pull requests, but I'd like to apply the suggestions from @stephentoub. Since this is already merged, that would have to be a new pull request, right?

ianhays · 2016-10-25T20:29:02Z

that would have to be a new pull request, right?

Correct.

jnm2 · 2016-11-11T14:53:16Z

I'm pinging here as well because communication on Connect is dodgy.
Can the .NET team confirm how soon this will make it into the full framework?

https://connect.microsoft.com/VisualStudio/feedback/details/816411/ziparchive-shouldnt-read-the-position-of-non-seekable-streams#commentContainer

Fix for ZipArchive depending on Position for Create streams Commit migrated from dotnet/corefx@e8c0e78

StephenCleary added 2 commits October 15, 2016 10:45

Update WrappedStream (test utility class) to disallow reading Positio…

631ab89

…n for non-seekable streams. This causes test failure due to issue #11497.

Introduce a stream wrapper with its own Position to fix #11497.

46e2bf6

dnfclas added the cla-required label Oct 15, 2016

dnfclas added cla-signed and removed cla-required labels Oct 15, 2016

karelz added the area-System.IO.Compression label Oct 15, 2016

karelz assigned StephenCleary and ianhays Oct 15, 2016

Fix position updates for asynchronous writes.

51a5e96

ianhays merged commit e8c0e78 into dotnet:master Oct 18, 2016

stephentoub reviewed Oct 18, 2016

View reviewed changes

karelz modified the milestone: 1.2.0 Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#12682 from StephenCleary/master

875afa4

Fix for ZipArchive depending on Position for Create streams Commit migrated from dotnet/corefx@e8c0e78

Conversation

StephenCleary commented Oct 15, 2016

Uh oh!

dnfclas commented Oct 15, 2016

Uh oh!

dnfclas commented Oct 15, 2016

Uh oh!

karelz commented Oct 15, 2016

Uh oh!

danmoseley commented Oct 17, 2016

Uh oh!

ianhays commented Oct 17, 2016

Uh oh!

StephenCleary commented Oct 17, 2016

Uh oh!

danmoseley commented Oct 18, 2016

Uh oh!

ianhays commented Oct 18, 2016

Uh oh!

stephentoub Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StephenCleary Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

ianhays Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

stephentoub Oct 22, 2016

Choose a reason for hiding this comment

Uh oh!

StephenCleary Oct 22, 2016

Choose a reason for hiding this comment

Uh oh!

stephentoub Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

stephentoub Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

StephenCleary commented Oct 25, 2016

Uh oh!

ianhays commented Oct 25, 2016

Uh oh!

jnm2 commented Nov 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stephentoub Oct 18, 2016 •

edited

Loading