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

Allow reading more than the minimum number when filling buffer in XmlBufferReader #72847

Closed
wants to merge 1 commit into from
Closed

Allow reading more than the minimum number when filling buffer in XmlBufferReader #72847

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBufferReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ private bool TryEnsureBytes(int count)
}
int needed = newOffsetMax - _offsetMax;
DiagnosticUtility.DebugAssert(needed > 0, "");
int read = _stream.ReadAtLeast(_buffer.AsSpan(_offsetMax, needed), needed, throwOnEndOfStream: false);
int read = _stream.ReadAtLeast(_buffer.AsSpan(_offsetMax), needed, throwOnEndOfStream: false);
Copy link
Member

Choose a reason for hiding this comment

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

Prior to the change to use ReadAtLeast, this was also limiting how much was read to needed. Is it possible changing it to read more could create bugs? For example, is it possible this might end up reading more from the stream that a consumer intended, such that it would erroneously consume something from the stream after what is being deserialized?

Copy link
Contributor Author

@Daniel-Svensson Daniel-Svensson Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch, I am thinking about closing this for now and maybe add a comment in a separate PR.

Is there any other reader which have any way around that (such as resetting the stream position) and if so how late (dispose or after each read) would it be ok to reset the position?

Copy link
Member

Choose a reason for hiding this comment

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

Resetting position isn't always possible: many (most) streams don't support it.

We should just close this PR for now. Thanks, though.

_offsetMax += read;

if (read < needed)
Expand Down