-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix memorystream local test failure #123709
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-io |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a test failure in MemoryStream_CapacityBoundaryChecks by adding explicit validation to throw ArgumentOutOfRangeException when setting the Capacity property to a value exceeding Array.MaxLength. Previously, attempting to set an invalid capacity would result in an OutOfMemoryException during array allocation, but the test expected ArgumentOutOfRangeException.
Changes:
- Added validation in the
Capacitysetter to throwArgumentOutOfRangeExceptionbefore attempting array allocation when the capacity exceeds the maximum allowed array length
src/libraries/System.Private.CoreLib/src/System/IO/MemoryStream.cs
Outdated
Show resolved
Hide resolved
…m.cs Co-authored-by: Copilot <[email protected]>
| if (value < Length) | ||
| throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_SmallCapacity); | ||
| if (value > MemStreamMaxLength) | ||
| throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_Capacity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this always resulted in an OOMException, even on .NET Framework? Or did it used to throw an ArgumentException for this, that regressed, and now this is just fixing the regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it results in an OutOfMemoryException because it tries to allocate a capacity larger than what’s possible. There wasn’t a guard before, so it would fail during the allocation rather than throwing an argument exception here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An OOM seems appropriate to me, or at least more consistent, given that even smaller values could produce an OOMException when trying to allocate the array, and there are many other places we allocate arrays without checking against Array.MaxValue. Without a strong reason to make a breaking change here, I don't think we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub Position, Seek and SetLength already do a MemStreamMaxLength check to throw AOOREx, we aligned the ctor with those in https://github.com/dotnet/runtime/pull/119089/changes#r2319364701 and this is now aligning Capacity, which we missed in the previous PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we shouldn't have done that? This got in in .NET 11 so we could backpedal if we think that's appropriate.
| @@ -263,6 +263,8 @@ public virtual int Capacity | |||
| // Special behavior if the MS isn't expandable: we don't throw if value is the same as the current capacity | |||
| if (value < Length) | |||
| throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_SmallCapacity); | |||
| if (value > MemStreamMaxLength) | |||
| throw new ArgumentOutOfRangeException(nameof(value), SR.ArgumentOutOfRange_Capacity); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use ArgumentOutOfRange_Capacity anywhere else in MemoryStream and what we use in other places e.g. ArgumentOutOfRange_StreamLength, includes the maximum array length:
Stream length must be non-negative and less than the maximum array length {0} - origin.
Should we add a new string mentioning that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I maybe just use a similar check to here instead of adding a new error string?
| ArgumentOutOfRangeException.ThrowIfGreaterThan(capacity, MemStreamMaxLength); |
Follow up for #123440