11 ms·
There’s absolutely a middle ground, and you are right: it is fork and exec with discrete arguments. It is the intermediary subshell in system() that is the roo
by infotogivenm 5y ago
There’s absolutely a middle ground, and you are right: it is fork and exec with discrete arguments.
It is the intermediary subshell in system() that is the root of all evil, not composing unix programs together; that is what our OS forefathers intended.
- theptip 5y agoI 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.
- elurg 5y agoDoing fork/exec for the mkdir tool is still many times slower that calling a good library function.
- fivea 5y ago> Doing fork/exec for the mkdir tool is still many times slower that calling a good library function. Would it be noticeably slower to the point it's measurably relevant, though? Or are we talking about something well within the domain of premature optimization? I mean,if you're doing IO, how often does creating a dir end up being the performance bottleneck?
- thebackup 5y agoPremature optimization is a common disease amongst C and C++ devs. I often come across code that is written in an obscure/complex way just because someone thought execution speed triumphs code readability. In most cases (around 9 times out of 10) it is on a non-time-critical path of the code and the developer who wrote it didn't even bother to run a profiler on it. My work would be significantly easier if we could learn to prioritize code readability.
- mgaunard 5y agoThe point of those languages is to make it explicit when you're doing ridiculously costly things so that you're forced to do a reasonable implementation instead.
- ptx 5y ago> It is the intermediary subshell in system() that is the root of all evil Certainly the worst of it, but even if you pass the untrusted data as a discrete arguments you're still vulnerable to parameter injection. Let's say, for example, that you're running ["find", USER_SUPPLIED_PATH, "-type", "f"]. If the user supplies "-delete" as the path, the resulting behaviour would not be as intended.
- infotogivenm 5y agoVery true. A pro tip here is use ‘—-‘ :)
- me_again 5y agoI'd want a better reason before shelling out personally. Apart from security, other issues are: - performance. Obviously that depends on the circumstances but if you are working on a function that is used in many places you don't know the circumstances. - error handling and reporting. Are you going to capture stderr from mkdir? If this fails can you provide a clear indication of the problem to the caller? - paths. If you run mkdir without specifying an absolute path your program may fail if $path is not what you expect. That can also be a vulnerability. - dependencies. Now you won't run in a distroless container or other cut down environment - you lose type safety Not saying "never shell out" but I've been burned by most of these so I am cautious about it.
- hamburglar 5y ago“Never shell out” is pretty close to correct though. I’d say “only shell out as a last resort, and only after you’ve considered all the drawbacks.”