Revisit Seekability of writable zip streams

[weltkante]

In PR #1311 (issue #585) two writable streams (Certificate.cs and XmlDigitalSignatureProcessor.cs) get wrapped in a memory stream by calling the new method GetSeekableStream(FileMode.Create, FileAccess.Write). It is unclear from the PR source how the written data will be propagated to the actual zip stream. Since this was noticed after the PR merged I'm opening a separate issue to revisit the handling of writable streams.

I'd also argue that the current implementation of GetSeekableStream should not have had FileMode and FileAccess paramters, since the only case which is handled correctly is the open/read combination.

I'd suggest either providing a dedicated stream wrapper class and keeping FileMode/FileAccess, or remove FileMode/FileAccess and provide a separate code path for callers who want to write data.

Related: issue #1363 apparently still has call sites which do not get wrapped

[rladuca]

@weltkante

This is not an issue as PackagePart.GetStream will return seekable streams when the stream is writeable.
The extension method GetSeekableStream already handles this case and will merely forward a seekable stream upward to the caller.

This is the reason that the workaround is to open your package as writeable.

[weltkante]

I see, though #1363 still produced a non seekable stream for writing, so I worry that it can also happen in GetSeekableStream and pass through undetected, silently dropping data. This worry might be unfounded since we don't yet know the root cause of #1363 but it might be worth adding checks to ensure that GetSeekableStream never creates a MemoryStream wrapper when opening something as writable. Maybe throw an exception if this unexpected case ever happens, so that instead of silently dropping the data there is something to diagnose.

Your choice, if you think your tests cover this well enough to detect it if that ever regresses it might not be necessary.

[rladuca]

I am entirely sympathetic to the more cautious approach. I'm going to root cause #1363 and figure out where that sits in relation to the changes.

I think your suggestion on testing write vs. seekability is a reasonable one and I'll leave this open as a conduit for potentially changing it.

[rladuca]

@weltkante Be aware that I am not assigning this just yet, I'll let our normal triage process pick it up since by then I'll have more info.