fix: do not walk past parent NUL in make_parents_safely

[SebTardif]

What Problem This Solves

file.write with createParents calls make_parents_safely on the destination parent. The walk started at parent + root_len + 1. When the file sits directly under the configured root (/sd/note.txt becomes parent /sd), that pointer is one byte past the strlcpy NUL.

The loop then reads the uninitialized PATH_MAX tail. If that tail has no later NUL, the next read is past the array. This happens when the card is missing after room_files_register_node_commands already stored the root string, so lstat of the parent fails and createParents tries to rebuild it.

Evidence

On current main, the walk has no length guard:

$ git show upstream/main:components/esp-openclaw-room-node/room_files.c | sed -n '399,416p'
static bool make_parents_safely(char *parent)
{
    size_t root_len = strlen(configured_root);
    for (char *p = parent + root_len + 1; *p != '\0'; ++p) {
        if (*p != '/') continue;
        ...
    }
    if (room_lstat(parent, &st) != 0) return mkdir(parent, 0755) == 0;
    return S_ISDIR(st.st_mode) && !S_ISLNK(st.st_mode);
}

Host compile of that loop on a PATH_MAX heap buffer (/sd plus X fill, no second NUL):

$ cc -std=c11 -Wall -Wextra -Werror /tmp/f002-walk-unfixed.c -o /tmp/f002-walk-unfixed
$ /tmp/f002-walk-unfixed
unfixed: parent_len=3 nul_off=3 start_off=4 first=0x58
unfixed: next read off=1024 is past PATH_MAX after 1020 tail reads

Same loop under AddressSanitizer (original for (*p != '\0') with no bounds check):

$ cc -std=c11 -Wall -Wextra -Werror -fsanitize=address /tmp/f002-walk-asan.c -o /tmp/f002-walk-asan
$ /tmp/f002-walk-asan
AddressSanitizer: heap-buffer-overflow
READ of size 1 at 0x619000000e80
0x619000000e80 is located 0 bytes after 1024-byte region [0x619000000a80,0x619000000e80)
SUMMARY: AddressSanitizer: heap-buffer-overflow

After this patch, room_file_parent_walk_start returns NULL when strlen(parent) <= strlen(root). make_parents_safely skips the walk and only lstat/mkdirs the parent path.

$ cc -std=c11 -Wall -Wextra -Werror /tmp/f002-walk-fixed.c -o /tmp/f002-walk-fixed
$ /tmp/f002-walk-fixed
fixed: walk=skip reads=0 parent_len=3
fixed: mkdir-only created missing root /tmp/f002-root-V7xYmk

$ cc -std=c11 -Wall -Wextra -Werror -I components/esp-openclaw-room-node \
    components/esp-openclaw-room-node/room_file_validation.c \
    components/esp-openclaw-room-node/tests/test_room_file_validation.c \
    -o /tmp/room_file_validation_test
$ /tmp/room_file_validation_test
room file validation tests passed

Why This Change Was Made

The walk is only needed for ancestors deeper than the configured root. When the parent is the root, the existing final lstat/mkdir of parent is the whole job. Starting at root_len + 1 in that case is not a walk; it is an out-of-bounds read.

User Impact

file.write with createParents to a file directly under the public root no longer reads past the parent string when that root directory is missing (unmounted card, path removed after register). Nested parents still walk root_len + 1 onward. Preflight (preflightOnly) is unchanged because it never calls make_parents_safely.

Real behavior proof

• Behavior or issue addressed: make_parents_safely started the createParents walk one byte past the parent NUL when the parent equaled the configured root, scanning the PATH_MAX tail and then past the array.

• Real environment tested: macOS 15, Apple clang, host checkout of openclaw/esp-openclaw-node at upstream/main 6f5c8e8 plus this branch. PATH_MAX is 1024. ESP-IDF is not installed on this machine, so firmware is not flashed here.

• Exact steps or command run after this patch: Compiled the unfixed walk (cc /tmp/f002-walk-unfixed.c and cc -fsanitize=address /tmp/f002-walk-asan.c), then the patched skip plus mkdir-only path (cc /tmp/f002-walk-fixed.c), then the host validation program already wired in CI.

• Evidence after fix: terminal output from the patched tree:

  $ /tmp/f002-walk-unfixed
  unfixed: parent_len=3 nul_off=3 start_off=4 first=0x58
  unfixed: next read off=1024 is past PATH_MAX after 1020 tail reads
  
  $ /tmp/f002-walk-asan
  AddressSanitizer: heap-buffer-overflow
  READ of size 1
  located 0 bytes after 1024-byte region
  SUMMARY: AddressSanitizer: heap-buffer-overflow
  
  $ /tmp/f002-walk-fixed
  fixed: walk=skip reads=0 parent_len=3
  fixed: mkdir-only created missing root /tmp/f002-root-V7xYmk
  
  $ /tmp/room_file_validation_test
  room file validation tests passed

• Observed result after fix: Parent /sd with root /sd no longer starts a walk (0 reads). The missing root is created by the existing mkdir/stat of parent only. AddressSanitizer overflow on the old loop is gone on the patched path.

• What was not tested: Flashing a room-node firmware image and invoking file.write over the gateway against a physically unmounted SD card. Nested parent creation on-device (the host case /sd/a/b still returns walk start parent + 4).

Related

• Walk introduced in #19 (17837ce, 2026-08-07).
• Same class as CWE-125 (out-of-bounds read) on a NUL-terminated path walk.
• Sibling hang fix in this repo: #28 (destroy timeout; different file).

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 7, 2026, 10:38 AM ET / 14:38 UTC.

ClawSweeper review

What this changes

The PR prevents room-node file writes from scanning past the parent path’s terminating NUL when creating a missing storage root, and adds focused helper tests.

Merge readiness

⛔ Blocked before merge - 2 items remain

The fix remains necessary on current main, and no introduced correctness defect was found. The existing request for production-path behavior proof remains unresolved.

Priority: P2
Reviewed head: b769f0142e87262f2472383fe46457584597ff04

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The focused patch is sound by inspection, but the supplied demonstration does not cover its production entrypoint.
Proof confidence	🦐 gold shrimp (3/6)	Needs stronger real behavior proof before merge: The macOS terminal traces demonstrate an extracted-loop repair and mkdir result, but do not exercise room_files.c through registered file.write after root loss; the prior production-path proof request remains outstanding. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs stronger real behavior proof before merge: The macOS terminal traces demonstrate an extracted-loop repair and mkdir result, but do not exercise room_files.c through registered file.write after root loss; the prior production-path proof request remains outstanding. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	6 items	Current main still contains the defect: The walk starts at parent + root_len + 1 without a length guard. The caller copies a root-equal parent into an uninitialized PATH_MAX buffer when the root disappears after command registration, making the reported tail read source-reproducible. Verified introduced repair: The complete pinned base-to-head diff adds a length-checked walk-start helper, uses it in parent creation, and tests root-equal, nested, and null inputs. Existing final directory checks and canonical containment remain intact. Production integration: File commands are registered when the board reports available storage; file.write then reaches resolve_write_path and make_parents_safely. Both changed C files already belong to the production component, and the helper header is private.
Findings	None	None.
Security	None	None.

How this fits together

The room-node file subsystem accepts registered file commands for a board-provided storage root. It validates paths, creates requested parent directories, and writes bounded file content or returns a typed error.

flowchart TD
 A[Registered file write] --> B[Validate path and content]
 B --> C{Parent missing and creation enabled?}
 C -->|Yes| D[Guard parent directory walk]
 D --> E[Check canonical containment]
 C -->|No| E
 E --> F[Write file or return error]

Loading

Before merge

• Add real behavior proof - Needs stronger real behavior proof before merge: The macOS terminal traces demonstrate an extracted-loop repair and mkdir result, but do not exercise room_files.c through registered file.write after root loss; the prior production-path proof request remains outstanding. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Complete next step (P2) - Add after-fix proof from the registered file.write path after storage-root loss. Terminal output or redacted device logs count; screenshots or recordings are welcome when they show the result. Redact credentials, IP addresses, private endpoints, and other private details. Update the PR body to trigger review; if that does not happen, ask a maintainer to comment @clawsweeper re-review.

Agent review details

Security

None.

Review metrics

None.

Technical review

Best possible solution:

Retain the narrow bounds guard and demonstrate safe completion or a typed filesystem error through the production file.write path after storage-root loss.

Do we have a high-confidence way to reproduce the issue?

Yes, from source: after successful storage registration, remove the root and request a direct-child file.write with createParents enabled; current main starts reading beyond the parent NUL. No production reproduction was executed during this read-only review.

Is this the best way to solve the issue?

Yes. The guard removes the invalid walk while preserving nested parent creation, final directory checks, and existing path validation; the added helper follows the component’s existing host-testable validation pattern.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 3294af3aaa94.

Labels

Label justifications:

• P2: This repairs a source-proven invalid memory read in optional file transfer after storage-root loss, with a limited trigger.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The macOS terminal traces demonstrate an extracted-loop repair and mkdir result, but do not exercise room_files.c through registered file.write after root loss; the prior production-path proof request remains outstanding. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Current main still contains the defect: The walk starts at parent + root_len + 1 without a length guard. The caller copies a root-equal parent into an uninitialized PATH_MAX buffer when the root disappears after command registration, making the reported tail read source-reproducible. (components/esp-openclaw-room-node/room_files.c:399, 3294af3aaa94)
• Verified introduced repair: The complete pinned base-to-head diff adds a length-checked walk-start helper, uses it in parent creation, and tests root-equal, nested, and null inputs. Existing final directory checks and canonical containment remain intact. (components/esp-openclaw-room-node/room_file_validation.c:78, b769f0142e87)
• Production integration: File commands are registered when the board reports available storage; file.write then reaches resolve_write_path and make_parents_safely. Both changed C files already belong to the production component, and the helper header is private. (components/esp-openclaw-room-node/esp_openclaw_room_node.c:841, b769f0142e87)
• Supplied proof and review continuity: The complete supplied body, captured under sourceRevision 50c36ada7197a4684391119eb04fa41545841d0b1d0a2d7244e0d5e0751a6427, contains macOS traces of extracted unfixed/fixed loops, a real temporary-directory mkdir, and helper tests. It explicitly excludes firmware file.write execution. The previous completed review at the same head requested the registered production path after root loss; the supplied evidence does not fulfill that request. (b769f0142e87)
• Feature-history routing: Available file history identifies Peter Steinberger’s shared-room-runtime extraction. Supplied GitHub metadata links steipete to the merged runtime PR feat(tab5): add reusable room runtime and Tab5 node #19. Rename-following history and blame encountered unavailable objects, so exact line introduction is not asserted. (components/esp-openclaw-room-node/room_files.c, 17837cee4852)
• Related work and release checks: The supplied merged runtime PR establishes the subsystem, not this fix. The open destroy-timeout and Talk-redirect PRs address different mechanisms. Local tags were empty; live canonical-search and release-list requests failed because GitHub networking was unavailable. No merged fixing PR or shipped fix is established, while inspected current main remains affected.

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Provide after-fix output from registered file.write with createParents after the storage root disappears, showing the returned result and absence of an invalid-memory failure.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (17 earlier review cycles; latest 8 shown)

• reviewed 2026-09-02T15:23:32.397Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-02T22:00:01.885Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-03T08:53:51.210Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-03T15:54:46.838Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-03T16:06:19.820Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-04T02:52:42.247Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-04T12:00:05.556Z sha b769f01 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-05T17:59:07.064Z sha b769f01 :: needs real behavior proof before merge. :: none