Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,20 @@ HRESULT ISequentialStream.Interface.Read(void* pv, uint cb, uint* pcbRead)
ActualizeVirtualPosition();

Span<byte> buffer = new(pv, checked((int)cb));
int read = _dataStream.Read(buffer);

Comment on lines 122 to +125
// Stream.Read can legally return fewer bytes than requested (e.g. chunked/network streams).
// Some callers (GDI+) treat a short read as failure, so fill the buffer until EOF.
int read = 0;
while (read < buffer.Length)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

{
int bytesRead = _dataStream.Read(buffer[read..]);
if (bytesRead == 0)
{
break;
}

read += bytesRead;
}

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Windows.Win32.Foundation;

namespace Windows.Win32.System.Com.Tests;

public class ComManagedStreamTests
Expand All @@ -22,6 +24,54 @@ public void Ctor_SeekableStream_UsesOriginalStream()
comManagedStream.GetDataStream().Should().BeSameAs(seekableStream);
}

[Fact]
public unsafe void Read_StreamReturnsShortReads_FillsBuffer()
{
// Streams are allowed to return fewer bytes than requested. Simulate a chunked/network stream
// and verify the wrapper keeps reading until the requested buffer is filled. See issue #14064.
byte[] sourceBytes = new byte[1024];
for (int index = 0; index < sourceBytes.Length; index++)
{
sourceBytes[index] = (byte)index;
}

using ChunkingStream shortReadStream = new(sourceBytes, chunkSize: 100);
ComManagedStream comManagedStream = new(shortReadStream);

byte[] destinationBuffer = new byte[sourceBytes.Length];
uint bytesReadCount;
fixed (byte* destinationPointer = destinationBuffer)
{
((IStream.Interface)comManagedStream).Read(destinationPointer, (uint)destinationBuffer.Length, &bytesReadCount).Should().Be(HRESULT.S_OK);
}

bytesReadCount.Should().Be((uint)sourceBytes.Length);
destinationBuffer.Should().Equal(sourceBytes);
}

[Fact]
public unsafe void Read_RequestPastEndOfStream_ReturnsOnlyAvailableBytes()
{
// When more bytes are requested than remain, only the available bytes are returned (no throw on EOF).
byte[] sourceBytes = new byte[50];
for (int index = 0; index < sourceBytes.Length; index++)
{
sourceBytes[index] = (byte)index;
}

using ChunkingStream shortReadStream = new(sourceBytes, chunkSize: 10);
ComManagedStream comManagedStream = new(shortReadStream);

byte[] destinationBuffer = new byte[sourceBytes.Length * 2];
uint bytesReadCount;
fixed (byte* destinationPointer = destinationBuffer)
{
((IStream.Interface)comManagedStream).Read(destinationPointer, (uint)destinationBuffer.Length, &bytesReadCount).Should().Be(HRESULT.S_OK);
}
Comment on lines +67 to +70

bytesReadCount.Should().Be((uint)sourceBytes.Length);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

}

private class TestStream : MemoryStream
{
private readonly bool _canSeek;
Expand All @@ -33,4 +83,60 @@ public TestStream(bool canSeek, int numBytes) : base(new byte[numBytes])
_canSeek = canSeek;
}
}

// Seekable stream that never returns more than a fixed chunk per Read, simulating chunked/network streams.
// Derives from Stream (not MemoryStream) so the span-based Read routes through this Read override on all targets.
private sealed class ChunkingStream : Stream
{
private readonly byte[] _sourceData;
private readonly int _maxBytesPerRead;
private int _position;

public ChunkingStream(byte[] sourceData, int chunkSize)
{
_sourceData = sourceData;
_maxBytesPerRead = chunkSize;
}

public override bool CanRead => true;
public override bool CanSeek => true;
public override bool CanWrite => false;
public override long Length => _sourceData.Length;

public override long Position
{
get => _position;
set => _position = (int)value;
}

public override int Read(byte[] buffer, int offset, int count)
{
int bytesToRead = Math.Min(Math.Min(count, _maxBytesPerRead), _sourceData.Length - _position);
if (bytesToRead <= 0)
{
return 0;
}

Array.Copy(_sourceData, _position, buffer, offset, bytesToRead);
_position += bytesToRead;
return bytesToRead;
}

public override long Seek(long offset, SeekOrigin origin)
{
_position = origin switch
{
SeekOrigin.Begin => (int)offset,
SeekOrigin.Current => _position + (int)offset,
SeekOrigin.End => _sourceData.Length + (int)offset,
_ => _position,
};

return _position;
}

public override void Flush() { }
public override void SetLength(long value) => throw new NotSupportedException();
public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException();
}
}
Loading