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.

You 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".

You'd have to cover that if you wrote your own library function, though; it's attack surface, but not uniquely so.

Hmmm. In Python, I would leverage the dedicated utility of the standard library:

  os.makedirs(path)

Yes, 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.

    subprocess.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.

You 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])

Does `path = "$(rm -rf /)"` work in this context?

No, 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 /)”.

It'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 ")"

I like to do this to avoid list syntax:

    def make_dir(path):
      subprocess.run('mkdir -p'.split() + [path])

So you like typing more and making your program harder to read to avoid using two apostrophes and two commas.

If 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.

Naively 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.

I 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.

I'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.

“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.”

> 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.

Very true. A pro tip here is use ‘—-‘ :)

Doing fork/exec for the mkdir tool is still many times slower that calling a good library function.

> 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?

Premature 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.

The 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.

No, any language that allows operator overloading can hide ridiculously costly operations behind mundane-looking code. C is pretty good about that, but C++ is not: you can even overload the comma operator!

Consider a + b + c.

When you read the code, you know what the types of a, b and c are. You know whether they're built-in types, and therefore whether those operator expressions are actually function calls.

Nothing is hidden, so long as the programmer doesn't make invalid assumptions based on his experience with other, different languages.

Your point appears to be "if you know the entire codebase, nothing is hidden." This is tautologically true of all languages. If you're accustomed to working on small projects, then that attitude will work out for you. If you work on large codebases, it isn't always so easy, especially if folks use a lot of template magic.

In your case, if a, b, and c are all the same type, then b+c can return a completely different type, and you'll need to track that down before you know what type a+(b+c) will have. And in that case, (a+b)+c can have a different type from a+(b+c). If you've memorized the associativity rules for the language, you'll know what to expect -- but it's not at all obvious from the notation.

And, sure. If you know absolutely everything about the language and everything about the codebase, you can make accurate predictions about the behavior of any given line. But if you're coming into a new codebase, there's no way to know at a glance what a given line of code is going to cost. That's what folks are calling hidden complexity.

Oh -- and thanks to the auto keyword, you don't always know the types of a, b, and c without hunting things down.

You don't need to know the whole codebase to understand a piece of code, just what the declarations of the names being referenced in that piece of code are (in C++, a name may be a macro, a type, a template, a variable or a function, and depending on which kind, may have all kinds of other properties attached).

Yes, you need to look at the declarations of names to know what the resulting type of expressions involving those names is. I don't see that as being surprising or unusual. The same is also true in C.

I disagree that C++ makes expensive operations explicit; see the copy constructor and the copy assignment operators. C, maybe.

A C++ developer would know what an assignment or copy entails and would spot them on sight.

It's impossible to tell whether a specific line of code makes a copy without knowing the types of all values. i.e.

    std::vector<int> numbers = Func();

may or may not make a copy depending on the signature of Func. So it's not clear at the callsite how expensive that line of code is. This is unlike Go (which has this explicitly as philosophy) where you need to call a copy function/use append to make a copy of a slice.

Knowing the declarations of the various names a piece of code uses is of course a requirement in order to understand what it does.

In that example, there would always be a move or copy there, though there are scenarios in which it might be elided.

It depends a lot on how often you’re trying to do it and the conditions you’re doing it under. I’ve worked on a couple of projects where I own an agent process that runs on thousands of machines that are not mine (so I have no control of whether they are overloaded or in some pathological state), some of which are bumping up against memory and/or cpu and or IO limits, and a single blocking system call is many orders of magnitude more practical than the overhead of fork/exec and the dozens or hundreds of blocking system calls and memory allocations that entails.

This is, of course, an extreme example. But there are definitely cases where it matters a lot.

Completely agreed. But likely many many times faster to write. Just depends what youre optimizing for.