4 ms·
I agree. Starting a subprocess is fine. Using an existing tool (mkdir) was probably a better approach than reimplementing it. The error: you should never pass
by theptip 5y ago
I agree. Starting a subprocess is fine. Using an existing tool (mkdir) was probably a better approach than reimplementing it.
The error: you should never pass unescaped/untrusted input into a subshell.
In Python, it’s the difference between the safe:
def make_dir(path):
subprocess.run([“mkdir”, “-p”, path])
Instead of:
def make_dir_unsafe(path):
subprocess.run(“mkdir -p “ + path, shell=True)
The latter is easier to type because you don’t need to use the annoying list syntax, but it’s unsafe. If you are writing a library function a subshell is not a sound approach.
- blorgle 5y agoYou shouldn't pass unescaped/untrusted input into a subshell but your suggestion doesn't solve for every case. It protects against ";" but not path traversal with "../../../../../foo/bar".
- yjftsjthsd-h 5y agoYou'd have to cover that if you wrote your own library function, though; it's attack surface, but not uniquely so.
- pabs3 5y agoI like to do this to avoid list syntax: def make_dir(path): subprocess.run('mkdir -p'.split() + [path])
- loloquwowndueo 5y agoSo you like typing more and making your program harder to read to avoid using two apostrophes and two commas.
- cowsandmilk 5y agoIf you have a command with lots of arguments, parent’s approach makes a lot of sense. 20 arguments that you can copy out of the shell into your program. And when you want to make changes and test via the shell, your copy and paste is also easy. I’ve seen bugs where some forgot to add quotes and commas to split some arguments and the program did the wrong thing. Parent’s approach avoids that. However, things like spaces in quoted strings are not correctly handled.
- div72 5y agoI am not exactly sure if parent's code execute the way you think it does. path is provided as an argument to mkdir -p, even if you set path to be a string with 20 arguments copied from your shell, it'll get passed as one. I assume you meant something like subprocess.run(['mkdir', '-p', '--', *path.split()]), which in that case using shlex.split will fix your problems with quoted strings.
- loloquwowndueo 5y agoNaively splitting commands using split() will bite you in the ass eventually. Use shlex.split as the other comment says, or just stop being lazy and craft the argument list carefully, once, and be done with it. Shortcuts here aren’t worth the risk.
- im3w1l 5y agosubprocess.run([“mkdir”, “-p”, path]) This does not work for all values of path. And I'm not just referring to blorgle's complaint. I'm being deliberately vague to make you pause for a second and think.
- Kinrany 5y agoDoes `path = "$(rm -rf /)"` work in this context?
- bstpierre 5y agoNo, because this isn’t being run through the shell, it’s directly exec’ing the program. So you’re just going to get a dir named “$(rm -rf /)”.
- im3w1l 5y agoIt's funny how easy it is to miss characters with special meaning. In this case it's the slash. You would get a directory named "$(rm -rf " with a subdirectory named ")"
- ynik 5y agoYou seem to be getting downvoted for being too vague, so I will post the solution: The snippet breaks for paths starting with a minus, as mkdir will interpret those as another option. The solution is: subprocess.run(["mkdir", "-p", "--", path])
- Noe2097 5y agoHmmm. In Python, I would leverage the dedicated utility of the standard library: os.makedirs(path)
- theptip 5y agoYes, agreed. I was using Python code to more explicitly illustrate the subprocess vs shell distinction in the general case, not to provide a solution to the specific problem of making a directory.