From 6c15869b70c7f19bdd6a3643314e69bc90faddb5 Mon Sep 17 00:00:00 2001 From: Alexander Riccio Date: Fri, 23 Sep 2022 18:18:24 -0400 Subject: [PATCH] Fix regression from #314 (chmod missed fix) #314 introduced a [regression](https://github.com/evilmartians/lefthook/pull/314#issuecomment-1230927802) 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. --- internal/lefthook/runner/runner.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/internal/lefthook/runner/runner.go b/internal/lefthook/runner/runner.go index dac53898..f54078d2 100644 --- a/internal/lefthook/runner/runner.go +++ b/internal/lefthook/runner/runner.go @@ -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 @@ -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 @@ -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{