Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#111 "Remove duplicate FDT loader test target"
head: 77ca811 author: Mohammed18-19 ci: fail (6 required checks red — inherited, see finding 1)
Verdict: Correct, minimal, and honestly described. I reproduced both claims in the PR
body exactly: configure passes with this patch and fails without it, and the host build
then stops on the same pre-existing sha512.c / ed25519_verify.c errors the body names.
The one thing worth acting on is that #115 contains this identical hunk, so one of the
two is redundant.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | (whole PR) | #115 removes the same five lines. git diff of #115 against its merge-base shows a byte-identical deletion of the test_fdt_loader block at the same anchor (after list(APPEND EBLDR_UNIT_TESTS test_ecc)), plus 14 other EBLDR_UNIT_TESTS additions and the sha512.c/ed25519_verify.c repairs. Because the hunk is identical on both sides, git will merge them without a textual conflict — so this is wasted review effort rather than a breakage risk. It does, however, mean that landing this PR alone leaves CI just as red as it is now, which a merge queue reading "removes the thing that breaks configure" would not expect. |
Land this one first — it is five deleted lines and trivially safe — and let #115 rebase and drop the now-empty hunk. Or close this in favour of #115. Either way, say in the PR which it is; do not merge both blind and assume the overlap is harmless. |
| 2 | Low | tests/CMakeLists.txt:187 |
The deletion leaves two consecutive blank lines where the block was, between list(APPEND EBLDR_UNIT_TESTS test_ecc) and the test_fw_decrypt comment. Every other section in this file is separated by exactly one. |
Delete one of the blank lines. |
| 3 | Low | tests/CMakeLists.txt:200-204 (the surviving block) |
The surviving test_fdt_loader registration calls add_test() but never list(APPEND EBLDR_UNIT_TESTS test_fdt_loader). The EBLDR_UNIT_TESTS list is consumed at tests/CMakeLists.txt:226 to generate the valgrind_* targets, so test_fdt_loader runs under plain ctest but gets no valgrind variant. That matters more here than for most tests: core/fdt_loader.c is a parser fed malformed device-tree blobs, which is exactly the class .ai/security.md says gets memory-checking coverage, and the block's own comment records that the file "was in no source list, so it had never been compiled." Not introduced by this PR, and #115 fixes it by adding the missing list(APPEND ...). |
Nothing to do here if #115 lands. If #115 is dropped or delayed, add the one list(APPEND EBLDR_UNIT_TESTS test_fdt_loader) line to this PR. |
Verification performed for this review
Detached scratch worktree under .ai/autoreview/state/verify/; the user's eBoot
checkout was not touched and nothing was pushed.
| Check | Result |
|---|---|
cmake -B build/host -DEBLDR_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug on this PR head |
PASS — "Configuring done", eBootloader v3.0.2, Tests: ON |
Same configure on clean origin/master (this PR not applied) |
FAIL — add_executable cannot create target "eboot_test_fdt_loader" because another target with that name already exists |
cmake --build build/host --parallel 4 on this PR head |
FAIL — core/sha512.c ×5 (eos_sha512_ctx_t has no member named 'count'), core/ed25519_verify.c:303 (redefinition of 'scalarbase') |
Same build from clean origin/master with only the duplicate removed |
FAIL — identical error set |
ctest |
NOT RUN — the build never produced binaries. |
That confirms the PR body line by line: the fix works, and the residual failures are not
this PR's. It also confirms the flip side — this PR does not make CI green on its own.
Architecture conformance
Conforms. §21: eBoot is Tier 1 Foundation and test-harness configuration belongs in the
owning repo. No #include, link line, target_link_libraries entry or manifest
dependency is added or changed, so §5.1's dependency direction is untouched; the diff is
pure deletion. Per .ai/architect.md this is not a structural change and needs no ADR.
Nothing here widens the TCB — tests/ is not linked into a boot image.
Was anything weakened?
No. test_fdt_loader is still registered and still runs — I observed it as Test #30 test_fdt_loader ... Passed in a full ctest run of a tree with this change applied.
The deleted block is the redundant second registration of the same executable and the
same test name, not a removed test. Assertion count, test count and coverage are unchanged.
Proposed changes
--- a/tests/CMakeLists.txt
@@ -184,7 +184,6 @@ add_test(NAME test_ecc COMMAND eboot_test_ecc)
list(APPEND EBLDR_UNIT_TESTS test_ecc)
-
# --- test_fw_decrypt: Streaming AES-256-GCM firmware decryption ---Then in the PR body, add one line naming the overlap with #115 and which of the two is
intended to land. Nothing else.
Not checked
ctestwas never reached, so this review has no evidence about test behaviour on
this branch — only that configure succeeds and compilation fails at a known pre-existing
point. The 31/31 pass result I have for eBoot comes from a tree that also had #115
applied, which is not this PR.- Cross-compile (STM32F4, Cortex-M4) and the fuzz harness build were not attempted
locally; those required checks are red for the same inherited reason, but I did not
confirm that by reading their job logs. - I did not read the
Analyze (C/C++)orCI Gatejob logs to confirm each red check
fails at the compile step rather than for some second reason. - Whether any open PR other than #115 also deletes this block — I compared against #115
only, because it was the other eBoot PR in this run's manifest that touches
tests/CMakeLists.txt.
Automated architecture review of 77ca8112a21f — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.
srpatcha
left a comment
There was a problem hiding this comment.
Thanks Mohammed — this is the right hunk: the second registration (now at tests/CMakeLists.txt:196-200) is the one to keep, and I confirmed configure goes from failing on master to passing on your branch.
Heads-up so the outcome is not a surprise: #115 (repair of master after the 09-07 batch merge) contains this identical change plus the compile fixes that follow it — on your branch cmake --build still fails in core/sha512.c and core/ed25519_verify.c:303. If #115 lands first this will merge as a no-op; if this lands first #115 rebases cleanly. Either order is fine. Approving.
Twenty PRs were merged into master in ninety minutes on 09-07, each on the base it was written against. Master has not configured, compiled, linked or passed its tests since. Every nightly and every eos simulation run (which builds eBoot master) has been red for the same reason. Configure: - tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it, and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111. Compile (eboot_core): - embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c). - embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written. CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb). - embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix. Header restored (964ebb8). - embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical static scalarbase(); both merged. One copy removed. - The same pair each added k_low_order[]/messages[] to test_ed25519.c. The embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[]. Tests that stopped passing because two merged PRs disagree on behaviour: - embeddedos-org#104 verifies the image signature at install unconditionally, before the anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and test_fw_transport's XMODEM install can no longer finalize. Both suites now stream genuinely signed images. eBoot has no Ed25519 signer in C, so tools/gen_fw_update_test_sigs.py signs the exact header prefixes those suites build under the RFC 8032 section 7.1 TEST 1 key and emits tests/vectors/fw_update_test_sigs.h; the suites serve that key from a simulated OTP slot 0. Negative control: one flipped signature byte fails test_write_streams_tlv_then_finalize_rejects_below_floor with EOS_ERR_SIGNATURE. - embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot() returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The fixture now places its image in slot A. Guards from embeddedos-org#95 that later merges walked back, never run until now because the C configure step failed first: - embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one. Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites had no list(APPEND ...) and so no Valgrind run. - Seven suites assign tests_run = <literal> and their TEST() does not count; four suites have no TEST() macro at all. Counted, and classified. - embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it. CI plumbing: - eosim-sanity.yml: the install-validate job is written in bash but ran under PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an unknown command and `|| { exit 1 }` is an unexecuted script block. - scorecard.yml: ossf/[email protected] pulls gcr.io, which now demands GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green. Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest. Not fixed here, reported separately: core/keystore.c's compiled-in default_dev_key is described as the RFC 8032 TEST 1 public key but differs from byte 21 on and is not a point on the curve, so nothing can verify against it on any board without OTP. With embeddedos-org#104 that makes firmware update refuse every image on such boards.
|
All six red checks here are inherited from
before anything is compiled — which is why every build-side check fails Both are repaired by #115 ( This PR removes the duplicate target, which is half of the repair — the same which is |
Twenty PRs were merged into master in ninety minutes on 09-07, each on the base it was written against. Master has not configured, compiled, linked or passed its tests since. Every nightly and every eos simulation run (which builds eBoot master) has been red for the same reason. Configure: - tests/CMakeLists.txt registered eboot_test_fdt_loader twice. embeddedos-org#84 added it, and embeddedos-org#85 -- stacked on embeddedos-org#84 -- re-added it at a different anchor when it was replayed onto a master that already had embeddedos-org#84. Same hunk as embeddedos-org#111. Compile (eboot_core): - embeddedos-org#55 restored a core/sha512.c from 02b7dac that keeps its byte count in ctx->count; the eos_sha512_ctx_t master ships has bitlen[2]/buffer_len (embeddedos-org#69/embeddedos-org#93). Back to the pre-embeddedos-org#55 file, blob-identical (d9aa57c). - embeddedos-org#55's source-list "correction" dropped core/boot_log.c, core/secure_boot.c and core/fdt_loader.c, which embeddedos-org#72/embeddedos-org#84 had added after embeddedos-org#55 was written. CMakeLists.txt is blob-identical to pre-embeddedos-org#55 again (f8fe6eb). - embeddedos-org#55 replaced the eos_boot_log_get_head() declaration with a second copy of eos_boot_log_read(); embeddedos-org#91 had already fixed the prototype it meant to fix. Header restored (964ebb8). - embeddedos-org#94 and embeddedos-org#105 each repaired the Ed25519 verifier and each added an identical static scalarbase(); both merged. One copy removed. - The same pair each added k_low_order[]/messages[] to test_ed25519.c. The embeddedos-org#105 copy is removed; embeddedos-org#94's stays because it also carries k_non_canonical[]. Tests that stopped passing because two merged PRs disagree on behaviour: - embeddedos-org#104 verifies the image signature at install unconditionally, before the anti-rollback check embeddedos-org#103 added, so embeddedos-org#103's unsigned images are refused as EOS_ERR_SIGNATURE before they can be refused as EOS_ERR_ANTI_ROLLBACK, and test_fw_transport's XMODEM install can no longer finalize. Both suites now stream genuinely signed images. eBoot has no Ed25519 signer in C, so tools/gen_fw_update_test_sigs.py signs the exact header prefixes those suites build under the RFC 8032 section 7.1 TEST 1 key and emits tests/vectors/fw_update_test_sigs.h; the suites serve that key from a simulated OTP slot 0. Negative control: one flipped signature byte fails test_write_streams_tlv_then_finalize_rejects_below_floor with EOS_ERR_SIGNATURE. - embeddedos-org#103's step 5b reads the TLV counter through the HAL slot containing the image; test_secure_boot_policy (embeddedos-org#82) declared no slots, so eos_secure_boot() returned EOS_SBOOT_ERR_BAD_HEADER two steps before the one under test. The fixture now places its image in slot A. Guards from embeddedos-org#95 that later merges walked back, never run until now because the C configure step failed first: - embeddedos-org#103 replayed the hand-written Valgrind foreach over the derived one. Restored foreach(TEST_NAME ${EBLDR_UNIT_TESTS}); eleven registered suites had no list(APPEND ...) and so no Valgrind run. - Seven suites assign tests_run = <literal> and their TEST() does not count; four suites have no TEST() macro at all. Counted, and classified. - embeddedos-org#101 added fuzz-build after embeddedos-org#90's gate; the gate did not wait for it. CI plumbing: - eosim-sanity.yml: the install-validate job is written in bash but ran under PowerShell on the Windows legs (no shell:), where SITE_PACKAGES=$(...) is an unknown command and `|| { exit 1 }` is an unexecuted script block. - scorecard.yml: ossf/[email protected] pulls gcr.io, which now demands GCP billing. v2.4.3 pulls ghcr.io; eos already pins it and is green. Verified locally (macOS, clang): Release build clean, 31/31 ctest; the same under -DEBLDR_SANITIZE=ON (ASan+UBSan); 78/78 pytest. Not fixed here, reported separately: core/keystore.c's compiled-in default_dev_key is described as the RFC 8032 TEST 1 public key but differs from byte 21 on and is not a point on the curve, so nothing can verify against it on any board without OTP. With embeddedos-org#104 that makes firmware update refuse every image on such boards.
Summary
eboot_test_fdt_loadertarget andtest_fdt_loaderregistration fromtests/CMakeLists.txt.Testing
cmake -S . -B build/host -DEBLDR_BUILD_TESTS=ON— passed.core/ed25519_verify.candcore/sha512.c.