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.

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

Fix Length for ReadOnlySequence created out of sliced Memory owned by MemoryManager #57479

Merged
merged 4 commits into from
Aug 16, 2021

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Aug 16, 2021

fixes #57472 (edited)

big thanks to @kuda1978 for providing not just a repro case but also the fix!

@ghost
Copy link

ghost commented Aug 16, 2021

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

Issue Details

fixes #57478

big thanks to @kuda1978 for providing not just a repro case but also the fix!

Author: adamsitnik
Assignees: -
Labels:

area-System.Memory

Milestone: 6.0.0

@campersau
Copy link
Contributor

Correct issue reference is #57472

return new ReadOnlySequence<T>(new ReadOnlyMemory<T>(new T[size + 1]).Slice(1));
#else
return new ReadOnlySequence<T>(new ReadOnlyMemory<T>(new T[size]));
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I generally prefer for the ifdef to be around the minimal thing that's different, so that it's clear what exactly the difference is. e.g.

ReadOnlyMemory<T> m = new T[size + 1];
#if DEBUG
m = m.Slice(1);
#endif
return new ReadOnlySequence<T>(m);

@davidfowl
Copy link
Member

Is it backport worthy?

@jeffhandley
Copy link
Member

Is it backport worthy?

@adamsitnik are there customers blocked by this in 3.1 or 5.0 that won't be migrating to 6.0 soon? Alternatively, does the bug present in a way that could lead to data loss or corruption in common use cases?

@adamsitnik adamsitnik merged commit d7a7898 into dotnet:main Aug 16, 2021
@adamsitnik adamsitnik deleted the issue57472 branch August 16, 2021 18:12
thaystg added a commit to thaystg/runtime that referenced this pull request Aug 16, 2021
…information

# By dotnet-maestro[bot] (4) and others
# Via GitHub
* origin/main: (58 commits)
  Localized file check-in by OneLocBuild Task (dotnet#57384)
  [debugger][wasm] Support DebuggerProxyAttribute (dotnet#56872)
  Account for type mismatch of `FIELD_LIST` members in LSRA (dotnet#57450)
  Qualify `sorted_table` allocation with `nothrow` (dotnet#57467)
  Rename transport packages to follow convention (dotnet#57504)
  Generate proper DWARF reg num for ARM32 (dotnet#57443)
  Enable System.Linq.Queryable and disable dotnet#50712 (dotnet#57464)
  Mark individual tests for 51211 (dotnet#57463)
  Fix Length for ReadOnlySequence created out of sliced Memory owned by MemoryManager (dotnet#57479)
  Add JsonConverter.Write/ReadAsPropertyName APIs (dotnet#57302)
  Remove workaround for dotnet/sdk#19482 (dotnet#57453)
  Do not drain HttpContentReadStream if the connection is disposed (dotnet#57287)
  [mono] Fix a few corner case overflow operations (dotnet#57407)
  make use of ports in SPN optional (dotnet#57159)
  Fixed H/3 stress server after the last Kestrel change (dotnet#57356)
  disable a failing stress test. (dotnet#57473)
  Eliminate temporary byte array allocations in the static constructor of `IPAddress`. (dotnet#57397)
  Update dependencies from https://github.com/dotnet/emsdk build 20210815.1 (dotnet#57447)
  [main] Update dependencies from mono/linker (dotnet#57344)
  Improve serializer performance (dotnet#57327)
  ...

# Conflicts:
#	src/mono/wasm/debugger/BrowserDebugProxy/MemberReferenceResolver.cs
#	src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
#	src/mono/wasm/debugger/BrowserDebugProxy/MonoSDBHelper.cs
@adamsitnik
Copy link
Member Author

adamsitnik commented Aug 17, 2021

Alternatively, does the bug present in a way that could lead to data loss or corruption in common use cases?

It can definitely lead to data loss. However, the scenario should not be very common.

I've sent ported the fix to 5.0 in #57562
Edit: as soon as dotnet/corefx#43100 is merged I should be able to build corefx and port it to 3.1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Negative Length in ReadOnlySequence
5 participants