fix: restore eboot_core buildability broken by unresolved merge conflicts - #55
Conversation
…icts master currently fails to build eboot_core. Bisecting the recent merge history turned up four independent, pre-existing regressions, all traceable to conflict-resolution mistakes in earlier merged PRs: 1. core/recovery.c: recovery_handle_write() declared slot_size twice in the same scope, introduced when PR embeddedos-org#33 replaced a working call to eos_recovery_write_in_range() with an inline reimplementation, not noticing slot_size was already declared earlier in the function. 2. include/eos_image.h: eos_crc32()'s declaration did not match its definition or its own doc comment (wrong return type and parameter count) - a stale/mistyped prototype, unrelated to any merge. 3. core/ed25519_verify.c: PR embeddedos-org#46's merge commit (19c7e72) resolved a conflict between two independently-written Ed25519 implementations by mixing incompatible halves - keeping the setup from master's gf-based implementation but pasting in the PR branch's ge25519_p3-based comparison logic, calling an undefined function (sc_reduce) and leaving "diff" undefined at the return statement. Restored master's original, self-contained verification tail (scalarmult/scalarbase/point_add/ point_pack + byte-compare), whose helper functions were still present in the file but orphaned since the bad merge. 4. core/sha512.c + CMakeLists.txt: the same PR embeddedos-org#46 replaced a working eos_sha512_init/update/final implementation (matching the API eos_crypto_boot.h declares and ed25519_verify.c depends on) with an incompatible, differently-named implementation (sha512_init, etc., under a new eos_sha512.h). The subsequent merge-conflict resolution then dropped core/sha512.c and core/rollback.c from eboot_core's source list entirely, and left core/boot_log.c listed twice. Restored the pre-conflict core/sha512.c from commit 02b7dac (confirmed to implement the API actually depended upon), removed the now-dead eos_sha512.h, and corrected the library's source list. 5. include/eos_boot_log.h: eos_boot_log_read()'s declaration was stale (unmodified since v0.1.0) and did not match the real implementation in core/boot_log.c or its caller in core/recovery.c, both of which use a single-index read. Corrected the declaration and doc comment to match. Each finding was verified independently: bisected against origin/master, confirmed with cc -fsyntax-only before attributing a fix, and cross-checked against pre-merge commits (02b7dac) where a working prior version existed, rather than guessing at intended behavior. Testing: - eboot_core now compiles and links cleanly. - test_recovery, test_image_verify, test_ed25519 all pass. - test_ed25519 in particular is the load-bearing check: it exercises the restored point-addition/comparison logic end-to-end, not just compilation. Known pre-existing, unrelated issues NOT addressed here (filed separately): - tests/unit/test_slot_manager.c: ~20 test functions are called from main() but never defined, introduced by PR embeddedos-org#37's incompletely-applied patch. - tests/unit/test_boot_log.c: defines a local mock of eos_boot_log_read() that conflicts with the real implementation once linked against eboot_core - a duplicate-symbol issue requiring redesign, not a one-line fix.
Review — verified, and this should merge first
Built this branch merged: configure and build both succeed, exit 0. So this is not an improvement to a working tree — it repairs a repository whose default branch cannot be built at all. That makes it the highest-priority open PR in this repo by some distance. Two things worth raising alongside it:
Nothing here needs to change for it to merge. Please get this in. |
|
Thanks for the review — chased down both. CI. Confirmed: ADR-011 / SHA-512. I don't have access to ADR-011 itself — it's on
The SHA-256 pattern is real, but it's one layer up: So: purely a build repair with respect to this question. The primitive this PR restores is independently RFC-8032-conformant by the test above; the pre-hash-vs-raw-message design choice ADR-011 is presumably about lives in a different function this PR never touches. #57 / #58. Both are fixing overlapping ground independently — worth saying so given the CI discussion above, since three people converging on the same red |
srpatcha
left a comment
There was a problem hiding this comment.
Correct diagnosis, and the earliest of the four PRs that independently found this
breakage. It is narrower than #58, which is the one already approved, so I want
to be specific about what the gap is rather than just saying "duplicate".
Every file here is also in #58
CMakeLists.txt
core/ed25519_verify.c
core/recovery.c
core/sha512.c
include/eos_boot_log.h
include/eos_image.h
Six files, zero unique to this PR.
What it leaves behind
Built standalone:
60 build errors
81% tests passed, 3 tests failed out of 16
test_slot_manager.c:39:5: error: 'erase_result' undeclared
test_slot_manager.c:40:5: error: 'erased_addr' undeclared
test_slot_manager.c:46:36: error: 'SLOT_A_ADDR' undeclared
The following tests FAILED:
11 - test_slot_manager (Not Run)
12 - test_boot_log (Not Run)
eboot_core builds — the title's claim holds — but test_slot_manager.c and
test_boot_log.c are broken independently of it, by the same merge, and are
not repaired here. Those two are tracked as #56. #58 fixes them, which is why it
reaches 16/16.
Nothing wrong with the scope you chose; the merge left several separable
breakages and you fixed one. It just means this cannot land on its own without
leaving the suite red.
Recommendation
Close in favour of #58. If any hunk here resolves something differently and
better than #58 does, point at it and I will fold it across with attribution —
that is what happened with #93's and #94's comment improvements in eos, and with
#94's sync.c wording.
There is one thing in this area genuinely worth picking up, if you want it:
core/ed25519_verify.c on both this branch and #58 accepts a forged
signature over any message when the public key is a low-order point:
identity_pub[32] = {1} /* identity point */
identity_sig[64] = {1} /* R = identity, S = 0 */
eos_ed25519_verify(...) -> rc=0, ACCEPTED
A complete secure-boot bypass, filed as #73. #57 fixes it with a subgroup check.
Neither this PR nor #58 has one — worth knowing, since the build repair is the
part everyone converged on and the crypto is the part that actually matters.
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#55 "fix: restore eboot_core buildability broken by unresolved merge conflicts"
head: 9fbfc91 author: tejasmhadgut ci: none in bundle
Verdict: The work was right and the diagnosis was right. It has since been overtaken
— all five defects are fixed on master, so merging this now would conflict in five
files and revert later work rather than repair anything. This should be closed, not
rebased.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | (whole PR) | Premise no longer holds. The PR exists because master did not build. It does now, and each of the five named defects is independently fixed upstream — see the table below. Merging would resolve five conflicts in favour of a six-day-old tree. |
Close with credit. The repair landed; the branch is the wrong vehicle for what is left. |
| 2 | Medium | core/sha512.c |
git merge-tree conflicts here. master's file is 252 lines with explicit load_be64() / store_be64() helpers (core/sha512.c:98, :110); this branch's is 155 lines. Both are byte-order-explicit and both look correct, but resolving toward this branch drops a later independent rewrite for no functional gain. |
If any part of this branch is salvaged, do not take sha512.c. |
| 3 | Medium | CMakeLists.txt, core/ed25519_verify.c, core/recovery.c, include/eos_boot_log.h |
The other four conflicts. CMakeLists.txt alone differs by 160 lines against master. The static function set in ed25519_verify.c is now identical between the two, so there is nothing left to recover there. |
Same — no salvage needed. |
Status of each of the five defects on current master (13a7a02)
| # | Defect as filed | State on master |
Evidence |
|---|---|---|---|
| 1 | recovery.c redefines slot_size |
Fixed | core/recovery.c:281 and :317 are separate functions; one declaration each |
| 2 | eos_crc32() prototype mismatch |
Fixed | include/eos_image.h:149 uint32_t eos_crc32(uint32_t addr, size_t len), plus a new eos_crc32_checked() at :135 |
| 3 | ed25519_verify.c mixed halves, undefined sc_reduce/diff |
Fixed | no sc_reduce; diff declared at core/ed25519_verify.c:143 and :442 |
| 4 | sha512.c/rollback.c dropped from eboot_core, boot_log.c listed twice |
Fixed | both present in the eboot_core list; boot_log.c appears once |
| 5 | eos_boot_log_read() stale declaration |
Fixed | include/eos_boot_log.h:72 is the single-index form |
master reached this through b0b2ab9 ("repair three merge regressions left by today's
PR flurry"), 038f624, b7e07d4 and others. Two items from the PR's own Limitations
section are also resolved: test_slot_manager and test_boot_log are both registered
and running on master (20 add_test entries in tests/CMakeLists.txt).
The CMake source list is the one place where something from this branch is still owed —
master still registers core/sha512.c and core/rollback.c twice and omits
core/fdt_loader.c entirely. That is #57's and #84's territory now, not this one's.
Architecture conformance
Conformed. §21 Tier 1 (eBoot, Foundation); a build repair inside the owning repo, no
dependency edges added, §5.1 untouched. The ADR-011 question raised in review was
answered correctly in-thread: the restoration touched eos_ed25519_verify(), which
hashes with real SHA-512, and not core/crypto_boot.c's SHA-256 pre-hash wrapper —
core/crypto_boot.c is still untouched by this branch.
Proposed changes
Close. If the maintainers would rather not lose the branch, the only content still
missing from master is the two duplicate CMake entries, and #57 already carries that
change with a regression test attached — reviewed there.
Not checked
- I did not build anything. The brief forbids checking out or otherwise disturbing
the repos, andeBoot's working tree is currently onfix/ed25519-low-order-keys.
Every statement above is fromgit show/git merge-treeagainstorigin/master
andrefs/pull/55/head. The claim "masterbuilds now" is inferred from the five
defects being individually absent, not verified by a compile. checks.txtis empty for this bundle; I have no CI signal for this head.- The PR's Limitations note about undiscovered instances of the same bad-merge pattern
elsewhere incore/— I did not audit for those either.
Automated architecture review of 9fbfc919e6fa — 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.
Issue
mastercurrently fails to buildeboot_core. Bisecting the recent merge historyturned up five independent, pre-existing regressions, all traceable to
conflict-resolution mistakes in earlier merged PRs — not something introduced by
this branch.
core/recovery.c—recovery_handle_write()declaresslot_sizetwice inthe same scope. Introduced when PR Fix OOB flash write and OOB stack read in recovery firmware-update path #33 replaced a working call to
eos_recovery_write_in_range()with an inline reimplementation, without noticingslot_sizewas already declared earlier in the function.include/eos_image.h—eos_crc32()'s declaration didn't match its owndefinition or doc comment (wrong return type, wrong parameter count). A stale
prototype, unrelated to any merge.
core/ed25519_verify.c— PR Add SHA-512 implementation and Ed25519 RFC 8032 verification #46's merge commit (19c7e72) resolved aconflict between two independently-written Ed25519 implementations by mixing
incompatible halves: it kept the setup from
master'sgf-based implementationbut pasted in the PR branch's
ge25519_p3-based comparison logic, calling anundefined function (
sc_reduce) and leavingdiffundefined at the returnstatement. This doesn't compile — but if it had (e.g. via a naive
diff = 0patch), it would have silently accepted every signature.
core/sha512.c+CMakeLists.txt— the same PR Add SHA-512 implementation and Ed25519 RFC 8032 verification #46 replaced a workingeos_sha512_init/update/finalimplementation (the APIeos_crypto_boot.hdeclares and
ed25519_verify.cdepends on) with an incompatible,differently-named implementation (
sha512_init, etc., under a neweos_sha512.h). The subsequent merge-conflict resolution then droppedcore/sha512.candcore/rollback.cfromeboot_core's source list entirely,and left
core/boot_log.clisted twice.include/eos_boot_log.h—eos_boot_log_read()'s declaration was stale(unmodified since v0.1.0) and didn't match the real implementation in
core/boot_log.cor its caller incore/recovery.c, both of which use asingle-index read.
Approach
recovery.c: removed the duplicate declaration.eos_image.h: correctedeos_crc32()'s prototype to match its definitionand doc comment.
ed25519_verify.c: restored the pre-conflict verification tail(
scalarmult/scalarbase/point_add/point_pack+ byte-compare intodiff),recovered from commit
02b7dac(masterimmediately before the bad merge). Allof that code's helper functions were still present in the file, just orphaned
since the conflict — nothing new was written, only restored.
sha512.c: restored from the same pre-conflict commit, confirmed toimplement the API actually depended upon elsewhere in the codebase. Removed the
now-fully-unused
eos_sha512.h.CMakeLists.txt: addedcore/sha512.candcore/rollback.cback toeboot_core's source list; removed the duplicatecore/boot_log.centry.eos_boot_log.h: corrected the declaration and doc comment to match thereal single-index-read implementation.
Every restoration above is evidence-backed against actual pre-conflict commits
(
02b7dac), not a guess at intended behavior.Testing
test_ed25519is the load-bearing check here — it exercises the restoredpoint-addition/comparison logic end-to-end (not just that it compiles), and passing
confirms the restored implementation is cryptographically correct, not merely
syntactically valid.
git statusis clean against the commit; the working tree used for testing exactlymatches what's in this PR.
Limitations
ctest --test-dir buildrun across everytarget — two pre-existing, unrelated test files are still broken on
masterandare out of scope here (filed as a separate issue: Two test files broken independently of eboot_core's build: test_slot_manager.c, test_boot_log.c #56):
tests/unit/test_slot_manager.c— ~20 test functions are called frommain()but never defined, from PR feat(slot_manager): boot attempt counter with rollback detection + fix fw_update_abort ordering #37's incompletely-applied patch.
tests/unit/test_boot_log.c— defines a local mock ofeos_boot_log_read()that conflicts with the real implementation once linked against
eboot_core(a duplicate-symbol issue needing a real redesign, not a one-line fix).
core/for the same class of bad-mergedamage — I fixed what was needed to get
eboot_corelinking and the directlyaffected tests passing. There may be other, undiscovered instances of the same
pattern elsewhere in the tree.