rebis-dev: Fix Heap::compute_pstr_size being off by one cell #2759
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
triska
reviewed
Jan 7, 2025
|
It turns out that I had missed one instance of the issue spotted in #2729: I've rewritten it (and renamed it to the more accurate To make it easy to use it in the implementations of |
- Fix Heap::compute_pstr_size being off at len=0, len=n*align and null bytes (compute_pstr_size now more accurately reflects the behavior of allocate_pstr). - Add debug assertion for the behaviour of compute_pstr_size - Fix off-by-one errors in build_functor
…t to deduplicate unsafe code scan_slice_to_str was close to being safe already, but the code using it needed to be unsafe, and the safety of scan_slice_to_str was hinging on the correctness of the caller unsafe block. It also wrongly assumed that Heap.inner.ptr was aligned to Heap::heap_cell_alignment(). Instead, I have rewritten `scan_slice_to_str` (and renamed it to the more accurate name `scan_pstr_at`), to fix the incorrect computation caused by this wrong assumption, and to reduce the unsafe blocks to their minimum, whose safety I could then prove. The invariants of Heap remain to be fully vetted, as some of the code in this module remain ~wildly unsafe~ :)
adri326
force-pushed
the
fix-compute_pstr_size
branch
from
8c856bd to
5e08d4d
Compare
adri326
changed the title
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.
So it looks like my fix for segfault around heap and PStr had an off-by-one error in the logic for
compute_pstr_size, which this PR fixes.This off-by-one error was compounded by a second off-by-one error within the logic of
build_functor!()(some parts did not account for the[]atom appended toPStrs), which happened to cancel out with the previous, invalid implementation ofcompute_pstr_size.This does not address #2757 (and I would prefer to fix that issue in a separate PR, so that we can get the unit tests of
rebis-devback in order), though the logic forcompute_pstr_sizeshould coincide with the corrected behavior of #2757. The test cases I've added here ought to be tweaked once the correct behavior is set in stone.Now
Heap::compute_pstr_sizeaccurately measures the number of bytes needed byHeap::allocate_pstr(except in the instances of #2757).I've also simplified the logic a bit, and cleaned up the affected tests of
build_functor:)