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

Conversation

ariccio
Copy link
Contributor

@ariccio ariccio commented Sep 23, 2022

#314 introduced a regression that passes quoted paths to fs.chmod. fs.Chmod/os.chmod on windows calls into the GetFileAttributes/SetFileAttributes 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. Guess I'll have to file a bug in go to satisfy my OCD :)

evilmartians#314 introduced a [regression](evilmartians#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.
@ariccio
Copy link
Contributor Author

ariccio commented Sep 23, 2022

Scratch that last bit about GetLastError, I didn't know that go did some inline assembly work behind the scenes to transparently grab it. Nice.

Fixing linter warning now.

Copy link
Member

@mrexox mrexox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Perfect! Just a small nitpick. If you don't have an opportunity to push changes, I can merge the request, just say about it!

internal/lefthook/runner/runner.go Outdated Show resolved Hide resolved
@ariccio
Copy link
Contributor Author

ariccio commented Sep 27, 2022

Will make the change.

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*
Copy link
Member

@mrexox mrexox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! 🍰

@mrexox mrexox merged commit 00831ee into evilmartians:master Sep 28, 2022
@ariccio
Copy link
Contributor Author

ariccio commented Sep 29, 2022

Yay! I've been using --no-verify for a month or so now, and it's been triggering this OCD-sufferer a LOT 😂

@ariccio
Copy link
Contributor Author

ariccio commented Oct 4, 2022

May I ask pretty please that you push this to release 😉

I suspect you're busy and it just fell off your radar!

@mrexox
Copy link
Member

mrexox commented Oct 10, 2022

@ariccio , sorry for the long wait. I have just released the fix in v1.1.2.

@ariccio
Copy link
Contributor Author

ariccio commented Oct 10, 2022

No need to apologize, much appreciated!!

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 this pull request may close these issues.

2 participants