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

Fix regression from #314 (chmod missed fix) #330

Merged
merged 3 commits into from
Sep 28, 2022
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix regression from #314 (chmod missed fix)
#314 introduced a [regression](#314 (comment)) that passes quoted paths to `fs.chmod`.
[`fs.Chmod`/`os.chmod` on windows](https://cs.opensource.google/go/go/+/master:src/syscall/syscall_windows.go;l=647;drc=242adb784cd64265ce803f6b0c59dbf126bcda9c) calls into the [`GetFileAttributes`](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfileattributesw)/[`SetFileAttributes`](https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-setfileattributesw) win32 api directly. The win32 api expects a "strongly typed" path, without quotes, unlike a command line string, and doesn't try to *parse* it. Passing quoted paths to fs.Chmod causes the operation to fail - valid paths do not begin with quotes!

This change moves the path quoting down into `runScript`, so that we can pass the UNquoted path to `fs.Chmod`.

To my great surprise, it looks like go does NOT check `GetLastError` when `GetFileAttributes`/`SetFileAttributes` fails? This is disappointing, because somewhere down the stack, procmon says windows returns "NAME INVALID", which is probably `STATUS_OBJECT_NAME_INVALID`. Poking around kernelbase.dll in GHIDRA shows a couple places where it sets last error, and you can see plenty more in the ReactOS sources.
  • Loading branch information
ariccio committed Sep 23, 2022
commit 6c15869b70c7f19bdd6a3643314e69bc90faddb5
15 changes: 8 additions & 7 deletions internal/lefthook/runner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,23 +161,24 @@ func (r *Runner) runScripts(dir string) {
continue
}

scriptPath := shellescape.Quote(filepath.Join(dir, file.Name()))
unquotedScriptPath := filepath.Join(dir, file.Name())

if r.hook.Parallel {
wg.Add(1)
go func(script *config.Script, path string, file os.FileInfo) {
defer wg.Done()
r.runScript(script, path, file)
}(script, scriptPath, file)
r.runScript(script, path, file, dir)
}(script, unquotedScriptPath, file)
} else {
r.runScript(script, scriptPath, file)
r.runScript(script, unquotedScriptPath, file, dir)
}
}

wg.Wait()
}

func (r *Runner) runScript(script *config.Script, path string, file os.FileInfo) {
func (r *Runner) runScript(script *config.Script, unquotedPath string, file os.FileInfo, dir string) {
quotedScriptPath := shellescape.Quote(unquotedPath)
if script.DoSkip(r.repo.State()) {
logSkip(file.Name(), "(SKIP BY SETTINGS)")
return
Expand All @@ -196,7 +197,7 @@ func (r *Runner) runScript(script *config.Script, path string, file os.FileInfo)

// Make sure file is executable
if (file.Mode() & executableMask) == 0 {
if err := r.fs.Chmod(path, executableFileMode); err != nil {
if err := r.fs.Chmod(unquotedPath, executableFileMode); err != nil {
log.Errorf("Couldn't change file mode to make file executable: %s", err)
r.fail(file.Name(), "")
return
Expand All @@ -208,7 +209,7 @@ func (r *Runner) runScript(script *config.Script, path string, file os.FileInfo)
args = strings.Split(script.Runner, " ")
}

args = append(args, path)
args = append(args, quotedScriptPath)
args = append(args, r.args[:]...)

r.run(RunOptions{
Expand Down