fix: skip permission-denied paths instead of crashing on directory walk (#3258)#4880
fix: skip permission-denied paths instead of crashing on directory walk (#3258)#4880tjhub1983 wants to merge 1 commit into
Conversation
…lk (anchore#3258) Signed-off-by: tjhub1983 <tjhub1983@users.noreply.github.com>
|
Hi @anchore/syft-maintainers! This PR fixes #3258 — a crash when scanning directories with excluded paths that contain permission-denied directories (e.g., The CI workflow runs are showing Could you please approve the CI runs for this PR? Happy to make any adjustments needed. Thank you! |
| // file access errors (e.g. permission denied on restricted directories like /boot/grub2) | ||
| // should not stop the walk -- skip and continue, matching filepath.Walk behavior. | ||
| // Only fail for truly unexpected errors (ErrSkipPath and fs.SkipDir are expected/safe). | ||
| if r.isFileAccessErr(targetPath, err) { |
There was a problem hiding this comment.
Is there some way to add a test for this that isn't too convoluted to set up?
| // file access errors (e.g. permission denied on restricted directories like /boot/grub2) | ||
| // should not stop the walk -- skip and continue, matching filepath.Walk behavior. | ||
| // Only fail for truly unexpected errors (ErrSkipPath and fs.SkipDir are expected/safe). | ||
| if r.isFileAccessErr(targetPath, err) { |
There was a problem hiding this comment.
@kzantow Thanks for the review!
Regarding the test: a reliable permission-denied test typically requires OS-level permission manipulation which can be flaky across platforms (especially Windows). The existing unit tests cover the core logic paths (walkWithExclusion, walkDir), but testing the permission-denied scenario at the integration level requires either:
- Using
os.Chmodto make a directory unreadable (platform-dependent, may not work reliably on Windows) - A mock filesystem approach
I'm happy to add a test using approach #1 if you think it's worth the added complexity, or handle it as a follow-up issue. Let me know how you'd like to proceed.
There was a problem hiding this comment.
This needs to have a test. The test can create a filesystem with this scenario.
|
Hi @anchore/syft-maintainers! All CI checks have passed:
Is there anything else needed to move this forward? This PR fixes the crash when scanning directories with excluded paths that contain permission-denied directories (e.g., /boot/grub2 on Linux). |
|
Hi maintainers! All CI checks have passed. Is there anything blocking this from being merged? Happy to address any feedback! |
|
Just checking in again - is there anything else needed to move this forward? All CI checks are green and kzantow reviewed previously. Happy to make any adjustments! @anchore/syft-maintainers |
|
Just checking - is there anything needed from my side to move this forward? |
|
?? Hi maintainers! Just checking in on this PR. Is there anything else needed from my side to move forward? Thanks! |
|
?? Hi maintainers! Just checking in on this PR. Is there anything else needed? Thanks for your time! |
Summary
Fixes #3258.
When scanning a directory with excluded paths, syft would crash if it encountered permission-denied directories (e.g.,
/boot/grub2owned by root) during the ancestor path walk, even when those paths were excluded.Before:
unable to index ancestor path="/boot/grub2": permission denied→ syft exits with errorAfter: Permission-denied paths are skipped and logged, scan continues
The fix adds
r.isFileAccessErr(targetPath, err)check at line 218 ofdirectory_indexer.go. Whenos.Lstatfails with a file access error, the ancestor walk continues instead of returning an error. This matches the behavior offilepath.Walkwhich skips permission errors.Changes
syft/internal/fileresolver/directory_indexer.go: Skip file access errors inindexBranchancestor walk instead of returning an errorTest
Related
This was discussed in syft#3258 with the suggestion that excluded paths should not be accessed at all. While the ideal long-term fix would prevent access to excluded paths entirely, this change provides immediate relief by making permission errors non-fatal.