Skip to content

fix: repair master — #86, #87 and #57 merged clean and do not compile - #97

Closed
Kartikey1306 wants to merge 1 commit into
embeddedos-org:masterfrom
Kartikey1306:fix/master-header-asserts-follow-reserved-split
Closed

fix: repair master — #86, #87 and #57 merged clean and do not compile#97
Kartikey1306 wants to merge 1 commit into
embeddedos-org:masterfrom
Kartikey1306:fix/master-header-asserts-follow-reserved-split

Conversation

@Kartikey1306

Copy link
Copy Markdown
Contributor

eBoot master (22d8f8b) does not build. Three separate collisions, all from my #86/#87 landing alongside #57. Every one of those merges was conflict-free; none of them was correct.

This is also what is failing eos CI — Cross-compile ARM64 kernel and Full-stack integration summary on eos PRs reach this build through eos-simulation.yml.

1. include/eos_image.h asserts on a field that no longer exists

include/eos_image.h:135: error: no member named 'reserved' in 'eos_image_header_t'
include/eos_image.h:142: error: no member named 'reserved' in 'eos_image_header_t'

#87 pinned every header field, reserved[] among them. #57 then split reserved[30] into tlv_len (2) + tlv_hash (28). Both landed, and git had no reason to object — they touch different lines of the same file.

Offset 62 is already pinned by #57's tlv_len assert, so the old reserved[] offset assert is deleted rather than renamed; keeping both would pin one byte range twice. The width guard follows the 30 bytes into their two successors instead, which is what it existed to do:

EOS_IMG_STATIC_ASSERT(sizeof(tlv_len) + sizeof(tlv_hash) == 30, ...);
EOS_IMG_STATIC_ASSERT(sizeof(tlv_hash) == 28, ...);

2. core/ed25519_verify.c has two identical point_is_identity()

#86 and #57 each added one, byte-for-byte the same, plus equivalent subgroup checks under different names. Kept #57's public_key_is_valid_subgroup() — the one eos_ed25519_verify() actually calls — and #86's fuller comment, which is the one that explains why both conditions are needed and why the identity has to be excluded separately. Dropped #86's now-dead key_has_prime_order().

3. tests/unit/test_ed25519.c kept the tests and dropped their data

test_ed25519_identity_key_forgery_rejected() was defined twice. messages[] and k_low_order[8][32] were dropped while the test that iterates them survived, so the file did not compile. Restored both verbatim from the #86 branch.

Two more surfaced once it built, neither of which any compiler could have caught:

  • test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery() was compiled but never called. The merge kept the function and dropped its run_ call.
  • tests_run was hardcoded to 11 while 12 tests ran, so the suite failed on its own arithmetic (12/11 tests passed, exit 1). It is now incremented by the macro that runs each test, so it cannot go stale again.

Validation

master this PR
header-only TU 2 errors compiles
cmake && ninja 3 errors clean
ctest cannot build 21/21 passed
test_ed25519 13/13 (was 12/11 → exit 1)
low-order forgery probe 0 accepted / 2048 attempts
RFC 8032 vectors all still accepted

The last two matter together: the forgery probe confirms consolidating the two subgroup checks did not weaken the rejection, and the RFC vectors confirm it did not start over-rejecting. A verifier that refused everything would pass the first alone.

Worth saying plainly

This is the third time in this repo that a clean merge has produced code that does not compile, and my PRs are half of this one. The tests_run case is the one I would flag hardest: a test that is compiled but never called is invisible to the build, to ctest, and to the registration guard, which checks for files rather than call sites. eos has a live instance of the same class (test_queue_send_waiter_overflow).

🤖 Generated with Claude Code

…dos-org#57 merged clean and do not compile

eBoot master (22d8f8b) does not build. Three separate collisions, all
from my embeddedos-org#86/embeddedos-org#87 landing alongside embeddedos-org#57. Each merge was conflict-free; none
of them was correct.

1. include/eos_image.h — asserts on a field that no longer exists.
   embeddedos-org#87 pinned every header field, reserved[] among them. embeddedos-org#57 then split
   reserved[30] into tlv_len (2) + tlv_hash (28). Both landed:

     error: no member named 'reserved' in 'eos_image_header_t'  (x2)

   Offset 62 is already pinned by embeddedos-org#57's tlv_len assert, so the old
   reserved[] offset assert is deleted rather than renamed — keeping both
   would pin one byte range twice. The width guard follows the 30 bytes
   into their two successors instead, which is what it was there to do.

2. core/ed25519_verify.c — two identical point_is_identity().
   embeddedos-org#86 and embeddedos-org#57 each added one, byte-for-byte the same, plus equivalent
   subgroup checks under different names. Kept embeddedos-org#57's
   public_key_is_valid_subgroup(), which is the one eos_ed25519_verify()
   actually calls, and embeddedos-org#86's fuller comment, which is the one that
   explains why both conditions are needed. Dropped embeddedos-org#86's now-dead
   key_has_prime_order().

3. tests/unit/test_ed25519.c — the merge kept tests and dropped their data.
   test_ed25519_identity_key_forgery_rejected() was defined twice.
   messages[] and k_low_order[8][32] were dropped while the test that
   iterates them survived, so the file did not compile. Restored both
   verbatim from the embeddedos-org#86 branch.

   Two more, found once it built:
   - test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery() was
     compiled but never called. The merge kept the function and dropped
     its run_ call, which no build error can catch.
   - tests_run was hardcoded to 11 while 12 tests ran, so the suite
     failed on its own arithmetic. It is now incremented by the macro
     that runs each test, so it cannot go stale again.

Verified:
  header TU compiles                    (2 errors on master)
  cmake + ninja                          clean (3 errors on master)
  ctest                                  21/21 passed
  test_ed25519                           13/13 (was 12/11 -> exit 1)
  low-order forgery probe                0 accepted / 2048 attempts
  RFC 8032 vectors                       all still accepted

This is the third time in this repo that a clean merge produced code
that does not compile. The tests_run case is the one worth noting: a
test that is compiled but never called is invisible to the build, to
ctest, and to the registration guard, which checks files rather than
call sites.
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

Closing in favour of #94, which is the same repair, opened eight hours earlier, and better in every place the two differ. My fault for not checking for an existing PR before opening this.

I verified #94 independently rather than assuming, building it from refs/pull/94/head:

cmake + ninja            clean
ctest                    21/21 passed
low-order forgery probe  0 accepted / 2048 attempts
RFC 8032 vectors         all still accepted

Where they differ, #94 wins:

#94 this PR
TEST() blocks / run_ calls 14 / 14 13 / 13
header width asserts sizeof(tlv_len) == 2 and sizeof(tlv_hash) == 28 sizeof(tlv_len) + sizeof(tlv_hash) == 30, plus tlv_hash == 28

Pinning the two widths individually implies the sum; pinning the sum plus one part is the weaker of the two statements. And #94 carries one more test than I did.

Both of us independently found the same three collisions (reserved[] vs the tlv_len/tlv_hash split, the duplicate point_is_identity(), and test_ed25519.c keeping tests while dropping messages[]/k_low_order[]), and both landed on tests_run++ in the macro rather than a hardcoded total. Nothing in this PR is lost by closing it.

The point from this PR's description that is worth carrying over to #94's review, since it is about the class rather than the diff: test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery() was compiled but never called — the merge kept the function and dropped its run_ call. No compiler, no linker, and no ctest run can see that, and test_cmake_test_registration.py cannot either, because it checks that each .c file has an add_executable, not that each TEST(x) has a matching run_x(). #94's tests_run++ change closes the arithmetic half of it. The symbol half is worth a follow-up guard in both repos — eos has a live instance in test_queue_send_waiter_overflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants