Windows escaping issues #314
Conversation
Signed-off-by: Valentin Kiselev <mrexox@evilmartians.com>
Signed-off-by: Valentin Kiselev <mrexox@evilmartians.com>
Signed-off-by: Valentin Kiselev <mrexox@evilmartians.com>
Signed-off-by: Valentin Kiselev <mrexox@evilmartians.com>
Signed-off-by: Valentin Kiselev <mrexox@evilmartians.com>
|
Thanks for paying attention to us windows users :) But it looks like this patch inadvertently added a regression. I'm not 100% sure what the offending change was in this commit. I have some scripts declared in my These scripts are in a path with a space in a parent directory's name - my username! - and it looks like lefthook is no longer passing the path directly to invoke the new process. So ruby gets invoked with a path that ends at the space. lefthook eventually spits out an error like: (yes, lmao, this was my dad's computer like 10 years ago, I never changed the username) ...as seen in mimes shaking fist path names are a such a pain! :D |
|
Hey! Thank you for the issue description! I'll check this. This is definitely fixable! |
|
Hay @ariccio , can you update to 1.1.1? This issue should be fixed there |
|
Ok, I couldn't get to it today, and I'm working the polls tomorrow (all day! 5AM - 11pm!), But I'll try and get to it Wednesday! |
|
Uh, I may have been stuck distracted by my family for like a week after working the polls (sshhh, don't tell them). I've updated lefthook, and am now seeing a different problem... looks like Looks like in go on Windows, |
|
Oh, I'll take a look at that issue. This is really surprising, because it's Go API, that should be working 🤔. |
|
I'm not a go programmer, I don't think I can easily hotpatch this locally right? I suspect it's not a problem with go, but that windows doesn't like quoted paths being passed to "strongly typed" APIs (which don't need no stinkin quotes)... I think you might have been passing the newly quoted path through to the other part of the program? |
|
I'll tell ya, as one of the few programmers out there who has made the mistake of a main user account with a space in it, nothing else has caused more pain, or uncovered more bugs, in my entire career 🤣 |
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.
|
@mrexox, I submitted a PR to fix the regression. |
* 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*
Closes #269
Summary 🔧
This bug is from go
execbuiltin package: https://cs.opensource.google/go/go/+/refs/tags/go1.19:src/os/exec/exec.go;l=269. They suggest passing args directly with syscall attrs. This is what is done with this PR.Also fixed a thing: if the {staged_files} or any other template is quoted like "{staged_files}", all the files from substitution will be quoted. Previously the whole line was quoted, and this is not what you want on Windows, I think.