tests: cover the firmware image TLV parser

[JoaoMorais03]

Contribution after reviewing the project: the mcuboot-style TLV parser in core/image_tlv.c is compiled into eboot_core but had no unit tests. test_image_verify.c includes the header and never calls it.

Issue

Firmware images can carry a TLV area (key hash, SHA-256, nonce, …) after the fixed header. The parser has several behaviors that were untested: magic mismatch, size bounds, 4-byte padding between odd-length entries, truncated entries, and eos_tlv_read_data buffer-too-small. The public header also documented magic mismatch as EOS_ERR_INVALID; the implementation returns EOS_ERR_NOT_FOUND (no TLV area is optional, not malformed).

Approach

Add tests/unit/test_image_tlv.c using the same simulated-flash + eos_hal_init pattern as test_image_verify.c. Register it in tests/CMakeLists.txt (including the valgrind list). Correct the eos_tlv_parse return-code comment in include/eos_image_tlv.h to match the implementation.

Cases:

• NULL ctx → EOS_ERR_INVALID
• bad magic → EOS_ERR_NOT_FOUND
• tlv_total_len above EOS_TLV_MAX_SIZE or smaller than the info header → EOS_ERR_INVALID
• two packed entries (SHA256 + KEYHASH): parse, find, read_data
• 1-byte nonce followed by SHA-256 (padding)
• truncated entry (claimed length past tlv_total_len) does not crash; count stays 0
• NULL args on find/read
• buf_sz < entry->len → EOS_ERR_FULL

Testing

Host unit test, no board port. I did not run CTest here (no local checkout). The test writes packed structs the same way the other flash tests do (native endianness).

Limitations

Does not change parser behavior except the header comment. Truncated entries still return EOS_OK with a partial/empty list (existing policy). Does not cover flash-read failures mid-parse, the 16-entry cap, or eos_tlv_read_data of an entry that did not come from the given ctx.

[Copilot]

🟡 Changes recommended

The updated eos_tlv_parse() documentation still omits a return code (EOS_ERR_FLASH) that the implementation can produce, leaving the public API contract incomplete.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds host-side unit coverage for the firmware image TLV parser (core/image_tlv.c) and aligns the public header documentation with the implementation’s return-code policy for “no TLV area”.

Changes:

• Introduce tests/unit/test_image_tlv.c to exercise TLV parse/find/read paths including alignment, truncation, and buffer-too-small handling.
• Register the new test_image_tlv target in tests/CMakeLists.txt, including Valgrind execution.
• Update include/eos_image_tlv.h documentation to treat magic mismatch as “optional region missing” (EOS_ERR_NOT_FOUND), not “malformed” (EOS_ERR_INVALID).

Review Status

• Status: CHANGES NEEDED
• Current mode: Reviewer
• Completed work: Reviewed all provided diffs for correctness, test harness consistency with existing unit suites, and API doc consistency with implementation.
• Files changed: tests/unit/test_image_tlv.c, tests/CMakeLists.txt, include/eos_image_tlv.h
• Verification: NOT RUN (no CTest/Valgrind execution observed)
• Remaining work: Fix eos_tlv_parse() return-code documentation to include EOS_ERR_FLASH (implementation returns it on TLV-info read failure).
• Known risks: API consumers relying on the header comment may miss EOS_ERR_FLASH handling.
• Assumptions: Host unit tests run on a platform compatible with the project’s existing “packed struct written to simulated flash” approach.
• Recommended next step: Apply the doc fix and run ctest (and optionally Valgrind label) to confirm the new test integrates cleanly.

File summaries

File	Description
tests/unit/test_image_tlv.c	New host unit tests for TLV parsing behaviors (bounds, alignment, truncation, read_data).
tests/CMakeLists.txt	Adds test_image_tlv build + CTest registration and includes it in Valgrind targets.
include/eos_image_tlv.h	Adjusts eos_tlv_parse() return-code documentation for magic mismatch policy.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

The new simulated-flash helpers use overflow-prone bounds checks, and the updated eos_tlv_parse API comment still omits an implemented EOS_ERR_FLASH return path.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

tests/unit/test_image_tlv.c:28

• The bounds check addr + len > SIM_FLASH_SIZE can overflow, allowing out-of-bounds access in the simulated flash backend. Use an overflow-safe range check before indexing into sim_flash.

This issue also appears in the following locations of the same file:

• line 32
• line 39

    if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH;

tests/unit/test_image_tlv.c:32

• The bounds check addr + len > SIM_FLASH_SIZE can overflow, allowing out-of-bounds access in the simulated flash backend. Use an overflow-safe range check before indexing into sim_flash.

    if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH;

tests/unit/test_image_tlv.c:39

• The bounds check addr + len > SIM_FLASH_SIZE can overflow, allowing out-of-bounds access in the simulated flash backend. Use an overflow-safe range check before indexing into sim_flash.

    if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH;

include/eos_image_tlv.h:86

• eos_tlv_parse can also return EOS_ERR_FLASH when the TLV info header cannot be read from flash; the updated return-code comment should document this to match the implementation.

 * @return EOS_OK on success, EOS_ERR_NOT_FOUND if there is no TLV area
 *         (magic mismatch), EOS_ERR_INVALID if the TLV size is out of range.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[srpatcha]

Verified — testing compiled-but-unreachable code is the right instinct

core/image_tlv.c is compiled into eboot_core and had no tests; test_image_verify.c includes the header and never calls it. That is the same category as the GPS parser in eos#83 and the empty modules in EoStudio#23 — code that ships, reads as capability, and is exercised by nothing.

The cases you picked are the ones that matter for a parser fed off-device input: magic mismatch, size bounds, 4-byte padding between odd-length entries, truncated entries, and eos_tlv_read_data with a buffer too small. Padding and truncation are exactly where a hand-written TLV walker goes wrong, and neither is reachable from a well-formed image, so no amount of integration testing would have found them.

The documentation mismatch is a genuinely useful find:

The public header documented magic mismatch as EOS_ERR_INVALID; the implementation returns EOS_ERR_NOT_FOUND (no TLV area is optional, not malformed).

Agreed that the implementation is right and the header was wrong — an absent TLV area is a legitimate image shape, not a malformed one. Fixing the header rather than the code is the correct direction, and it is the kind of divergence that only surfaces when someone writes the test that reads both.

Verification

Merged onto origin/master + #58: configure OK, 0 build errors, ctest 17/17.

Not verified against master alone — master does not build (eos_crc32 conflicting types), which #58 repairs. This needs #58 to land first.

Blocker outside this PR

eBoot sets required_signatures: true on master and commits here are unsigned. That makes this unmergeable regardless of review — a repo policy question I have raised with the maintainer, not something for you to change.