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

Catch any os.Stat error #540

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

spenserblack
Copy link
Contributor

tl;dr I discovered some weird Windows behavior where an invalid path causes scc.exe to panic. Surprisingly, the error is not a "does not exist" error. After some thought it seemed better to exit on any non-nil error.

In PowerShell, I tried to run scc.exe *.go, and to my surprise I got a panic. PowerShell did not expand the glob -- that's not scc's fault, although potentially glob expansion can be added as a feature if that's reasonable. But the issue is that, on Windows, apparently this does not raise a "does not exist" error. So os.Stat() returns nil, err where err is not nil. So the call to s.IsDir() raises a nil pointer dereference error.

I did some playing around using the code below:

package main

import "os"
import "path/filepath"
import "fmt"
import "io/fs"

func main() {
	arg := "."
	if len(os.Args) > 1 {
		arg = os.Args[1]
	}
	arg = filepath.Clean(arg)
	fmt.Println("statting", arg)
	s, err := os.Stat(arg)
	fmt.Printf("s = %v, err = %v\n", s, err)

	checks := []struct {
		name  string
		check func(err error) bool
	}{
		{
			name:  "IsNotExist",
			check: os.IsNotExist,
		},
		{
			name:  "IsExist",
			check: os.IsExist,
		},
		{
			name:  "IsPermission",
			check: os.IsPermission,
		},
	}

	for _, c := range checks {
		fmt.Println(c.name, c.check(err))
	}

	if err, ok := err.(*fs.PathError); ok {
		fmt.Println("err.Op =", err.Op)
	}
}

And here's the result of go run .\main.go "?":

statting ?
s = <nil>, err = CreateFile ?: The filename, directory name, or volume label syntax is incorrect.
IsNotExist false
IsExist false
IsPermission false
err.Op = CreateFile

Perhaps unexpectedly, filepath.Clean does not clean the invalid character ?. Also, CreateFile is not the op that I would expect 🤔

At first I wanted to narrow down to exactly the error type, but then I figured that it should be fine to treat any os.Stat error the same, especially with more unusual errors like these.

@boyter boyter merged commit 440b0ef into boyter:master Oct 15, 2024
5 checks passed
@boyter
Copy link
Owner

boyter commented Oct 15, 2024

Now that is a doozy. Thanks so much for the investigation. No idea what I was thinking there, you would have thought I would have caught all errors after checking, but apparently not. Thanks for this and it will make it into the next release.

@spenserblack spenserblack deleted the bugfix/fail-on-any-stat-error branch October 16, 2024 02:14
@spenserblack
Copy link
Contributor Author

To be fair it's pretty hard to find if you don't have a Windows machine. I honestly don't know how I'd trigger it on Linux, since AFAIK any character is valid in a path name, even if it's not valid in a filename. And it's perfectly reasonable to assume that filepath.Clean would return a valid path 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: DONE
Development

Successfully merging this pull request may close these issues.

2 participants