Fix crash: Create ByteBuffer Slice from ByteBuffer Slice after 16MB #1813
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lukasa
added
the
patch-version-bump-only
fabianfett
force-pushed
the
ff-fix-slice-copy-crash
branch
from
f3a98d6
to
5037dfc
Compare
Lukasa
requested changes
Apr 21, 2021
weissi
requested changes
Apr 21, 2021
There was a problem hiding this comment.
Thank you so much @fabianfett , this is really fantastic work!!
Tests/NIOTests/ByteBufferTest.swift
Outdated
| // 16MiB after the originating backing storage. | ||
|
|
||
| // create a buffer with (16MiB - 1byte) + 2 bytes = 16MiB + 1 byte | ||
| let inputBufferLength = Int(_UInt24.max) + 2 |
There was a problem hiding this comment.
Just hardcode the value here. The _ means that _UInt24 is meant to be private. So let inputBufferLength = 1 << 24 + 1 or similar.
| let finalSlice = remainingSlice.readSlice(length: finalSliceLength) | ||
| XCTAssertEqual(finalSlice?.storageCapacity, 1) | ||
| XCTAssertEqual(finalSlice?._slice.lowerBound, 0) | ||
|
|
There was a problem hiding this comment.
we should additionally assert
- that the slices/values read have the correct content (
1s,2s, and3s or so as suggested above) - that the underlying storage has indeed changed. There's a (test only) property called
buffer.storageIdentifieror so which stays the same if nothing got reallocated/copied and changes if it has
weissi
reviewed
Apr 21, 2021
fabianfett
force-pushed
the
ff-fix-slice-copy-crash
branch
from
5037dfc
to
ff3956e
Compare
fabianfett
force-pushed
the
ff-fix-slice-copy-crash
branch
from
ff3956e
to
5c8e24c
Compare
Lukasa
reviewed
Apr 22, 2021
Co-authored-by: Cory Benfield <lukasa@apple.com>
weissi
approved these changes
Apr 22, 2021
Co-authored-by: Johannes Weiss <johannesweiss@apple.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes an issue that could lead to an assert in Debug mode and to undefined behavior in Release mode.
Motivation:
ByteBufferslice from an originatingByteBufferslice where the newByteBufferslice starts more than 16MiB into the underlyingByteBufferstorage, the newByteBufferslice will get a new 0 based storage.Modifications:
ByteBufferslice which is more than 16MiB into its originating storage.Result:
ByteBufferslices from slices after the 16MiB threshold.