consider getting rid of `sh` entirely?

[oconnor663]

https://www.reddit.com/r/rust/comments/618jvw/announcing_duct_a_child_process_library_thats/dfdn2m3/

[oconnor663]

If we believe that sh is too unsafe to keep as is, my next best proposal would've been this: Restrict sh to accept one of two types, either static strings, or regular strings wrapped in some type with a scary name. That would force people to confront the scary name before they do anything wildly unsafe, but would allow the current most common usages with a single hardcoded string.

The main downside I see to that approach is that it won't translate well to other languages. As far as I know, Rust is unique in its ability to tell the difference between a static string and one that's been constructed from user input. (I remember hearing that Ruby has some kind of taint tracking in their strings, though I don't know if that's Ruby proper or actually part of Rails.) I'm not very keen on an API that's tied to Rust's type system.

[oconnor663]

For the record, the one really good use case of sh I know of (that's not just reducing the number of quotation marks you have to type) is on Windows, where you need to use shell mode to execute anything that isn't a binary or a .bat script. However, the details of how that works are also very tricky. There are several reg keys that control e.g. which Python interpreter gets used. Creating your own .bat wrapper, rather than relying on cmd.exe to find the interpreter for you, is probably a better approach for that case. (We could consider switching to that approach in peru, @olson-sean-k.)

[passcod]

For programs that advertise they're going to run the given command in a shell as a child, this is useful. For example, cargo-watch, which I was in the process of refactoring to use duct when I saw this deprecation pop up.

The one really good use case of sh I know of (that's not just reducing the number of quotation marks you have to type) is on Windows, where you need to use shell mode to execute anything that isn't a binary or a .bat script. However, the details of how that works are also very tricky.

Thus, my use case for sh() is to abstract away the cross-platforms concerns when I specifically want to run a command the user gave me as-is. For Unix, I could of course just rewrite as cmd!("/bin/sh", "-c", command).

My vote on this is to rename, not remove. Alternatively, to extract the start_sh bit into its own tiny crate so it can be used in conjunction with duct, or standalone.

[oconnor663]

The small separate crate option might make sense, or maybe a submodule. In that thread at the top, @dpc suggested we could provide a shell escape function too, which would be nice to have in a cross platform way. I don't think a function like that quite fits into duct (since the vast majority of use cases really should be using cmd! instead I think), but it could definitely fit into a separate shell utilities crate.

Right now, what sh is doing for you could be totally captured by a single function like this:

fn sh_command_line<T: Into<OsString>>(command: T) -> Vec<OsString> {
    if cfg!(windows) {
        let shell = std::env::var_os("COMSPEC").unwrap_or("cmd.exe".into());
        vec![shell, "/C".into(), command.into()]
    } else {
        vec!["/bin/sh".into(), "-c".into(), command.into()]
    }
}

Would you want to pull in an extra crate to get that function, or is it small enough that it makes more sense to just copy it?

[dpc]

I am all for rename, not a remove. Rust has unsafe for a reason. Just because something is dangerous, doesn't mean there are no legitimate reasons to use it in some circumstances. On reddit I've proposed unescaped_sh, dangerous_sh, unsafe_sh. Putting the risk uprfont in the name should prevent people from being mindless about it.

Shell escaping is another story. It is useful, but is shell-dependant. Eg for bash probably this (might not be Rust syntax):

"'" + str.replace("\n", "'$'\\n''").replace("'",  "'$'\\'''") + "'"

will work as per http://wiki.bash-hackers.org/syntax/quoting#ansi_c_like_strings, but I'm not sure about portable way to do it in sh. Anyway, something to discuss in separate issue.

[oconnor663]

Ok, here's what I'm leaning towards right now:

• Add an sh_unsafe function (or however we want to bikeshed the name), which works just like sh does now, and comes with appropriately scary warnings in the docs. The intention will be that languages that can't distinguish between safe and unsafe strings (so, anything besides Rust and Ruby?) should only provide sh_unsafe.
• Modify sh to accept only static strings (probably &'static T where T: AsRef<OsStr>), which should prevent the vast majority of shell injection bugs, while allowing the vast majority of safe, convenient use cases. I'd appreciate comments about whether this is a reasonable assumption to make about static references. The only way I know of to bypass this in safe code would be to somehow read user input into a lazy_static string. Is that enough of a ridiculous corner case that we can ignore it? Is the convenience of having a not-scarily-named sh function worth forcing everyone to think about the difference?

[oconnor663]

Also leaning towards splitting these out into a duct-sh sub-crate. I might not prefer to make them part of the standard cross-language duct API. In Python, for example, you both 1) already have easy access to shell=True and 2) have no way to provide the sh function safely.

[oconnor663]

Just published a new crate: https://crates.io/crates/duct_sh

Also uploaded version 0.9.0 of duct, which removes the sh function in favor of that crate.