From 6d79f6b97355681a47958d8a99df67aef772b67d Mon Sep 17 00:00:00 2001 From: Alexander Medvednikov Date: Tue, 12 May 2026 11:13:17 +0300 Subject: [PATCH] os: safer exec([]string) --- vlib/os/README.md | 8 ++++ vlib/os/execute_capture_nix.h | 69 +++++++++++++++++++++++++++++++++++ vlib/os/os.js.v | 21 +++++++++++ vlib/os/os_nix.c.v | 49 +++++++++++++++++++++++++ vlib/os/os_test.c.v | 31 ++++++++++++++++ vlib/os/os_windows.c.v | 56 ++++++++++++++++++++++------ vlib/v2/types/checker.v | 61 +++++++++++++++++++++++++++++-- vlib/v2/types/checker_test.v | 41 +++++++++++++++++++++ 8 files changed, 321 insertions(+), 15 deletions(-) diff --git a/vlib/os/README.md b/vlib/os/README.md index 0750aa1d8..dc42b30e4 100644 --- a/vlib/os/README.md +++ b/vlib/os/README.md @@ -7,6 +7,14 @@ handling processes etc. On Windows, `os.data_dir()` uses `%LocalAppData%` for user-specific application data. +### Running commands + +Use `os.exec(['program', 'arg 1', 'arg 2'])` when the command and its arguments +are already separate values. It runs the program directly and does not invoke a +shell, so spaces and shell metacharacters inside arguments are passed literally. + +Use `os.execute('command string')` only when shell syntax is intended. + --- ### Security advice related to TOCTOU attacks diff --git a/vlib/os/execute_capture_nix.h b/vlib/os/execute_capture_nix.h index e959a5b46..11bb63abb 100644 --- a/vlib/os/execute_capture_nix.h +++ b/vlib/os/execute_capture_nix.h @@ -46,6 +46,36 @@ static int v_os_execute_capture_start(const char *cmd, int *child_pid, int *read *read_fd = pipefd[0]; return 0; } + +static int v_os_exec_capture_start(char *const argv[], int *child_pid, int *read_fd) { + if (argv == NULL || argv[0] == NULL) { + return -1; + } + int pipefd[2]; + if (pipe(pipefd) != 0) { + return -1; + } + v_os_execute_set_cloexec(pipefd[0]); + v_os_execute_set_cloexec(pipefd[1]); + pid_t pid = fork(); + if (pid < 0) { + close(pipefd[0]); + close(pipefd[1]); + return -1; + } + if (pid == 0) { + dup2(pipefd[1], STDOUT_FILENO); + dup2(pipefd[1], STDERR_FILENO); + close(pipefd[0]); + close(pipefd[1]); + execvp(argv[0], argv); + _exit(127); + } + close(pipefd[1]); + *child_pid = (int)pid; + *read_fd = pipefd[0]; + return 0; +} #else // Use opaque void* declarations for posix_spawn instead of #include . // Including transitively pulls in /, which @@ -58,6 +88,7 @@ static int v_os_execute_capture_start(const char *cmd, int *child_pid, int *read // are passed identically regardless of pointee type. typedef struct { unsigned char _opaque[128]; } v_posix_spawn_file_actions_t; extern int posix_spawn(pid_t *, const char *, const void *, const void *, char *const [], char *const []); +extern int posix_spawnp(pid_t *, const char *, const void *, const void *, char *const [], char *const []); extern int posix_spawn_file_actions_init(void *); extern int posix_spawn_file_actions_destroy(void *); extern int posix_spawn_file_actions_adddup2(void *, int, int); @@ -100,4 +131,42 @@ static int v_os_execute_capture_start(const char *cmd, int *child_pid, int *read *read_fd = pipefd[0]; return 0; } + +static int v_os_exec_capture_start(char *const argv[], int *child_pid, int *read_fd) { + if (argv == NULL || argv[0] == NULL) { + return -1; + } + int pipefd[2]; + if (pipe(pipefd) != 0) { + return -1; + } + v_os_execute_set_cloexec(pipefd[0]); + v_os_execute_set_cloexec(pipefd[1]); + v_posix_spawn_file_actions_t actions; + if (posix_spawn_file_actions_init(&actions) != 0) { + close(pipefd[0]); + close(pipefd[1]); + return -1; + } + int err = 0; + if ((err = posix_spawn_file_actions_adddup2(&actions, pipefd[1], STDOUT_FILENO)) != 0 + || (err = posix_spawn_file_actions_adddup2(&actions, pipefd[1], STDERR_FILENO)) != 0 + || (err = posix_spawn_file_actions_addclose(&actions, pipefd[0])) != 0 + || (err = posix_spawn_file_actions_addclose(&actions, pipefd[1])) != 0) { + posix_spawn_file_actions_destroy(&actions); + close(pipefd[0]); + close(pipefd[1]); + return -1; + } + err = posix_spawnp(child_pid, argv[0], &actions, NULL, argv, environ); + posix_spawn_file_actions_destroy(&actions); + if (err != 0) { + close(pipefd[0]); + close(pipefd[1]); + return -1; + } + close(pipefd[1]); + *read_fd = pipefd[0]; + return 0; +} #endif diff --git a/vlib/os/os.js.v b/vlib/os/os.js.v index 4cd963f76..1bf13caa9 100644 --- a/vlib/os/os.js.v +++ b/vlib/os/os.js.v @@ -141,6 +141,27 @@ pub fn execute(cmd string) Result { } } +// exec starts the specified command with arguments, waits for it to complete, and returns its output. +pub fn exec(args []string) Result { + if args.len == 0 { + return Result{ + exit_code: -1 + output: 'exec requires at least one argument' + } + } + mut exit_code := 0 + mut stdout := '' + #let commands = args.arr.map((x) => x.valueOf() + ''); + #let output = $child_process.spawnSync(commands[0], commands.slice(1, commands.length)); + #exit_code = new int(output.status === null ? -1 : output.status) + #stdout = new string((output.stdout ? output.stdout + '' : '') + (output.stderr ? output.stderr + '' : '')) + + return Result{ + exit_code: exit_code + output: stdout + } +} + pub fn system(cmd string) int { exit_code := 0 #let commands = cmd.str.split(' '); diff --git a/vlib/os/os_nix.c.v b/vlib/os/os_nix.c.v index b8601ddca..8cb3a7424 100644 --- a/vlib/os/os_nix.c.v +++ b/vlib/os/os_nix.c.v @@ -74,6 +74,8 @@ fn C.getegid() i32 fn C.v_os_execute_capture_start(cmd &char, child_pid &int, read_fd &int) int +fn C.v_os_exec_capture_start(argv &&char, child_pid &int, read_fd &int) int + enum GlobMatch { exact ends_with @@ -363,6 +365,53 @@ pub fn execute(cmd string) Result { } } +// exec starts the specified command with arguments, waits for it to complete, and returns its output. +pub fn exec(args []string) Result { + if args.len == 0 { + return Result{ + exit_code: -1 + output: 'exec requires at least one argument' + } + } + mut cargs := []&char{cap: args.len + 1} + for arg in args { + cargs << &char(arg.str) + } + cargs << &char(unsafe { nil }) + mut pid := 0 + mut read_fd := -1 + v_os_execute_lock() + rc := C.v_os_exec_capture_start(cargs.data, &pid, &read_fd) + v_os_execute_unlock() + if rc != 0 { + return Result{ + exit_code: -1 + output: 'exec("${args[0]}") failed' + } + } + soutput := fd_slurp(read_fd).join('') + fd_close(read_fd) + mut status := 0 + for { + C.errno = 0 + if C.waitpid(pid, &status, 0) != -1 { + break + } + if C.errno == C.EINTR { + continue + } + return Result{ + exit_code: -1 + output: soutput + } + } + exit_code, _ := posix_wait4_to_exit_status(status) + return Result{ + exit_code: exit_code + output: soutput + } +} + // raw_execute does the same as `execute` on Unix platforms. // On Windows raw_execute starts the specified command, waits for it to complete, and returns its output. // It's marked as `unsafe` to help emphasize the problems that may arise by allowing, for example, diff --git a/vlib/os/os_test.c.v b/vlib/os/os_test.c.v index bacd009bb..1c05fc7f8 100644 --- a/vlib/os/os_test.c.v +++ b/vlib/os/os_test.c.v @@ -1046,6 +1046,37 @@ fn test_execute() { assert hexresult.ends_with('0a7878') } +fn test_exec_with_args() { + source_path := os.join_path_single(tfolder, 'exec_args.v') + output_arg := os.join_path_single(tfolder, 'exec_args') + exe_path := if os.user_os() == 'windows' { + output_arg + '.exe' + } else { + output_arg + } + os.write_file(source_path, + "import os\n\nfn main() {\n\tprintln(os.args[1..].join('|'))\n\teprintln('stderr-ok')\n}\n")! + defer { + os.rm(source_path) or {} + os.rm(exe_path) or {} + os.rm(output_arg + '.c') or {} + } + compile_result := + os.execute('${os.quoted_path(@VEXE)} -o ${os.quoted_path(output_arg)} ${os.quoted_path(source_path)}') + assert compile_result.exit_code == 0, compile_result.output + + result := os.exec([exe_path, 'one two', 'semi;colon']) + normalized_output := result.output.replace('\r\n', '\n') + assert result.exit_code == 0, result.output + assert normalized_output.contains('one two|semi;colon\n'), result.output + assert normalized_output.contains('stderr-ok\n'), result.output + + shell_metachar_result := os.exec([exe_path, 'first; echo injected']) + normalized_shell_metachar_output := shell_metachar_result.output.replace('\r\n', '\n') + assert shell_metachar_result.exit_code == 0, shell_metachar_result.output + assert normalized_shell_metachar_output.contains('first; echo injected\n'), shell_metachar_result.output +} + fn test_execute_with_stderr_redirection() { result := os.execute('${os.quoted_path(@VEXE)} wrong_command') assert result.exit_code == 1 diff --git a/vlib/os/os_windows.c.v b/vlib/os/os_windows.c.v index a21701ea0..7dfcdb8fd 100644 --- a/vlib/os/os_windows.c.v +++ b/vlib/os/os_windows.c.v @@ -476,6 +476,26 @@ pub fn execute(cmd string) Result { return unsafe { raw_execute(cmd) } } +// exec starts the specified command with arguments, waits for it to complete, and returns its output. +pub fn exec(args []string) Result { + if args.len == 0 { + return Result{ + exit_code: -1 + output: 'exec requires at least one argument' + } + } + mut command_line := requote_arg(args[0]) + if args.len > 1 { + command_line += ' ' + requote_args(args[1..]) + } + mut application_name := '' + filename_lc := args[0].to_lower_ascii() + if is_abs_path(args[0]) && !filename_lc.ends_with('.bat') && !filename_lc.ends_with('.cmd') { + application_name = args[0] + } + return windows_execute_command_line(command_line, application_name, args.join(' '), false) +} + // windows_execute_has_forbidden_shell_operator rejects only the command chaining operators that `cmd.exe` // treats specially outside double-quoted strings. fn windows_execute_has_forbidden_shell_operator(cmd string) bool { @@ -507,6 +527,19 @@ fn windows_execute_has_forbidden_shell_operator(cmd string) bool { // user provided escape sequences. @[unsafe] pub fn raw_execute(cmd string) Result { + mut pcmd := cmd + if cmd.contains('./') { + pcmd = pcmd.replace('./', '.\\') + } + if cmd.contains('2>') { + pcmd = 'cmd /c "${pcmd}"' + } else { + pcmd = 'cmd /c "${pcmd} 2>&1"' + } + return windows_execute_command_line(pcmd, '', cmd, true) +} + +fn windows_execute_command_line(command_line_text string, application_name string, display_cmd string, expand_environment bool) Result { mut child_stdin := &u32(unsafe { nil }) mut child_stdout_read := &u32(unsafe { nil }) mut child_stdout_write := &u32(unsafe { nil }) @@ -545,25 +578,24 @@ pub fn raw_execute(cmd string) Result { dw_flags: u32(C.STARTF_USESTDHANDLES) } - mut pcmd := cmd - if cmd.contains('./') { - pcmd = pcmd.replace('./', '.\\') + mut command_line_ptr := command_line_text.to_wide() + mut expanded_command_line := [32768]u16{} + if expand_environment { + C.ExpandEnvironmentStringsW(command_line_ptr, voidptr(&expanded_command_line), 32768) + command_line_ptr = &expanded_command_line[0] } - if cmd.contains('2>') { - pcmd = 'cmd /c "${pcmd}"' - } else { - pcmd = 'cmd /c "${pcmd} 2>&1"' + mut application_name_ptr := &u16(unsafe { nil }) + if application_name != '' { + application_name_ptr = application_name.to_wide() } - command_line := [32768]u16{} - C.ExpandEnvironmentStringsW(pcmd.to_wide(), voidptr(&command_line), 32768) - create_process_ok := C.CreateProcessW(0, &command_line[0], 0, 0, C.TRUE, C.CREATE_NO_WINDOW, 0, - 0, voidptr(&start_info), voidptr(&proc_info)) + create_process_ok := C.CreateProcessW(application_name_ptr, command_line_ptr, 0, 0, C.TRUE, + C.CREATE_NO_WINDOW, 0, 0, voidptr(&start_info), voidptr(&proc_info)) if !create_process_ok { error_num := int(C.GetLastError()) error_msg := get_error_msg(error_num) return Result{ exit_code: error_num - output: 'exec failed (CreateProcess) with code ${error_num}: ${error_msg} cmd: ${cmd}' + output: 'exec failed (CreateProcess) with code ${error_num}: ${error_msg} cmd: ${display_cmd}' } } C.CloseHandle(child_stdin) diff --git a/vlib/v2/types/checker.v b/vlib/v2/types/checker.v index 5ea90734c..65ba1ed8b 100644 --- a/vlib/v2/types/checker.v +++ b/vlib/v2/types/checker.v @@ -4292,6 +4292,7 @@ fn (mut c Checker) infer_generic_type(param_type Type, arg_type Type, mut type_m fn (mut c Checker) call_expr(expr ast.CallExpr) Type { lhs_expr := c.resolve_expr(expr.lhs) + c.warn_if_composed_os_execute(lhs_expr, expr) if lhs_expr is ast.Ident { match lhs_expr.name { 'env' { @@ -4583,6 +4584,55 @@ fn (mut c Checker) call_expr(expr ast.CallExpr) Type { c.error_with_pos('call on non fn: ${fn_.name()}', expr.pos) } +fn (mut c Checker) warn_if_composed_os_execute(lhs_expr ast.Expr, expr ast.CallExpr) { + if !is_os_execute_selector(lhs_expr) || expr.args.len == 0 { + return + } + if os_execute_arg_uses_composed_string(expr.args[0]) { + c.warn_with_pos('os.execute() with a composed string can be unsafe; use os.exec([]string) for argv-style execution', + expr.args[0].pos()) + } +} + +fn is_os_execute_selector(expr ast.Expr) bool { + if expr is ast.SelectorExpr { + if expr.lhs is ast.Ident { + return expr.lhs.name == 'os' && expr.rhs.name == 'execute' + } + } + return false +} + +fn os_execute_arg_uses_composed_string(expr ast.Expr) bool { + return match expr { + ast.StringInterLiteral { + true + } + ast.InfixExpr { + expr.op == .plus || os_execute_arg_uses_composed_string(expr.lhs) + || os_execute_arg_uses_composed_string(expr.rhs) + } + ast.ParenExpr { + os_execute_arg_uses_composed_string(expr.expr) + } + ast.ModifierExpr { + os_execute_arg_uses_composed_string(expr.expr) + } + ast.PrefixExpr { + os_execute_arg_uses_composed_string(expr.expr) + } + ast.CastExpr { + os_execute_arg_uses_composed_string(expr.expr) + } + ast.AsCastExpr { + os_execute_arg_uses_composed_string(expr.expr) + } + else { + false + } + } +} + fn (mut c Checker) array_magic_arg_type(arg ast.Expr, it_type Type) Type { if arg is ast.LambdaExpr { c.open_scope() @@ -5637,9 +5687,14 @@ fn (mut c Checker) error_message(msg string, kind errors.Kind, pos token.Positio errors.error(msg, errors.details(file, pos, 2), kind, pos) } -// fn (mut c Checker) warn(msg string) { -// c.error_message(msg, .warning, p.current_position()) -// } +fn (mut c Checker) warn_with_pos(msg string, pos token.Pos) { + if !pos.is_valid() { + eprintln('warning: ${msg} (no position info)') + return + } + file := c.file_set.file(pos) + c.error_message(msg, .warning, file.position(pos), file) +} // [noreturn] // fn (mut c Checker) error(msg string) { diff --git a/vlib/v2/types/checker_test.v b/vlib/v2/types/checker_test.v index e06a70ea2..12ffd98e2 100644 --- a/vlib/v2/types/checker_test.v +++ b/vlib/v2/types/checker_test.v @@ -5,6 +5,7 @@ module types import os +import v2.ast import v2.parser import v2.pref import v2.token @@ -94,6 +95,46 @@ fn test_basic_literal_string() { assert has_type(env, 'string'), 'string literal should have string type' } +fn test_os_execute_warning_detection() { + os_execute_selector := ast.Expr(ast.SelectorExpr{ + lhs: ast.Ident{ + name: 'os' + } + rhs: ast.Ident{ + name: 'execute' + } + }) + assert is_os_execute_selector(os_execute_selector) + + literal := ast.Expr(ast.StringLiteral{ + value: 'echo ok' + }) + assert !os_execute_arg_uses_composed_string(literal) + + interpolation := ast.Expr(ast.StringInterLiteral{ + values: ['echo ', ''] + inters: [ + ast.StringInter{ + expr: ast.Ident{ + name: 'name' + } + }, + ] + }) + assert os_execute_arg_uses_composed_string(interpolation) + + concat := ast.Expr(ast.InfixExpr{ + op: .plus + lhs: ast.Ident{ + name: 's1' + } + rhs: ast.Ident{ + name: 's2' + } + }) + assert os_execute_arg_uses_composed_string(concat) +} + fn test_iclone_struct_clone_returns_self_type() { env := check_code(' interface IClone {} -- 2.39.5