fix(build): repair merge artifacts that left the tree uncompilable - #69
Conversation
The tree did not build. `gcc -c core/image_verify.c` failed on the first core source file, and once past it two more sources and eleven of the seventeen unit tests could not compile or link. Every defect here is a leftover from a merge, not a design decision. Sources - include/eos_image.h declared `int eos_crc32(uint32_t, size_t, uint32_t *)` while core/image_verify.c defines `uint32_t eos_crc32(uint32_t, size_t)`. Conflicting types; the build stopped there. The declaration now matches the definition and the doc comment that sits above it. - core/ed25519_verify.c: eos_ed25519_verify() never verified anything. Two copies of the challenge-hash step had been merged into the function, the second calling identifiers that do not exist (sha512_ctx_t, sc_reduce), and RFC 8032 step 4 -- the [S]B == R + [k]A check -- was missing outright, leaving the function returning an undeclared `diff`. The duplicate is removed and the group equation restored using the primitives already in the file. - core/recovery.c: recovery_handle_write() declared slot_size twice. The bounds check now goes through eos_recovery_write_in_range(), which the unit tests already exercise, so the rule for wire-supplied offset/len has a single definition and an unmapped slot (base == 0) is rejected too. - SHA-512 had two incompatible declarations: include/eos_sha512.h (sha512_*, sha512_ctx_t) against include/eos_crypto_boot.h (eos_sha512_*, eos_sha512_ctx_t, plus a one-shot eos_sha512()). core/sha512.c implemented the first; ed25519_verify.c and the tests called the second, which nothing defined. core/sha512.c now implements the eos_-prefixed API including the missing one-shot, eos_sha512_ctx_t keeps the 128-bit length counter FIPS 180-4 requires, and the duplicate header is deleted. Build files - core/sha512.c and core/rollback.c were never compiled, so eboot_core could not resolve eos_sha512_* or eos_rollback_*. core/boot_log.c was listed twice. - The EBLDR_BOARD dispatch chain was duplicated from cortex_m3 onward with a stray message(FATAL_ERROR ...) inside the kalimba branch, so `cmake -DEBLDR_BOARD=kalimba` aborted configuration for a supported board and 110 later branches were unreachable. This is exactly the regression tests/unit/test_cmake_board_dispatch.py was written to catch; it had come back and those three tests were failing on master. - tests/unit/test_fw_transport.c existed but was never built or run. Tests - tests/unit/test_slot_manager.c was two different test files spliced together mid-function: stub bodies cut in half, and a main() calling twenty functions that are not in the file. Rebuilt as one suite that drives the real core/slot_manager.c through scriptable per-slot mocks of the three verification stages, following the harness conventions in test_slot_size_bounds.c. - tests/unit/test_recovery.c defined local boot-log stand-ins that conflicted with include/eos_boot_log.h and duplicated symbols now linked from core/boot_log.c. Verified: cmake configure, `cmake --build` (0 errors) and `ctest` 17/17 pass; `pytest tests` 27/27 pass, including the three board-dispatch tests that were failing before this change. test_ed25519 exercises the RFC 8032 vectors, tampered messages, every single-bit signature flip, wrong keys and malleated signatures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QjaFDcvWmF1ApZQedLXJz5
srpatcha
left a comment
There was a problem hiding this comment.
Approving, with a note on overlap you should know about before spending more time
on it.
This substantially duplicates #58
Both repair the same merge artifacts, and both converge on the same resolution.
Shared files:
CMakeLists.txt
core/ed25519_verify.c
core/recovery.c
core/sha512.c
include/eos_crypto_boot.h
include/eos_image.h
include/eos_sha512.h (deleted by both, consolidated into eos_crypto_boot.h)
#58 is already approved and is the base of the eBoot chain. It also covers
things this does not — stage0/jump_stage1.c, include/eos_boot_log.h,
tests/unit/test_boot_log.c, and the CI workflows.
Two people independently deleting include/eos_sha512.h and folding it into
eos_crypto_boot.h is a good sign that is the right call. I checked nothing else
includes it.
Your unique contribution is the recovery tests, and they are good
tests/unit/test_recovery.c is the part that is not in #58, and it is the more
interesting half of this PR:
These tests exercise the real
core/slot_manager.c. The three image
verification steps it calls are replaced with per-slot scriptable mocks so each
stage can be failed independently without having to build and sign real images.
That is the right structure. Mocking the three verification calls while keeping
the real slot manager and the real eos_board_ops_t geometry means the test
exercises the logic under test rather than a reimplementation of it — the failure
mode where a suite passes because it is testing its own mock.
Verified on a base where master's build is repaired:
0 build errors
100% tests passed, 0 tests failed out of 16
Suggested path
Rather than resolving this against #58, rebase onto it and reduce this PR to just
tests/unit/test_recovery.c and its tests/CMakeLists.txt entry. The build
repair is then #58's, the recovery coverage is yours, and there is no conflict to
settle. It also makes this much easier to review on its merits.
Two things to expect when you do:
tests/unit/test_slot_manager.cconflicts against #58's version. That file is
where the two PRs genuinely diverge.- eBoot#71 namespaces all test targets as
eboot_*, sinceeosandeBootboth
definedtest_cryptoandtest_multicoreandebuildcomposes them into one
CMake project.test_recoverybecomeseboot_test_recovery; the
add_test(NAME ...)label is unchanged.
Approving so this is not blocked on me. Merge order is #58, then #71, then this.
Master stopped building/testing clean again after several PRs (embeddedos-org#60, embeddedos-org#61, embeddedos-org#64, embeddedos-org#67, embeddedos-org#69, embeddedos-org#71) landed back-to-back without an integration build between them: - tests/CMakeLists.txt: embeddedos-org#61 registered test_fw_transport without the eboot_ prefix embeddedos-org#71's namespace guard now requires, so configure aborted with "Test target 'test_fw_transport' is not namespaced." Renamed the target to eboot_test_fw_transport (add_test NAME stays test_fw_transport per the guard's own guidance). - tests/CMakeLists.txt: the valgrind foreach block still referenced the pre-embeddedos-org#60/embeddedos-org#71 bare target names (test_bootctl, etc.) in $<TARGET_FILE:...>, which no longer resolve now that every target is eboot_-prefixed. Fixed the generator-expression reference while leaving the valgrind_${TEST_NAME} test labels unchanged. - core/recovery.c: recovery_handle_write() called eos_recovery_write_in_range(base, slot_size, ...) with slot_size never declared -- embeddedos-org#69 introduced the call but the counterpart eos_hal_slot_size(slot) lookup (mirroring the existing eos_hal_slot_addr(slot) line right above it) never made it in. - tests/unit/test_slot_manager.c: two versions of the file's fixture and test bodies had been spliced together by a merge (duplicate globals/slot_index, an unused old RUN macro next to the real TEST macro), and the TEST macro's simulated-flash fixture (sim_flash/sim_tick/sim_ops) was referenced but never defined anywhere in the file. Rebuilt the file as one coherent suite matching its own doc comment and every sibling test file's sim_board_ops_t convention; converted the three tests that were still plain functions to the same TEST() macro as the rest for consistency. Also gave sim_flash_erase real erase_result/ erased_addr/erased_size bookkeeping -- the generic version copied in ignored those script variables entirely, which the erase test relies on. Verified: cmake configure + build, 0 errors. ctest 17/17 (non-valgrind) and 32/32 including valgrind, all passing -- including test_recovery's real HAL-level exercise of the new slot_size bounds check and test_slot_manager's erase-failure/erase-bookkeeping cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VvWBEZhDegTQMaqVtry2mM
The tree did not build.
gcc -c core/image_verify.cfailed on the first core source file, and once past it two more sources and eleven of the seventeen unit tests could not compile or link. Every defect here is a leftover from a merge, not a design decision.Sources
int eos_crc32(uint32_t, size_t, uint32_t *)while core/image_verify.c definesuint32_t eos_crc32(uint32_t, size_t). Conflicting types; the build stopped there. The declaration now matches the definition and the doc comment that sits above it.diff. The duplicate is removed and the group equation restored using the primitives already in the file.Build files
cmake -DEBLDR_BOARD=kalimbaaborted configuration for a supported board and 110 later branches were unreachable. This is exactly the regression tests/unit/test_cmake_board_dispatch.py was written to catch; it had come back and those three tests were failing on master.Tests
Verified: cmake configure,
cmake --build(0 errors) andctest17/17 pass;pytest tests27/27 pass, including the three board-dispatch tests that were failing before this change. test_ed25519 exercises the RFC 8032 vectors, tampered messages, every single-bit signature flip, wrong keys and malleated signatures.