Add CHERI-safe memmove implementation - #823
Open
David CARLIER (devnexen) wants to merge 7 commits into
Open
Conversation
New attempt at custom memmove, succeeding PR microsoft#593 which was reverted due to fuzzer-found bugs (off-by-one in reverse copy loop with size_t underflow) and CHERI incompatibility (byte-at-a-time reverse copy destroys capability tags). Key changes from the original PR microsoft#593: - Per-Arch move() and forward_move() methods following the existing copy() pattern, instead of a single generic byte-by-byte reverse. - Three-way overlap detection: non-overlapping uses optimized Arch::copy(), dst > src overlap uses Arch::move() (reverse), and dst < src overlap uses Arch::forward_move() (forward without the copy_end trick that re-reads already-overwritten bytes). - block_reverse_copy<Size> operates at register width (16 bytes on x86-64/PPC64/CHERI) instead of byte-by-byte, with byte-by-byte only for the sub-register remainder. - GenericStrictProvenance (CHERI) move() and forward_move() preserve capability tags by using pointer-pair (Ptr2) operations on aligned regions, with byte-by-byte only for sub-pointer head/tail padding where no aligned capabilities can exist. - Comprehensive tests: overlapping copies at various sizes (1-2048) and overlap amounts, exhaustive offset x length testing for small buffers (2-64), alignment boundary tests, bounds checking, and direct snmalloc::memmove<false> unchecked path tests. - Re-enables memmove fuzz tests (simple_memmove, forward_memmove, backward_memmove) in snmalloc-fuzzer.cpp. Co-authored-by: Claude <noreply@anthropic.com>
David CARLIER (devnexen)
marked this pull request as draft
March 3, 2026 22:54
Collaborator
Author
|
So to summarize, Claude filled the gap in regard of CHERI here. |
Add copy_one_move<Size> that always uses struct-copy instead of __builtin_memcpy_inline, which ASan treats as memcpy and flags overlapping src/dst as an error. Use it in block_reverse_copy and a new block_copy_move for all forward_move/move overlap paths.
Move all memmove tests into a new src/test/func/memmove/ directory so they build as func-memmove-fast and func-memmove-check, separate from func-memcpy-fast and func-memcpy-check. This makes it clear which test is failing when CI reports errors.
The struct copy (*d = *s) in copy_one_move was being lowered by the compiler into a memcpy call, which ASan then flagged as memcpy-param-overlap for memmove's overlapping buffers. Switch to __builtin_memmove which correctly handles overlap and still optimizes to register-width loads/stores. Add a byte-by-byte fallback for compilers lacking __builtin_memmove.
David CARLIER (devnexen)
marked this pull request as ready for review
March 4, 2026 05:08
Member
|
Nathaniel Wesley Filardo (@nwf) would you be able to review this? |
Matthew Parkinson (mjp41)
left a comment
Matthew Parkinson (mjp41)
left a comment
Member
There was a problem hiding this comment.
It would be great to get some eyes from CHERI experts on this (Nathaniel Wesley Filardo (@nwf), David Chisnall (@davidchisnall))
Comment on lines
+45
to
+51
| // Fallback: byte-by-byte copy through a temporary buffer to avoid | ||
| // the compiler generating a memcpy call for struct assignment. | ||
| char tmp[Size]; | ||
| for (size_t i = 0; i < Size; ++i) | ||
| tmp[i] = static_cast<const char*>(src)[i]; | ||
| for (size_t i = 0; i < Size; ++i) | ||
| static_cast<char*>(dst)[i] = tmp[i]; |
There was a problem hiding this comment.
This doesn't feel safe on CHERI? I guess that CHERI platforms will always have __builtin_memove?
| SNMALLOC_FAST_PATH_INLINE void copy_one_move(void* dst, const void* src) | ||
| { | ||
| #if __has_builtin(__builtin_memmove) | ||
| __builtin_memmove(dst, src, Size); |
There was a problem hiding this comment.
Is it possible that __builtin_memmove can map back onto memmove? Would lead to infinite recursion, though that is not the worst failure as it would be easy to see what is happening wrong.
| * No rep movsb in reverse (ERMS doesn't support DF=1), so use | ||
| * SSE-width block_reverse_copy. | ||
| */ | ||
| static void* move(void* dst, const void* src, size_t len) |
There was a problem hiding this comment.
For symmetry should this be called reverse_move? Same comment for all ::moves as they all appear to be reverse.
David CARLIER (devnexen)
force-pushed
the
memmove_try2
branch
from
May 3, 2026 07:43
d621573 to
a0e88f3
Compare
David CARLIER (devnexen)
force-pushed
the
memmove_try2
branch
from
May 3, 2026 07:55
a0e88f3 to
c93ea4d
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
New attempt at custom memmove, succeeding PR #593 which was reverted due to fuzzer-found bugs (off-by-one in reverse copy loop with size_t underflow) and CHERI incompatibility (byte-at-a-time reverse copy destroys capability tags).
Key changes from the original PR #593:
Per-Arch move() and forward_move() methods following the existing copy() pattern, instead of a single generic byte-by-byte reverse.
Three-way overlap detection: non-overlapping uses optimized Arch::copy(), dst > src overlap uses Arch::move() (reverse), and dst < src overlap uses Arch::forward_move() (forward without the copy_end trick that re-reads already-overwritten bytes).
block_reverse_copy operates at register width (16 bytes on x86-64/PPC64/CHERI) instead of byte-by-byte, with byte-by-byte only for the sub-register remainder.
GenericStrictProvenance (CHERI) move() and forward_move() preserve capability tags by using pointer-pair (Ptr2) operations on aligned regions, with byte-by-byte only for sub-pointer head/tail padding where no aligned capabilities can exist.
Comprehensive tests: overlapping copies at various sizes (1-2048) and overlap amounts, exhaustive offset x length testing for small buffers (2-64), alignment boundary tests, bounds checking, and direct snmalloc::memmove unchecked path tests.
Re-enables memmove fuzz tests (simple_memmove, forward_memmove, backward_memmove) in snmalloc-fuzzer.cpp.