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

added real bounds checking to ByteString.Slice #6709

Merged

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented May 2, 2023

Changes

Fixed a very weird bit of code that could result in byte buffers constantly allocating despite being of illegal size. This caused parts of Petabridge.Cmd's Akka.IO message decoding to run out of memory during message decoding.

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

Fixed a very weird bit of code that could result in byte buffers constantly allocating despite being of illegal size.
Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Detailed my changes

@@ -57,8 +60,10 @@ public void A_ByteString_must_be_sequential_when_slicing_from_start()
[Fact]
public void A_ByteString_must_be_sequential_when_slicing_from_index()
{
Prop.ForAll((ByteString a, ByteString b) => (a + b).Slice(a.Count).SequenceEqual(b))
Copy link
Member Author

Choose a reason for hiding this comment

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

Some of these property-based tests no longer pass because, as should be no surprise, the ByteString.Slice method will no longer work with arbitrary index and count values - hence why certain random-generated values fail during testing now.

public ByteString Slice(int index, int count)
{
//TODO: this is really stupid, but previous impl didn't throw if arguments
Copy link
Member Author

Choose a reason for hiding this comment

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

@Horusiath 's comment here was the first sign that there was something weird going on inside this method, when I started investigating how our Petabridge.Cmd code could run out of memory. Sure enough - this method will produce arbitrary byte arrays regardless of what the index and count values are, which caused our looping mechanism for decoding messages to go haywire.

If someone is depending on this old behavior - sorry, but that's your problem now - we're going back to byte arrays meaning what they mean starting in Akka.NET V1.5.

@Aaronontheweb Aaronontheweb added this to the 1.5.5 milestone May 2, 2023
@Aaronontheweb Aaronontheweb marked this pull request as ready for review May 2, 2023 19:41
@Aaronontheweb
Copy link
Member Author

My ideal solution to this issue is #6708 - but that's going to need to wait until Akka.NET v1.6 most likely. We'll need to get rid of SocketAsyncEventArgs and refactor Akka.IO to use System.Memory or System.Buffers constructs instead.

Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb merged commit a1a371d into akkadotnet:dev May 3, 2023
8 of 12 checks passed
@Aaronontheweb Aaronontheweb deleted the fix-bytestring-bounds-checking branch May 3, 2023 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants