Skip to content

Floor the temp-name truncation to a char boundary - #18

Open
kridaydave wants to merge 1 commit into
mainfrom
k5/pass-oot-unicode-filename-panic
Open

kridaydave wants to merge 1 commit into
mainfrom
k5/pass-oot-unicode-filename-panic

Conversation

@kridaydave

Copy link
Copy Markdown
Contributor

oot update panics with exit 101 when a stored file's name is longer than 128 bytes and byte 128 falls inside a multi-byte character. The truncation that guards against ENAMETOOLONG sliced a String at a fixed byte index.

Red-first proof, run against pristine origin/main source:

thread 'test_update_long_unicode_filename_does_not_panic' panicked at tests/update_test.rs:690:5:
update must not panic on a long unicode filename:
thread 'main' panicked at src/update.rs:610:19:
end byte index 128 is not a char boundary; it is inside 'é' (bytes 127..129 of string)
test result: FAILED. 0 passed; 1 failed

With the change, that same test binary reports 1 passed; 0 failed.

The trigger is a 129-byte name of 127 ASCII characters plus one 2-byte character. The panic lands inside write_file_atomic, so it fires partway through the write loop and leaves the working copy half-written: on origin/main the long-named file came back MISSING afterwards, and the temp name it had already deleted files to make room for was never written.

Diff

src/update.rs        |  6 ++++--
tests/update_test.rs | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 2 deletions(-)

One line of logic. full_name.floor_char_boundary(128) keeps the same 128-byte budget and lands on a valid boundary instead of panicking.

Not shipped

  • .pem in visibility.toml:14 does not match a real certificate. path_is_private does segment-boundary matching, so .pem only matches a file literally named .pem, not certs/server.pem. Fail-open on the export path. Fix is either the config (*.pem) or a suffix rule in the matcher. Not shipped here because the right answer is a policy decision, and it belongs with the duplicated private-branch matcher below.
  • Private-branch matching exists twice and the copies disagree. visibility.rs:107 branch_is_private uses segment boundaries; visibility.rs:203-212 in check() uses a bare String::contains and does not trim the pattern. Measured: private_branches = ["internal-audit"] gives export_drops=false, adjudicate_flags=true for internal-audit-refactor and hotfix-internal-audit. A trailing space in the config makes adjudication cloak a branch that export publishes.
  • The embargo date is compared in UTC. visibility.rs:247-252 divides epoch seconds by 86400 with no local offset, so the embargo lifts up to 12 hours early in western timezones.

A file name longer than 128 bytes whose 128th byte falls inside a
multi-byte character panicked `oot update` with exit 101, partway
through materializing the tree.
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.

1 participant