Fixes System.Drawing.Image.FromStream(...) throws "Parameter is not valid." when Stream.Read does not return full file length

[ricardobossan]

Fixes #14064

Proposed changes

• Fix ComManagedStream.Read (the COM IStream wrapper used when a managed Stream is handed to GDI+) to keep reading until the requested buffer is filled or the stream reaches EOF, instead of issuing a single Stream.Read.
• This repairs Image.FromStream / Bitmap.FromStream / new Bitmap(stream) / Metafile(stream) for any stream that legally returns fewer bytes than requested (chunked / network / zip-wrapped), which GDI+ was rejecting with Parameter is not valid.
• Add regression tests for the short-read case and the read-past-EOF case.
• Loading images through System.Drawing from a stream that returns partial reads now succeeds instead of throwing ArgumentException: Parameter is not valid. (Also reported in System.Drawing.Bitmap.FromStream fails when called with ZipWrappingStream #8824).
• No change for streams that already returned full reads (FileStream, MemoryStream, ...); those keep working exactly as before.

Regression?

• No. The single-read behavior was ported verbatim from the legacy GPStream (git blame) and predates that; System.Drawing.Bitmap.FromStream fails when called with ZipWrappingStream #8824 shows the same failure on .NET Core 3.1 / System.Drawing.Common 4.7.0.

Risk

• Low. The change is confined to one method and only adds a fill loop: it returns identical results for full-read streams and the correct total for short-read streams. The COM IStream::Read contract explicitly allows satisfying a request across multiple source reads.

Screenshots

Before

After

Test methodology

• New unit tests in ComManagedStreamTests call IStream::Read directly against a stream that caps every read: one asserts the buffer is filled across chunks, the other asserts a request past EOF returns only the available bytes without throwing.
• Manual end-to-end check using the attached Program.cs (extract and paste it into src/test/integration/ScratchProject/Program.cs and run): it decodes an in-memory PNG through a 32-byte-per-read stream via Image.FromStream and shows the loaded image. Throws Parameter is not valid. before the fix, loads after.
• Full System.Private.Windows.Core.Tests suite.

Test environment(s)

• 11.0.100-preview.5.26302.115

Microsoft Reviewers: Open in CodeFlow

[Copilot]

Pull request overview

This PR fixes System.Drawing stream decoding failures by improving the COM IStream wrapper (ComManagedStream) so it can satisfy a single read request across multiple underlying Stream.Read calls (handling “short reads” correctly), and adds regression tests to cover the scenario.

Changes:

• Update ComManagedStream’s ISequentialStream::Read implementation to keep reading until the requested buffer is filled or EOF is reached.
• Add unit tests that exercise short-read streams and reads that request more bytes than remain.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/System.Private.Windows.Core/src/Windows/Win32/System/Com/ComManagedStream.cs	Implements a fill-loop in Read to handle short reads from managed streams passed to native consumers (e.g., GDI+).
src/System.Private.Windows.Core/tests/System.Private.Windows.Core.Tests/Windows/Win32/System/Com/ComManagedStreamTests.cs	Adds regression coverage for chunked/short-read behavior and read-past-EOF behavior via direct IStream::Read calls.

Comments suppressed due to low confidence (1)

src/System.Private.Windows.Core/src/Windows/Win32/System/Com/ComManagedStream.cs:143

• ISequentialStream::Read should return S_FALSE when fewer than cb bytes are read due to end-of-stream. Returning S_OK even on short reads diverges from the COM contract and makes it harder for callers to distinguish a complete read from EOF.

        if (pcbRead is not null)
            *pcbRead = (uint)read;

        return HRESULT.S_OK;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[LeafShi1]

Behavioral note: this change makes ComManagedStream.Read block until the full cb bytes are available or EOF is reached. This wrapper backs all IStream consumers (clipboard, drag/drop, OLE, GDI+), not just image decoding, so for a blocking/network-backed stream a caller that previously received a prompt short read will now be held until the entire request can be satisfied. This is the correct fix for GDI+, but worth confirming it's acceptable for the other callers.

[LeafShi1]

Per the ISequentialStream::Read / IStream::Read contract, when fewer bytes than cb are returned because the end of the stream was reached, the method should return S_FALSE (with *pcbRead < cb) rather than S_OK. Now that this method deliberately reads until the buffer is full or EOF, a short result is a meaningful EOF signal. Since the whole point of this fix is that some COM callers are strict about read results, a strict caller that inspects the HRESULT (rather than just pcbRead) could still misbehave. Consider return read < buffer.Length ? HRESULT.S_FALSE : HRESULT.S_OK;. (The previous single-read code also returned S_OK, so this is pre-existing, but the fix makes it more relevant.)

[LeafShi1]

Unlike Read_StreamReturnsShortReads_FillsBuffer (which asserts destinationBuffer.Should().Equal(sourceBytes)), this test only verifies the returned byte count. Consider also asserting that the first sourceBytes.Length bytes of destinationBuffer equal sourceBytes, so the test confirms the data is correctly assembled across chunks up to EOF, not just that the count is right.