Repository navigation
fix: improve Windows host build and strengthen heap/package tests - #60
Conversation
Review — three separate changes; two are fine, one needs a decisionBuilt merged onto current 1. 2. /* 11. Double-free should not corrupt allocator */
eos_free(a); /* a was already freed in step 9 */This pins double-free tolerance as a contract. If the allocator is ever changed to detect double-free and trap — which is a defensible hardening choice — this test starts failing and someone will "fix" it by weakening the allocator. If the intent is "we currently survive this", a comment saying so keeps the option open. If the intent is "we guarantee this", it should be stated in the heap's documentation, because it constrains future work. 3. Removing the x86_64 -#elif defined(__x86_64__) || defined(_M_X64)
- __asm volatile ("hlt");You are right that Splitting 3 into its own PR would let 1 and 2 land quickly. |
40806e1 to
a06bf7c
Compare
|
Thanks for the review. I’ve addressed the requested changes:
PR #60 now contains only the heap and package test improvements. The working tree is clean and the branch is pushed to my fork. |
…ree() (#77) The kernel allocator has a validation boundary in one of its two pointer-taking entry points. eos_free() range-checks its argument against the heap; eos_realloc() takes the same kind of pointer, derives a header from it, and trusts whatever it finds: block_header_t *block = (block_header_t *)((uint8_t *)ptr - HEADER_SIZE); size_t current_usable = block->size - HEADER_SIZE; if (new_size <= current_usable) return ptr; Two consequences, both reproduced against the unmodified allocator: 1. A pointer that never came from this heap is read out of bounds for its "header" and then handed straight back, as if it had been resized. eos_free() rejects exactly that pointer. 2. `block->size - HEADER_SIZE` is unsigned. A header whose size field is below HEADER_SIZE underflows current_usable to nearly SIZE_MAX, so every growth request looks already satisfied and realloc returns the original short buffer. The caller then writes new_size bytes into it. That turns a single corrupted header byte -- the thing a prior small overflow leaves behind -- into an unbounded overflow, which is the opposite of what an allocator's metadata checks are for. Both paths now go through one block_from_ptr() helper: the pointer must lie inside the heap, its header must be 8-byte aligned relative to the heap base, and the size field must be at least MIN_BLOCK_SIZE and not claim to extend past the end of the heap. eos_free() uses it too and gains the size checks it did not have. An unrecognised pointer makes realloc return NULL and free a no-op, which is what each already did for the cases it did check. Verified: ctest 24/24, pytest 10 passed. tests/test_heap.c gains coverage for both paths plus a grow-and-preserve case; against the unmodified heap.c it fails on "realloc of a non-heap pointer must be rejected". Note for review: PR #60 also touches tests/test_heap.c (it does not touch kernel/src/mem/heap.c). If that lands first this needs a trivial rebase in the test file. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Needs a rebase — #77 landed in the same fileOnly Worth reading #77 before you resolve, because it overlaps this PR's subject rather than just its line numbers. It added:
So some of what this PR strengthens in On that: I cannot verify the Windows changes here at all. Linux is the only host available to me, so for that half I am relying on your testing rather than mine, and I would rather say so than imply I checked it. If CI covers the Windows build after the rebase, that is the evidence I would go on.
|
a06bf7c to
2a7a452
Compare
|
Thanks for the review. I’ve completed the requested rebase and cleanup.
The branch is clean and pushed to my fork. I’ve also verified that the current PR diff contains only Please take another look when convenient. |
…ree()
The kernel allocator has a validation boundary in one of its two pointer-taking
entry points. eos_free() range-checks its argument against the heap;
eos_realloc() takes the same kind of pointer, derives a header from it, and
trusts whatever it finds:
block_header_t *block = (block_header_t *)((uint8_t *)ptr - HEADER_SIZE);
size_t current_usable = block->size - HEADER_SIZE;
if (new_size <= current_usable) return ptr;
Two consequences, both reproduced against the unmodified allocator:
1. A pointer that never came from this heap is read out of bounds for its
"header" and then handed straight back, as if it had been resized. eos_free()
rejects exactly that pointer.
2. `block->size - HEADER_SIZE` is unsigned. A header whose size field is below
HEADER_SIZE underflows current_usable to nearly SIZE_MAX, so every growth
request looks already satisfied and realloc returns the original short
buffer. The caller then writes new_size bytes into it. That turns a single
corrupted header byte -- the thing a prior small overflow leaves behind --
into an unbounded overflow, which is the opposite of what an allocator's
metadata checks are for.
Both paths now go through one block_from_ptr() helper: the pointer must lie
inside the heap, its header must be 8-byte aligned relative to the heap base,
and the size field must be at least MIN_BLOCK_SIZE and not claim to extend past
the end of the heap. eos_free() uses it too and gains the size checks it did
not have. An unrecognised pointer makes realloc return NULL and free a no-op,
which is what each already did for the cases it did check.
Verified: ctest 24/24, pytest 10 passed. tests/test_heap.c gains coverage for
both paths plus a grow-and-preserve case; against the unmodified heap.c it
fails on "realloc of a non-heap pointer must be rejected".
Note for review: PR embeddedos-org#60 also touches tests/test_heap.c (it does not touch
kernel/src/mem/heap.c). If that lands first this needs a trivial rebase in the
test file.
Summary
This PR improves Windows host/simulation compatibility and strengthens the heap and package test coverage.
Changes
Removed the x64
hltinstruction from the host/simulation idle-task path inkernel/src/task.cto avoid executing a privileged CPU instruction during host-based testing.Expanded
tests/test_heap.cto validate:calloczero-initializationreallocdata preservationUpdated
tests/test_package.cto use staticEosPackageSetinstances, preventing stack-related issues in the Windows test environment.Validation
git diff --checkcompleted with no errors.Test Command
Result