Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

glob expansion should ignore files that cannot be stat()'ed #1674

Closed
krader1961 opened this issue Mar 16, 2023 · 4 comments
Closed

glob expansion should ignore files that cannot be stat()'ed #1674

krader1961 opened this issue Mar 16, 2023 · 4 comments

Comments

@krader1961
Copy link
Contributor

@hanche noticed that stat /var/db/DifferentialPrivacy fails with error "operation not permitted". This is because of a macOS security mechanism known as SIP. This causes Elvish glob expansion to short-circuit when it is unable to stat() the file:

~> cd /var/db
> put *
▶ Accessibility
▶ BootCache.data
▶ BootCache.playlist
▶ BootCaches
▶ CVMS
▶ ConfigurationProfiles
▶ CoreDuet
▶ DetachedSignatures
▶ DiagnosticPipeline

This is related to issue #1240 that I fixed almost a year ago. It didn't occur to me at the time that there could be scenarios, short of a broken filesystem, that stat() could fail. The simplest solution is to silently ignore any file that cannot be stat()'ed. However, in the context of a simple glob expansion we should still be able to include the problematic path name in the generated output.

@krader1961
Copy link
Contributor Author

I've got a tentative fix for the glob expansion problem involving files that are protected by macOS SIP. With my change I can execute put /var/db/* and enumerate every file in that directory; including /var/db/DifferentialPrivacy. It even passes all the existing unit tests. However, this is a pretty big change to the behavior of glob expansion so I want to cogitate on my fix for a few days before opening a pull-request. In particular, it's not at all obvious how to verify the change since this is a corner case that is hard to simulate with the current Elvish unit test infrastructure.

With my fix this now works:

~> put /var/db/*
▶ /var/db/Accessibility
▶ /var/db/BootCache.data
▶ /var/db/BootCaches
▶ /var/db/CVMS
▶ /var/db/ConfigurationProfiles
▶ /var/db/CoreDuet
▶ /var/db/DiagnosticPipeline
▶ /var/db/DiagnosticsReporter
▶ /var/db/DifferentialPrivacy
▶ /var/db/DuetActivityScheduler
...
▶ /var/db/vmware
▶ /var/db/volinfo.database

@krader1961
Copy link
Contributor Author

I'll also note that by "pretty big change" I do not mean the number of lines changed by my fix. That is actually quite small. It's a big change in the sense that glob expansion now ignores EPERM errors (on Unix) from os.Lstat() and it's unclear what the equivalent is on Windows.

@krader1961
Copy link
Contributor Author

On Windows fs.ErrPermission corresponds to ERROR_ACCESS_DENIED (defined in https://pkg.go.dev/golang.org/x/sys/windows). This does seem to be directly analogous to EPERM on Unix platforms and is probably safe to use in the same manner on Windows. It is still unclear whether there are other corner cases involving that error value but I am satisfied that special-casing it in the Elvish glob expansion code is justified and safe to do.

TBD is how to augment the existing glob unit tests to verify the new handling of a "permission denied" error returned by os.Lstat(), or even if that complication is justified.

@krader1961
Copy link
Contributor Author

I'm glad I slept on my solution and tried to find ways to break it. I expected os.Lstat() to return a "zero value" fs.FileInfo structure if an error (such as EPERM) occurred. However, it instead returns a nil pointer because fs.FileInfo is an interface, not a structure. Which means that passing the value, when an error is reported, to the callback can panic when it receives an unexpected nil pointer. Such as when doing put /var/db/*[type:regular]. My expectation was that a "zero valued" fs.FileInfo structure would simply fail most checks (such as whether the path was a regular file or directory) if the os.Lstat() call returned an error and therefore was safe to use. /facepalm

So fixing this is going to be slightly more complicated than I had expected.

krader1961 added a commit to krader1961/elvish that referenced this issue Mar 23, 2023
Specifically, ignore EPERM errors returned by os.Lstat() since

a) that is not documented as a valid error on any system I currently have
access to (which includes macOS, Linux, and FreeBSD), and

b) on macOS EPERM is only returned due to security restrictions on gathering
information about the path and which should otherwise not short-circuit
glob expansion by treating it as a fatal error.

We only care about EPERM if there is explicit filtering of the glob
expansion; e.g., `put **[type:regular]`. Otherwise we should include the path
name in the results and not prematurely terminate inclusion of glob matches.

Fixes elves#1674
krader1961 added a commit to krader1961/elvish that referenced this issue Mar 23, 2023
While working on the fix for issue elves#1674 I noticed that most, but not all,
uses of testutil.Set() explicitly modified a pointer to a function being
mocked. However, there are a few cases that use a different pattern.
This changes those cases so future readers of the code aren't wondering
why there are two patterns for using testutil.Set(). As a bonus this allows
removing one source file that exists only to enable the alternative pattern
being eliminated.

This change also includes a few renaming changes that aren't strictly
speaking necessary (e.g., `osExit` => `OSExit`) but are included for
consistency with similar cases.
@xiaq xiaq closed this as completed in bda2879 Apr 10, 2023
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 a pull request may close this issue.

1 participant