python / cpython

The Python programming language
https://www.python.org/
Other
61.24k stars 29.55k forks source link

Add subprocess.shell(cmd) function: similar to os.system(), but use subprocess #120952

Open vstinner opened 3 weeks ago

vstinner commented 3 weeks ago

Feature or enhancement

Hi,

I propose to add a subprocess.shell(cmd) function similar to os.system(), but use subprocess: close file descriptors, restore signal handlers, etc. The function runs a shell command and so is vulnerable by design to shell injection vulnerability if an attacker can modify the shell command.

The function accepts keyword arguments: subprocess.shell(cmd, env=env), subprocess.shell(cmd, stdout=subprocess.PIPE), etc.

This proposition is related to os.system() soft deprecation: https://github.com/python/cpython/pull/120744

What do you think?

Victor

cc @gpshead @zooba

Linked PRs

zooba commented 3 weeks ago

So essentially shell = functools.partial(subprocess.run, shell=True)? That seems fine to me.

gpshead commented 3 weeks ago

I think that functools.partial definition is exactly all this should be (though lets make sure it does not allow shell=False to be passed - I don't recall off the top of my what functools.partial does in that situation).

Documentation wise it'd get listed in the subprocess docs after subprocess.run() with a one or two sentence description that it is just "run" with "shell=True" for convenience. With an example. And with the caveats of shell=True being re-stated.

barneygale commented 3 weeks ago

Are we handing beginners a foot-shotgun by making shells (and all their quoting problems) easier to access? I like the explicit subprocess.run(..., shell=True) spelling, but maybe I've stockholm syndrome'd myself.

zooba commented 3 weeks ago

Possibly.

I think it has plenty of legitimate uses, which is why keeping os.system is important. But adding something new to support it is certainly endorsing its use. And it doesn't really have much of a place in production or library code, while it does have an important place in scripting.

It's being added here so we can soft deprecate it elsewhere. I'd rather just leave the original one in place without any kind of deprecation.

gpshead commented 3 weeks ago

The benefit of this one is that it's subprocess backed with all that brings. I agree shell=True is not what I promote use of but if we recommend people skip over os.system in favor of something else, subprocess.shell might make a good gateway drug out of os land.

os.system is, as the os module is generally intended to be, a thin wrapper around the platform's libc or platform provided system() or _wsystem() C calls.