1

I've used successfully subprocess.check_output to call plenty of windows programs. Yet, I'm troubled at calling icacls.
By cmd, this works:
cmd>icacls "C:\my folder" /GRANT *S-1-1-0:F
I've tried:
subprocess.check_output(['C:\\Windows\\System32\\icacls.exe','"C:\\my folder"','/GRANT *S-1-1-0:F'],shell=True,stderr=subprocess.STDOUT)
but return code is 123 (according to micrsoft, invalid file name).
I've also tried (which also works from cmd)
subprocess.check_output(['C:\\Windows\\System32\\icacls.exe','"C:/my folder"','/GRANT *S-1-1-0:F'],shell=True,stderr=subprocess.STDOUT)
but return code is also 123.

Any idea?

glezo
  • 742
  • 2
  • 9
  • 22

3 Answers3

3

don't overquote your arguments, or they are passed literally. Let check_output handle the quoting when needed. Best way using a list of arguments:

subprocess.check_output(['icacls.exe',r'C:\my folder','/GRANT','*S-1-1-0:F'],stderr=subprocess.STDOUT)

(note that I removed shell=True and the path to the command, and used raw prefix to avoid doubling the backslashes for the folder argument)

Jean-François Fabre
  • 137,073
  • 23
  • 153
  • 219
  • `'/grant'` needs to be a separate item, e.g. `subprocess.check_output(['icacls.exe', r'C:\my folder', '/grant', '*S-1-1-0:F'], stderr=subprocess.STDOUT)`. – Eryk Sun Jul 31 '17 at 19:12
  • If glezo was using `shell=True` to hide the console that icacls.exe creates when run from a non-console process, it would be better to simply tell icacls.exe to skip creating a console by passing the argument `creationflags=DETACHED_PROCESS`, where `DETACHED_PROCESS == 8`. – Eryk Sun Jul 31 '17 at 19:16
  • @eryksun: overlooked the "grant" part. thanks. About DETACHED_PROCESS, okay, isn't that the same effect as `startupinfo set to STARTUPINFO_NOWINDOW` ? – Jean-François Fabre Jul 31 '17 at 19:22
  • 1
    I assume you mean setting the flag `STARTF_USESHOWWINDOW` and setting the field `wShowWindow` to `SW_HIDE` (0). A console app does honor this setting (as do some GUI apps such as notepad), and it's a good way to hide the console window. You can also use the creation flag `CREATE_NO_WINDOW`, which spawns a console host process (conhost.exe) that has valid streams but doesn't create a window. Either option is good if the child creates other child console processes, since they'll inherit the console with a hidden window or no window. – Eryk Sun Jul 31 '17 at 19:41
  • The creation flag `DETACHED_PROCESS` is particularly good when running a simple command-line program such as icacls.exe that doesn't spawn child console processes. This flag makes the app completely skip inheriting or spawning a console host process and attaching to it. With this flag it's best to force the standard handles to the `\\.\NUL` device (i.e. `subprocess.DEVNULL`) if you're not setting them to pipes because some console apps expect all of their standard handles to be valid. – Eryk Sun Jul 31 '17 at 19:46
0

On Windows, you're probably better off providing a string for the command line rather than a sequence, particularly if you already know exactly what the command line you want looks like. Passing a string tells Python that you know what you're doing, and it shouldn't try to add quotes or otherwise modify the command:

subprocess.check_output(
    r'C:\Windows\System32\icacls.exe "C:\my folder" /GRANT *S-1-1-0:F',
    stderr=subprocess.STDOUT)
Harry Johnston
  • 35,639
  • 6
  • 68
  • 158
  • If you know the application follows VC++ command-line parsing rules, I think it's simpler to use a list of arguments and let `subprocess.list2cmdline` handle the quoting for you. You only need to use a string if an application has different command-line parsing rules, which is obviously the case when using `shell=True` since CMD is in its own universe. – Eryk Sun Aug 01 '17 at 12:32
  • This particular case doesn't need `shell=True` and shouldn't use it. Also, it shouldn't hard code the path to icacls.exe. – Eryk Sun Aug 01 '17 at 12:33
  • @eryksun, I think the original post is a counter-example to your assertion; the OP put quotes around the file name because that's what you do in Windows and was not expecting Python to try to double them up. If you're more used to UNIX then using a sequence may be more natural, if you're native to Windows using a string is more natural. – Harry Johnston Aug 01 '17 at 21:16
  • @eryksun, it is important to use a full path, because failing to do so results in a binary planting vulnerability. Strictly speaking, you should be looking up the system directory rather than hard-coding it, but that's outside the scope of this particular question. (I've removed `shell=True` though.) – Harry Johnston Aug 01 '17 at 21:20
  • Starting with Vista, both `CreateProcess` and CMD (which does its own search) can be configured to skip the implicit search of the working directory by defining the environment variable `NoDefaultCurrentDirectoryInExePath`. The working directory can still be explicitly searched by adding "." to `PATH`. That said, I only said it shouldn't hard code the icacls.exe path. Use `icacls_path = os.path.join(os.environ['SystemRoot'], 'System32', 'icacls.exe')`. Windows isn't always installed in "C:\Windows". – Eryk Sun Aug 01 '17 at 22:50
  • As to using a command-line string vs an args list, I can't help it if people never [read the docs](https://docs.python.org/3/library/subprocess.html#frequently-used-arguments). If they did they would know that "[p]roviding a sequence of arguments ... allows the module to take care of any required escaping and quoting of arguments (e.g. to permit spaces in file names)". And they would know [precisely what it does](https://docs.python.org/3/library/subprocess.html#converting-argument-sequence) to convert an args list to a command line on Windows. – Eryk Sun Aug 01 '17 at 22:55
  • @eryksun, I agree that well-written code shouldn't assume that the Windows directly is `c:\windows` but note that AFAIK the option to choose another directory during setup was removed several Windows versions previously. :-) – Harry Johnston Aug 01 '17 at 23:03
  • ... and I'm not saying you *shouldn't* use a sequence if you want to, just that it requires more effort since it means that you must (as you say) read the documentation to make sure what you're doing is going to work. If you use a string, it will *always* work, you don't need to think about it. – Harry Johnston Aug 01 '17 at 23:05
  • That's assuming you know the common parsing rules, which requires reading and experience (e.g. [here](https://msdn.microsoft.com/en-us/library/17w5ykft.aspx) or [here](https://msdn.microsoft.com/en-us/library/bb776391)). Using an args list actually supports the majority of devs who never read. It should be intuitive for a novice. It's possibly a problem for someone with 'experience' who understands things just enough to get into trouble. – Eryk Sun Aug 01 '17 at 23:20
  • @eryksun, I think that for a novice the way tokenization works would be counter-intuitive, e.g., intuitively the `/grant` switch should be part of the same sequence element as the permission being granted, and it isn't going to be at all obvious why that doesn't work. If you use a string the rule is just "if it works on the command line, it will work in your program" and that's a lot simpler to explain. Still, YMMV, it isn't like I've conducted a survey or anything. :-) – Harry Johnston Aug 01 '17 at 23:43
0

@Jean-François Fabre gave me the clue:
Quoting my target argument made sense, since it has blanks and, so, quoting is required when calling from cmd. However, it seems python will over-quote.

Thank you all guys for your help!!!

glezo
  • 742
  • 2
  • 9
  • 22