Skip to content

Update documentation related to the recent cmd.exe fix #123709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 32 additions & 17 deletions library/std/src/os/windows/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,14 @@ pub trait CommandExt: Sealed {

/// Append literal text to the command line without any quoting or escaping.
///
/// This is useful for passing arguments to applications which doesn't follow
/// This is useful for passing arguments to applications that don't follow
/// the standard C run-time escaping rules, such as `cmd.exe /c`.
///
/// # Bat files
/// # Batch files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many kinds of batch files used by many different job control languages. I believe only the .bat files of Windows are under discussion here. This retitling seems to be less correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the proper name is "DOS Batch file" but figured the DOS part is implied when discussing Windows. I have indeed seen "BAT File" though, just not 🦇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BAT file seems like the one we might prefer here, yes.

However, I must wonder... do all of these caveats apply to files with the .cmd extension also?

Copy link
Member

@workingjubilee workingjubilee Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation so far doesn't seem to mention .cmd at all, despite that also being an applicable extension for Windows batch files with basically-the-same command language. I won't pretend to know what the exact caveats and corner cases are, though.

Copy link
Contributor Author

@tgross35 tgross35 Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I must wonder... do all of these caveats apply to files with the .cmd extension also?

Hey, according to wiki .cmd is just another "Batch File" 😄 https://en.wikipedia.org/wiki/Batch_file

(I'll take another read through and make sure both are covered appropriately)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed!

I had a frisson of annoyance at that, as technically, there are many JCLs etc. etc. and then I realized that while indeed even in 2024 some people do use mainframes with such batch files that are not in the MS-DOS command language... everyone understands the meaning here from context, obviously.

///
/// Note the `cmd /c` command line has slightly different escaping rules then bat files
/// Note the `cmd /c` command line has slightly different escaping rules than batch files
/// themselves. If possible, it may be better to write complex arguments to a temporary
/// .bat file, with appropriate escaping, and simply run that using:
/// `.bat` file, with appropriate escaping, and simply run that using:
///
/// ```no_run
/// # use std::process::Command;
Expand All @@ -217,7 +217,7 @@ pub trait CommandExt: Sealed {
///
/// # Example
///
/// Run a bat script using both trusted and untrusted arguments.
/// Run a batch script using both trusted and untrusted arguments.
///
/// ```no_run
/// #[cfg(windows)]
Expand All @@ -241,9 +241,10 @@ pub trait CommandExt: Sealed {
/// if !user_name.chars().all(|c| c.is_alphanumeric()) {
/// return Err(Error::new(ErrorKind::InvalidInput, "invalid user name"));
/// }
/// // now we've checked the user name, let's add that too.
/// cmd_args.push(' ');
/// cmd_args.push_str(&format!("--user {user_name}"));
///
/// // now we have validated the user name, let's add that too.
/// cmd_args.push_str(" --user ");
/// cmd_args.push_str(user_name);
///
/// // call cmd.exe and return the output
/// Command::new("cmd.exe")
Expand Down Expand Up @@ -287,25 +288,37 @@ pub trait CommandExt: Sealed {
#[unstable(feature = "windows_process_extensions_async_pipes", issue = "98289")]
fn async_pipes(&mut self, always_async: bool) -> &mut process::Command;

/// Sets a raw attribute on the command, providing extended configuration options for Windows processes.
/// Set a raw attribute on the command, providing extended configuration options for Windows
/// processes.
///
/// This method allows you to specify custom attributes for a child process on Windows systems
/// using raw attribute values. Raw attributes provide extended configurability for process
/// creation, but their usage can be complex and potentially unsafe.
///
/// This method allows you to specify custom attributes for a child process on Windows systems using raw attribute values.
/// Raw attributes provide extended configurability for process creation, but their usage can be complex and potentially unsafe.
/// The `attribute` parameter specifies the raw attribute to be set, while the `value`
/// parameter holds the value associated with that attribute. Please refer to the
/// [`windows-rs` documentation] or the [Win32 API documentation] for detailed information
/// about available attributes and their meanings.
///
/// The `attribute` parameter specifies the raw attribute to be set, while the `value` parameter holds the value associated with that attribute.
/// Please refer to the [`windows-rs`](https://microsoft.github.io/windows-docs-rs/doc/windows/) documentation or the [`Win32 API documentation`](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute) for detailed information about available attributes and their meanings.
/// [`windows-rs` documentation]: https://microsoft.github.io/windows-docs-rs/doc/windows/
/// [Win32 API documentation]: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute
///
/// # Note
///
/// The maximum number of raw attributes is the value of [`u32::MAX`].
/// If this limit is exceeded, the call to [`process::Command::spawn`] will return an `Error` indicating that the maximum number of attributes has been exceeded.
/// If this limit is exceeded, the call to [`process::Command::spawn`] will return an `Error`
/// indicating that the maximum number of attributes has been exceeded.
///
/// # Safety
///
/// The usage of raw attributes is potentially unsafe and should be done with caution. Incorrect attribute values or improper configuration can lead to unexpected behavior or errors.
/// The usage of raw attributes is potentially unsafe and should be done with caution.
/// Incorrect attribute values or improper configuration can lead to unexpected behavior or
/// errors.
///
/// # Example
///
/// The following example demonstrates how to create a child process with a specific parent process ID using a raw attribute.
/// The following example demonstrates how to create a child process with a specific parent
/// process ID using a raw attribute.
///
/// ```rust
/// #![feature(windows_process_extensions_raw_attribute)]
Expand Down Expand Up @@ -339,7 +352,9 @@ pub trait CommandExt: Sealed {
///
/// # Safety Note
///
/// Remember that improper use of raw attributes can lead to undefined behavior or security vulnerabilities. Always consult the documentation and ensure proper attribute values are used.
/// Remember that improper use of raw attributes can lead to undefined behavior or security
/// vulnerabilities. Always consult the documentation and ensure proper attribute values are
/// used.
#[unstable(feature = "windows_process_extensions_raw_attribute", issue = "114854")]
unsafe fn raw_attribute<T: Copy + Send + Sync + 'static>(
&mut self,
Expand Down
48 changes: 27 additions & 21 deletions library/std/src/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@
//!
//! # Windows argument splitting
//!
//! On Unix systems arguments are passed to a new process as an array of strings
//! but on Windows arguments are passed as a single commandline string and it's
//! On Unix systems arguments are passed to a new process as an array of strings,
//! but on Windows arguments are passed as a single commandline string and it is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commander Data, is that you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, the grammar lords always said to avoid short contractions in technical writing

//! up to the child process to parse it into an array. Therefore the parent and
//! child processes must agree on how the commandline string is encoded.
//!
Expand All @@ -107,26 +107,26 @@
//! * Use [`raw_arg`] to build a custom commandline. This bypasses the escaping
//! rules used by [`arg`] so should be used with due caution.
//!
//! `cmd.exe` and `.bat` use non-standard argument parsing and are especially
//! `cmd.exe` and `.bat` files use non-standard argument parsing and are especially
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it sound like "cmd.exe files" is a valid decomposition of the sentence's parts.

//! vulnerable to malicious input as they may be used to run arbitrary shell
//! commands. Untrusted arguments should be restricted as much as possible.
//! For examples on handling this see [`raw_arg`].
//!
//! ### Bat file special handling
//! ### Batch file special handling
//!
//! On Windows, `Command` uses the Windows API function [`CreateProcessW`] to
//! spawn new processes. An undocumented feature of this function is that,
//! spawn new processes. An undocumented feature of this function is that
//! when given a `.bat` file as the application to run, it will automatically
//! convert that into running `cmd.exe /c` with the bat file as the next argument.
//! convert that into running `cmd.exe /c` with the batch file as the next argument.
//!
//! For historical reasons Rust currently preserves this behaviour when using
//! [`Command::new`], and escapes the arguments according to `cmd.exe` rules.
//! Due to the complexity of `cmd.exe` argument handling, it might not be
//! possible to safely escape some special chars, and using them will result
//! possible to safely escape some special characters, and using them will result
//! in an error being returned at process spawn. The set of unescapeable
//! special chars might change between releases.
//! special characters might change between releases.
//!
//! Also note that running `.bat` scripts in this way may be removed in the
//! Also note that running batch scripts in this way may be removed in the
//! future and so should not be relied upon.
//!
//! [`spawn`]: Command::spawn
Expand Down Expand Up @@ -655,16 +655,19 @@ impl Command {
///
/// Note that the argument is not passed through a shell, but given
/// literally to the program. This means that shell syntax like quotes,
/// escaped characters, word splitting, glob patterns, variable substitution, etc.
/// have no effect.
/// escaped characters, word splitting, glob patterns, variable substitution,
/// etc. have no effect.
///
/// <div class="warning">
///
/// On Windows use caution with untrusted inputs. Most applications use the
/// standard convention for decoding arguments passed to them. These are safe to use with `arg`.
/// However some applications, such as `cmd.exe` and `.bat` files, use a non-standard way of decoding arguments
/// and are therefore vulnerable to malicious input.
/// In the case of `cmd.exe` this is especially important because a malicious argument can potentially run arbitrary shell commands.
/// On Windows, use caution with untrusted inputs. Most applications use the
/// standard convention for decoding arguments passed to them. These are safe to
/// use with `arg`. However, some applications such as `cmd.exe` and `.bat` files
/// use a non-standard way of decoding arguments. They are therefore vulnerable
/// to malicious input.
///
/// In the case of `cmd.exe` this is especially important because a malicious
/// argument can potentially run arbitrary shell commands.
///
/// See [Windows argument splitting][windows-args] for more details
/// or [`raw_arg`] for manually implementing non-standard argument encoding.
Comment on lines +663 to 673
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither this nor the linked page really answer "I have untrusted input, how do I know if I can use it with .arg?". I think this could be improved but not really sure how (I didn't change it, just rewrapped).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and that's a tricky one to answer. I guess the short answer is you just have to know and trust the application/script that you're using uses MSVC compatible argument parsing rules. Which, I mean, is annoying. On the other hand sending untrusted arguments to an unknown application sounds wrong to me in any case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can put something like that into words, just explaining how Rust does quoting would help.

We now use the cmd quoting if the executable for Command::new is cmd.exe, *.bat or *.cmd, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't for an explicit call to cmd.exe. We do for *.bat or *.cmd (case insensitive) as that's an implicit call to cmd.exe.

Note that we also don't prevent Linux users from using sh to execute things in the shell so that was not a goal here.

Expand Down Expand Up @@ -706,11 +709,14 @@ impl Command {
///
/// <div class="warning">
///
/// On Windows use caution with untrusted inputs. Most applications use the
/// standard convention for decoding arguments passed to them. These are safe to use with `args`.
/// However some applications, such as `cmd.exe` and `.bat` files, use a non-standard way of decoding arguments
/// and are therefore vulnerable to malicious input.
/// In the case of `cmd.exe` this is especially important because a malicious argument can potentially run arbitrary shell commands.
/// On Windows, use caution with untrusted inputs. Most applications use the
/// standard convention for decoding arguments passed to them. These are safe to
/// use with `arg`. However, some applications such as `cmd.exe` and `.bat` files
/// use a non-standard way of decoding arguments. They are therefore vulnerable
/// to malicious input.
///
/// In the case of `cmd.exe` this is especially important because a malicious
/// argument can potentially run arbitrary shell commands.
///
/// See [Windows argument splitting][windows-args] for more details
/// or [`raw_arg`] for manually implementing non-standard argument encoding.
Expand Down
Loading