Skip to content

Commit

Permalink
fix: Regression from #314 (chmod missed fix) (#330)
Browse files Browse the repository at this point in the history
* 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.

* remove unused dir param.

* Move `quotedScriptPath` initialization near actual use

Believe it or not, I'm NOT a C89 programmer, I hate code that initializes variables at the top of the block. And yet, here I am. *Something something cobblers kids have no shoes something*
  • Loading branch information
ariccio authored Sep 28, 2022
1 parent 6548f66 commit 00831ee
Showing 1 changed file with 7 additions and 6 deletions.
13 changes: 7 additions & 6 deletions internal/lefthook/runner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,23 +161,23 @@ 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)
}(script, unquotedScriptPath, file)
} else {
r.runScript(script, scriptPath, file)
r.runScript(script, unquotedScriptPath, file)
}
}

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) {
if script.DoSkip(r.repo.State()) {
logSkip(file.Name(), "(SKIP BY SETTINGS)")
return
Expand All @@ -196,7 +196,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 +208,8 @@ func (r *Runner) runScript(script *config.Script, path string, file os.FileInfo)
args = strings.Split(script.Runner, " ")
}

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

r.run(RunOptions{
Expand Down

0 comments on commit 00831ee

Please sign in to comment.