Skip to content

Fix Position and Seek for MemoryStream with custom origin - #88572

Merged
jozkee merged 3 commits into
dotnet:mainfrom
manandre:memory-stream-seek-max-length
Aug 21, 2023
Merged

Fix Position and Seek for MemoryStream with custom origin#88572
jozkee merged 3 commits into
dotnet:mainfrom
manandre:memory-stream-seek-max-length

Conversation

@manandre

Copy link
Copy Markdown
Contributor

Fixes#88541

@ghostghost added area-System.IO community-contribution Indicates that the PR has been added by a community member labels Jul 10, 2023
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #88541

Author:manandre
Assignees:-
Labels:

area-System.IO

Milestone:-

(10, 5),
(10, 10),
(Array.MaxLength, 0),
(Array.MaxLength, Array.MaxLength)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@elgonzo

Copy link
Copy Markdown

Note that this will change the behavior of the MemoryStream.Property and will be in conflict with the current MemoryStream.Position documentation (https://learn.microsoft.com/en-us/dotnet/api/system.io.memorystream.position?view=net-8.0#exceptions). If this pull request is accepted, please don't forget to update the MemoryStream.Position documentation accordingly.

@runfoapprunfoappBot mentioned this pull request Jul 10, 2023
@jozkee

Copy link
Copy Markdown
Member

@elgonzo@manandre I do believe this is the right approach and we should continue with updating the docs for Seek() and Position accordingly.

@jozkeejozkee added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 17, 2023
@ghostghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 18, 2023
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));

@jozkeejozkeeAug 18, 2023

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.

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.

@elgonzoelgonzoAug 18, 2023

Copy link
Copy Markdown

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
Copy Markdown
ContributorAuthor

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.

@manandre
manandreforce-pushed the memory-stream-seek-max-length branch from 6f31b63 to 23e33c7CompareAugust 18, 2023 20:32

@jozkeejozkee left a comment

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.

LGTM, thanks!

@jozkee
jozkee merged commit 2d9d468 into dotnet:mainAug 21, 2023
@manandre
manandre deleted the memory-stream-seek-max-length branch August 21, 2023 18:51
@adamsitnikadamsitnik added this to the 9.0.0 milestone Aug 24, 2023
@ghostghost locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IOcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.IO.MemoryStream: ReadByte() and WriteByte() can throw IndexOutOfRangeException

5 participants

@manandre@elgonzo@jozkee@danmoseley@adamsitnik