Remove invalid math in ByteBuffer slice slow path #2075
Merged
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
Motivation: ByteBuffer uses a synthetic UInt24 as the lower bound of the slice of memory that it can access. This necessitates a slow path when we create slices of ByteBuffers that would force the lower bound to be larger than a UInt24. This slow path has a bunch of math within it, and it turns out that some of it is invalid. This means in rare cases slicing a ByteBuffer can cause a crash. That's not great: the code should only crash if we broke an internal invariant, not because we did some invalid math. In particular, the relevant check is calculating the upper bound of the storage to be copied. This was: ``` min(storageRebaseAmount + _toCapacity(self._slice.count), self._slice.upperBound, storageRebaseAmount + capacity) ``` In this case, `storageRebaseAmount` is either `self._slice.lowerBound` or `self._slice.lowerBound + self._readerIndex`: that is, `storageRebaseAmount >= self._slice.lowerBound`. Given that `self._slice.count == self._slice.upperBound - self._slice.lowerBound`, we know that `storageRebaseAmount + self._slice.count` is always strictly greater than or equal to `self._slice.upperBound`. Calculating this is unnecessary. Unfortunately, it can also overflow: if `self._slice.upperBound + self._readerIndex` is greater than UInt32 then this will crash. Note that the other math, `storageRebaseAmount + capacity`, has been kept. `capacity` is always computed by us and this should only ever overflow if we are increasing the size of the buffer past UInt32.max, which we cannot handle anyway. Modifications: - Restructured and heavily commented _copyStorageAndRebase. - Removed the redundant crashing check. Result: Less trapping!
Lukasa
added
the
patch-version-bump-only
weissi
reviewed
Apr 11, 2022
|
|
||
| // Step 5: Fixup the indices. These can never underflow: `indexRebaseAmount` can only ever be 0 or `self._readerIndex`, so we either do nothing | ||
| // or we move the indices backwards by a valid range, as `self._writerIndex` is always >= `self._readerIndex`. | ||
| self._moveReaderIndex(to: self._readerIndex &- indexRebaseAmount) |
There was a problem hiding this comment.
Thanks for rewriting, commenting & fixing this! The only question I have is why we use unchecked math here. This function is the slow path and never inlined. So I don't think there's any harm caused by always doing checked math (even if it's clearly safe do use the unchecked math in this version of the code.)
The reason I'm mildly worried is because of potential future changes, they might cargo cult the unchecked math even in a place where it doesn't matter and so a new vulnerability or smth may slip in.
Feel free to ignore :)
There was a problem hiding this comment.
Yeah, I'm definitely on the fence about that. Let's see what the others think.
FranzBusch
reviewed
Apr 12, 2022
| // We only want to run this test on 64-bit systems: 32-bit systems can't allocate buffers | ||
| // large enough to run this test safely. | ||
| guard MemoryLayout<Int>.size >= 8 else { | ||
| return |
There was a problem hiding this comment.
Nit: Can we use XCTSkip here instead of just returning
Suggested change
| return | |
| throw XCTSkip("This test is only supported on 64-bit systems") |
FranzBusch
approved these changes
Apr 13, 2022
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.
Motivation:
ByteBuffer uses a synthetic UInt24 as the lower bound of the slice of
memory that it can access. This necessitates a slow path when we create
slices of ByteBuffers that would force the lower bound to be larger than
a UInt24.
This slow path has a bunch of math within it, and it turns out that some
of it is invalid. This means in rare cases slicing a ByteBuffer can
cause a crash. That's not great: the code should only crash if we broke
an internal invariant, not because we did some invalid math.
In particular, the relevant check is calculating the upper bound of the
storage to be copied. This was:
In this case,
storageRebaseAmountis eitherself._slice.lowerBoundorself._slice.lowerBound + self._readerIndex: that is,storageRebaseAmount >= self._slice.lowerBound. Given thatself._slice.count == self._slice.upperBound - self._slice.lowerBound,we know that
storageRebaseAmount + self._slice.countis alwaysstrictly greater than or equal to
self._slice.upperBound. Calculatingthis is unnecessary. Unfortunately, it can also overflow: if
self._slice.upperBound + self._readerIndexis greater than UInt32 thenthis will crash.
Note that the other math,
storageRebaseAmount + capacity, has beenkept.
capacityis always computed by us and this should only everoverflow if we are increasing the size of the buffer past UInt32.max,
which we cannot handle anyway.
Modifications:
Result:
Less trapping!