ZipArchive CreateMode tries to read Position on non-seekable Stream #11497

Closed
petertiedemann opened this Issue Sep 7, 2016 · 4 comments

Projects

None yet

6 participants

@petertiedemann

Looking at https://msdn.microsoft.com/en-us/library/hh158263(v=vs.110).aspx i find this remark:

If the mode parameter is set to Read, the stream must support reading. If the mode parameter is set to Create, the stream must support writing. If the mode parameter is set to Update, the stream must support reading, writing, and seeking.

Based on that i would conclude that using CreateMode, i should be able to write to a non-seekable stream. The corefx tests for Create also seems to indicate that this is the intention at first glance.

However, even in CreateMode, the code attempts to read the Position from the stream it is writing to, unless i am mistaken that is typically not supported on a non-seekable stream?

In the abovementioned test a test WrappedStream class is used that can be configured as non-seekable. However, it does support getting a Position which i find a bit strange.

I can see that there is also a closed issue on Connect:
https://connect.microsoft.com/VisualStudio/feedback/details/816411/ziparchive-shouldnt-read-the-position-of-non-seekable-streams

Below is an example that will throw NotSupportedException:

using System;
using System.IO;
using System.IO.Compression;

namespace SeekeableStreamBug {
  public class NonSeekableMemoryStream: MemoryStream {
    public override bool CanSeek => false;

    public override long Position {
      get { throw new NotSupportedException(); }
      set { throw new NotSupportedException(); }
    }
  }

  public class Program {
    public static void Main( string[] args ) {
      using ( ZipArchive archive = new ZipArchive( new NonSeekableMemoryStream(), ZipArchiveMode.Create, false ) ) {
        var entry = archive.CreateEntry( "foo" );
        using ( var es = entry.Open() ) {
          es.Write( new byte[] { 4, 2 }, 0, 2 );
        }
      }
    }
  }
}

@svick
Contributor
svick commented Sep 7, 2016

unless i am mistaken that is typically not supported on a non-seekable stream

Yes, that's the core of the problem. The MSDN documentation for Stream.Position says:

The stream must support seeking to get or set the position. Use the CanSeek property to determine whether the stream supports seeking.

To me, this says that streams that don't support seeking shouldn't implement Position. But ZipArchive just assumes that any stream used with the Create mode does support Position.

Since ZipArchive in Create mode only reads Position, I think the fix is to make ZipArchive track Position on its own, either always, or only when the underlying Stream's CanSeek returns false.


A workaround for this is to wrap the Stream in a Stream that tracks the position, like the one posted in my answer on SO.

@stephentoub
Member

To me, this says that streams that don't support seeking shouldn't implement Position

Totally as an aside from the main thread of discussion here, I don't think that's exactly what the docs mean, or at least not what they were intended to mean. What I believe they intend to say is that as a consumer of a stream, you shouldn't expect Position to be implemented if !CanSeek. As the implementer of a Stream, though, you're not required to throw NotSupportedException if !CanSeek... you could provide a valid non-throwing implementation of Position, but you shouldn't expect any consumers using the type polymorphically via the base Stream to use your implementation, given they can't expect it to be valid.

@ianhays
Member
ianhays commented Oct 10, 2016

I think the fix is to make ZipArchive track Position on its own ... only when the underlying Stream's CanSeek returns false.

Seems reasonable to me assuming it doesn't incur any unexpected perf costs. Marked as up for grabs.

@ianhays ianhays added this to the Future milestone Oct 10, 2016
@StephenCleary StephenCleary added a commit to StephenCleary/corefx that referenced this issue Oct 15, 2016
@StephenCleary StephenCleary Update WrappedStream (test utility class) to disallow reading Positio…
…n for non-seekable streams.

This causes test failure due to issue #11497.
631ab89
@karelz karelz self-assigned this Oct 15, 2016
@karelz
Member
karelz commented Oct 15, 2016

@StephenCleary works on it ...

@ianhays ianhays closed this in 46e2bf6 Oct 18, 2016
@karelz karelz assigned StephenCleary and unassigned karelz Oct 18, 2016
@karelz karelz modified the milestone: Future, 1.2.0 Dec 3, 2016
@StephenCleary StephenCleary was unassigned by karelz Dec 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment