Skip to content

erofs: Validate directory '.'/'..' entries and nlink counts#272

Merged
cgwalters merged 1 commit intocomposefs:mainfrom
cgwalters:erofs-more-validation
Mar 18, 2026
Merged

erofs: Validate directory '.'/'..' entries and nlink counts#272
cgwalters merged 1 commit intocomposefs:mainfrom
cgwalters:erofs-more-validation

Conversation

@cgwalters
Copy link
Collaborator

When going over dumpfile canonicalization related to hardlinks, it made me think about the potential for skew between EROFS nlink on inode vs what's actually present.

This then led to: our reader should be enforcing this matches. And while there, my agent also pointed out we could be checking ./.. among other things.

The previously unused InvalidSelfReference and InvalidParentReference error variants are now wired up.

Assisted-by: OpenCode (Claude Opus 4)

@cgwalters cgwalters requested a review from giuseppe March 18, 2026 13:06
@cgwalters
Copy link
Collaborator Author

Man, the flakes on c9s are annoying. That's tracked by bootc-dev/bcvk#153 ...will investigate

@cgwalters cgwalters enabled auto-merge March 18, 2026 16:20
@jeckersb
Copy link
Collaborator

Hm I consistently get these two failures when running the unit tests locally:

---- erofs::reader::tests::test_rejects_corrupted_directory_nlink stdout ----

thread 'erofs::reader::tests::test_rejects_corrupted_directory_nlink' (222124) panicked at crates/composefs/src/erofs/reader.rs:2439:13:
fsck.erofs should also reject corrupted directory nlink
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- erofs::reader::tests::test_rejects_corrupted_nlink stdout ----

thread 'erofs::reader::tests::test_rejects_corrupted_nlink' (222126) panicked at crates/composefs/src/erofs/reader.rs:2387:13:
fsck.erofs should also reject corrupted nlink


failures:
    erofs::reader::tests::test_rejects_corrupted_directory_nlink
    erofs::reader::tests::test_rejects_corrupted_nlink

test result: FAILED. 120 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.46s

Haven't looked further to see what that's about.

giuseppe
giuseppe previously approved these changes Mar 18, 2026
Copy link
Collaborator

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cgwalters cgwalters added this pull request to the merge queue Mar 18, 2026
@giuseppe giuseppe removed this pull request from the merge queue due to a manual request Mar 18, 2026
@giuseppe
Copy link
Collaborator

I've removed the PR from the merge queue because of #272 (comment)

@giuseppe
Copy link
Collaborator

@cgwalters if you think it is fine to merge, I am fine with it

@cgwalters cgwalters force-pushed the erofs-more-validation branch 2 times, most recently from a050065 to 8e05e09 Compare March 18, 2026 20:46
@cgwalters
Copy link
Collaborator Author

cgwalters commented Mar 18, 2026

Hm I consistently get these two failures when running the unit tests locally:

Thank you for looking at this! Indeed the devcontainer doesn't have erofs-utils installed and neither does the CI env...and my bad for not noticing this drift!

And now that I do...it turns out that while fsck.erofs does catch missing ./.. it doesn't catch incorrect nlink, which seems like a pretty basic thing for a fsck implementation to miss. But OTOH it matters a lot less for a readonly filesystem...

@cgwalters cgwalters force-pushed the erofs-more-validation branch from 8e05e09 to 2709af0 Compare March 18, 2026 20:53
When going over dumpfile canonicalization related to hardlinks,
it made me think about the potential for skew between EROFS
nlink on inode vs what's actually present.

This then led to: our reader should be enforcing this matches.
And while there, my agent also pointed out we could be checking `.`/`..`
among other things.

The previously unused InvalidSelfReference and InvalidParentReference
error variants are now wired up.

Assisted-by: OpenCode (Claude Opus 4)
Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters cgwalters force-pushed the erofs-more-validation branch from 2709af0 to c1a10b6 Compare March 18, 2026 20:54
@cgwalters cgwalters enabled auto-merge March 18, 2026 20:55
@cgwalters cgwalters added this pull request to the merge queue Mar 18, 2026
Merged via the queue into composefs:main with commit 4cd19bc Mar 18, 2026
20 checks passed
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.

3 participants