Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Position and Seek for MemoryStream with custom origin #88572

Merged
merged 3 commits into from
Aug 21, 2023
Merged

Fix Position and Seek for MemoryStream with custom origin #88572

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d07b344
Fix Position and Seek for MemoryStream with custom origin
manandre Jul 9, 2023
6880c4d
Run test on 64bit platforms only
manandre Jul 10, 2023
23e33c7
Add tests for extreme long overflow
manandre Aug 18, 2023
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
4 changes: 0 additions & 4 deletions src/libraries/Common/tests/StreamConformanceTests/System/IO/StreamConformanceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,6 @@ public static IEnumerable<object[]> AllSeekModes() =>
from mode in Enum.GetValues<SeekMode>()
select new object[] { mode };

public static IEnumerable<object[]> AllSeekModesAndValue(object value) =>
from mode in Enum.GetValues<SeekMode>()
select new object[] { mode, value };

public static async Task<int> ReadAsync(ReadWriteMode mode, Stream stream, byte[] buffer, int offset, int count, CancellationToken cancellationToken = default)
{
if (mode == ReadWriteMode.SyncByte)
Expand Down
30 changes: 30 additions & 0 deletions src/libraries/System.IO/tests/MemoryStream/MemoryStreamTests.cs
View file
Open in desktop
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 System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Xunit;
Expand Down Expand Up @@ -82,6 +84,34 @@ public static void MemoryStream_WriteToTests_Negative()
}
}

public static IEnumerable<object[]> MemoryStream_PositionOverflow_Throws_MemberData() =>
from mode in Enum.GetValues<SeekMode>()
from bufferContext in
new (int bufferSize, int origin)[]
{
(0, 0),
(1, 0),
(1, 1),
(10, 0),
(10, 5),
(10, 10),
(Array.MaxLength, 0),
(Array.MaxLength, Array.MaxLength)
Copy link
Member

Choose a reason for hiding this comment

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

these last two cases are causing OOM on 32 bit

https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-88572-merge-044351d2f9494dfeb8/System.IO.Tests/1/console.f70c368a.log?helixlogtype=result

}
select new object[] {mode, bufferContext.bufferSize, bufferContext.origin};

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))]
[MemberData(nameof(MemoryStream_PositionOverflow_Throws_MemberData))]
public void MemoryStream_SeekOverflow_Throws(SeekMode mode, int bufferSize, int origin)
{
byte[] buffer = new byte[bufferSize];
using (MemoryStream ms = new MemoryStream(buffer, origin, buffer.Length - origin, true))
{
Seek(mode, ms, int.MaxValue - origin);
Assert.Throws<ArgumentOutOfRangeException>(() => Seek(mode, ms, (long)int.MaxValue - origin + 1));
Copy link
Member

@jozkee jozkee Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Can you please test with an offset of long.MinValue + 1 (uncasted). The check we discussed in https://github.com/dotnet/runtime/pull/88572/files/6880c4d6d148f46a878f929303fd2472e8fbc363#r1297747637 may be there to prevent an overflow after the (int) conversion.

Copy link

@elgonzo elgonzo Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, removing the unchecked(loc + offset) < _origin check was a mistake. Because (int)offset used for calculating tempPosition will just cut the upper 32 bit from the long value, turning certain negative offset values into positive tempPosition values that are equal to or larger than _origin, thus not correctly failing on offset values representing invalid negative positions. For example, with this change, calling Seek(long.MinValue, SeekOrigin.Begin) would lead to tempPosition being equal to the value of _origin and effectively set the stream position to zero instead of throwing, if i am not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added two Seek calls with long.MinValue + 1 and long.MaxValue - 1 as offset and checked that an exception is correctly raised in all cases.
Indeed I have manually checked that these calls are not always sending an exception when the unchecked(loc + offset) < _origin check is not present.

}
}
jozkee marked this conversation as resolved.
Show resolved Hide resolved

[Fact]
public void DerivedMemoryStream_ReadWriteSpanCalled_ReadWriteArrayUsed()
{
Expand Down
53 changes: 19 additions & 34 deletions src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ public override long Position
ArgumentOutOfRangeException.ThrowIfNegative(value);
EnsureNotClosed();

if (value > MemStreamMaxLength)
if (value > MemStreamMaxLength - _origin)
throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_StreamLength);
_position = _origin + (int)value;
}
Expand Down Expand Up @@ -512,41 +512,26 @@ public override long Seek(long offset, SeekOrigin loc)
{
EnsureNotClosed();

if (offset > MemStreamMaxLength)
throw new ArgumentOutOfRangeException(nameof(offset), SR.ArgumentOutOfRange_StreamLength);

switch (loc)
return SeekCore(offset, loc switch
{
case SeekOrigin.Begin:
{
int tempPosition = unchecked(_origin + (int)offset);
if (offset < 0 || tempPosition < _origin)
throw new IOException(SR.IO_SeekBeforeBegin);
_position = tempPosition;
break;
}
case SeekOrigin.Current:
{
int tempPosition = unchecked(_position + (int)offset);
if (unchecked(_position + offset) < _origin || tempPosition < _origin)
throw new IOException(SR.IO_SeekBeforeBegin);
_position = tempPosition;
break;
}
case SeekOrigin.End:
{
int tempPosition = unchecked(_length + (int)offset);
if (unchecked(_length + offset) < _origin || tempPosition < _origin)
throw new IOException(SR.IO_SeekBeforeBegin);
_position = tempPosition;
break;
}
default:
throw new ArgumentException(SR.Argument_InvalidSeekOrigin);
}
SeekOrigin.Begin => _origin,
SeekOrigin.Current => _position,
SeekOrigin.End => _length,
_ => throw new ArgumentException(SR.Argument_InvalidSeekOrigin)
});
}

Debug.Assert(_position >= 0, "_position >= 0");
return _position;
private long SeekCore(long offset, int loc)
{
if (offset > MemStreamMaxLength - loc)
throw new ArgumentOutOfRangeException(nameof(offset), SR.ArgumentOutOfRange_StreamLength);
int tempPosition = unchecked(loc + (int)offset);
if (unchecked(loc + offset) < _origin || tempPosition < _origin)
jozkee marked this conversation as resolved.
Show resolved Hide resolved
throw new IOException(SR.IO_SeekBeforeBegin);
_position = tempPosition;

Debug.Assert(_position >= _origin, "_position >= _origin");
return _position - _origin;
}

// Sets the length of the stream to a given value. The new
Expand Down