firmware: don't truncate tar content_end on binary payloads

[SwiggMcJigger]

detect_tar_md5_info() finds the block-aligned MD5 trailer correctly, but then unconditionally scanned back through the 64 KiB tail for '\n'. Any 0x0A byte in the preceding TAR payload (common in lz4 images) moved content_end into the archive data, so MD5 verification failed on valid firmware, e.g. A325FXXSCDYB2 (all four archives).

A TAR archive always ends on a 512-byte boundary, so a block-aligned MD5 is the trailer start as-is. The newline scan now runs only for the non-aligned fallback, and never crosses the preceding 512-byte boundary.

The trailer location logic moves into a dependency-free header (src/firmware/tar_md5_trailer.h) so it can be unit tested; behaviour of detect_tar_md5_info() is otherwise unchanged.

Fixes #279

Claude-Session: https://claude.ai/code/session_01HHr8mBtQ7BuQgtfG1QS8iC

Summary

Type of change

• Bug fix
• New feature
• Refactor
• Documentation
• CI / Build

Testing

• Builds successfully
• Manual CLI test
• Device test (if applicable)

Notes

Unit tests: tests/test_tar_md5_trailer.cpp (10 cases), odin4_tests 47/47 pass.

Summary by CodeRabbit

• Bug Fixes
  • Improved detection of MD5 trailers in Samsung .tar.md5 firmware packages, including trailers at aligned or unaligned positions.
  • Firmware package validation continues to check the detected MD5 and report errors when validation fails, supporting consistent checks across different TAR layouts.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 508964a6-760b-432d-a5eb-f06877a09f23

📥 Commits

Reviewing files that changed from the base of the PR and between 6a7bf91 and 2744c49.

📒 Files selected for processing (3)

• src/firmware/firmware_package.cpp
• src/firmware/tar_md5_trailer.h
• tests/test_tar_md5_trailer.cpp

🚧 Files skipped from review as they are similar to previous changes (2)

• src/firmware/firmware_package.cpp
• src/firmware/tar_md5_trailer.h

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds a shared TAR MD5 trailer locator and uses it in firmware package detection. The locator prefers block-aligned MD5 candidates and bounds newline scanning for unaligned candidates. New tests cover candidate positions, payload alignment, and missing or short trailer data.

Changes

TAR MD5 trailer detection

Layer / File(s)	Summary
Trailer location utility src/firmware/tar_md5_trailer.h	Adds tar_md5::TrailerPos, is_hex_char, and locate_trailer. The locator prefers valid 512-byte-aligned MD5 candidates. For unaligned candidates, it bounds its backward newline scan by the preceding 512-byte boundary.
Firmware package detection src/firmware/firmware_package.cpp	Uses the shared hexadecimal predicate and trailer locator to set the MD5 position and trailer start.
Trailer locator regression tests tests/test_tar_md5_trailer.cpp, CMakeLists.txt	Adds tests for aligned and unaligned payloads, nonzero tail offsets, MD5 position, and missing or short trailer data. Adds the test file to the odin4_tests source list.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 2744c

This PR changes TAR MD5 boundary detection to avoid payload newlines truncating the verified range and adds focused alignment and fallback tests. No specific user-facing regression is supported by the supplied context, so merge risk is minimal.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 2744c

The change corrects which archive bytes are checked without an identified weakening of firmware verification. The available evidence does not establish every way an archive might be supplied.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The consequential path is production firmware processing: configured archive paths reach processing for a connected device. Repository evidence does not establish a remotely reachable archive source.

Trust Boundaries and Controls

• observed — The changed locator determines the prefix covered by the existing MD5 check. Aligned candidates use their block boundary, unaligned candidates have a bounded backward search, and verification rejects a zero-length prefix.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR meets the coding requirements in issue #279. tar_md5::locate_trailer() uses a 512-byte-aligned MD5 position directly. It uses the newline scan only for the non-aligned fallback and limits tha…
Out of Scope Changes check	✅ Passed	The changed files support issue #279. The header extracts trailer-location logic for testing. The firmware code integrates that logic. The tests add regression coverage, including uppercase MD5 digits…
Title check	✅ Passed	The title clearly identifies the firmware TAR handling bug and the fix for incorrect content_end truncation with binary payloads.
Description check	✅ Passed	The description explains the defect, the fix, the refactor, the linked issue, and testing results. It includes all required template sections and marks the relevant change and testing items.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[SwiggMcJigger]

@coderabbitai full review

[coderabbitai]

⚠️ Action not completed

Review rate limited.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 41 minutes.

[SwiggMcJigger]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[SwiggMcJigger]

@coderabbitai which functions touched by this PR are counted as missing docstrings for the Docstring Coverage check?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -u
gh pr checks 280 --repo Llucs/odin4 || true
gh api repos/Llucs/odin4/pulls/280/files --paginate --jq '.[] | {filename,status,patch}'

Length of output: 15377

---

🏁 Script executed:

#!/bin/bash
set -u
printf '%s\n' 'REPO MATCHES'
rg -n -i -C2 'docstring.coverage|docstring|documentation.coverage' --glob '!*.lock' . | head -140 || true
printf '%s\n' 'PR DETAILS'
gh pr view 280 --repo Llucs/odin4 --json headRefOid,statusCheckRollup,reviews,comments --jq '{headRefOid, statusCheckRollup, reviews: [.reviews[] | {author:.author.login,body:.body}], comments: [.comments[] | {author:.author.login,body:.body}]}' | head -c 20000
printf '\n%s\n' 'CHECK RUNS'
oid=$(gh pr view 280 --repo Llucs/odin4 --json headRefOid --jq .headRefOid)
gh api "repos/Llucs/odin4/commits/$oid/check-runs" --jq '.check_runs[] | {name,conclusion,details_url,output:.output}' | head -c 30000

Length of output: 13069

---

🤖 web_search executed:

CodeRabbit pre-merge checks Docstring Coverage 77.78 functions touched by diff C++ lambdas documentation comments which functions missing docstrings how calculated

Length of output: 7036

---

@SwiggMcJigger, I found the Docstring Coverage warning in the CodeRabbit PR comment. It reports 77.78% coverage across 18 functions. That means the check counted four functions as missing docstrings, but the report does not name them.

The diff shows two plausible candidates: the hex_at and find_md5 lambdas inside tar_md5::locate_trailer(). Neither has its own documentation comment. I cannot confirm that the check counts those lambdas, or identify the other two functions, from the available report. My earlier reply missed the coverage figures in the PR comment; sorry about that.

You are interacting with an AI system.