vlib: os.execute and arguments with linefeeds causes invalid exit_code

[hholst80]

Describe the bug

os.execute does not sanitize arguments properly and the exit_code cannot be trusted.

Reproduction Steps

test1() and test2() should present the same exit code (assuming there is no process 19935)

import os

fn test1()
{
    eprintln("probling 19935")
    pid := "19935"
    status := os.execute("ps $pid")
    eprintln(status)

}

fn test2()
{
    eprintln("probing 19935 with a line feed at the end")
    pid := "19935\n"
    status := os.execute("ps $pid\n")
    eprintln(status)
}

fn main() {
    test1()
    test2()
}

Expected Behavior

test1 and test2 should both return the same exit code (1) given that there is no process 19935.

Current Behavior

test2 function returns exit code 0 even though test1 returns 1 (the expected exit code)

Possible Solution

No response

Additional Information/Context

No response

V version

V 0.4.4 cf7dcfe

Environment details (OS name and version, etc.)

V full version: V 0.4.4 cf7dcfe OS: linux, "Artix Linux" Processor: 8 cpus, 64bit, little endian, Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz

getwd: /home/root vexe: /home/root/Apps/v/v vexe mtime: 2024-02-24 09:52:28

vroot: OK, value: /home/root/Apps/v VMODULES: OK, value: /home/root/.vmodules VTMP: OK, value: /tmp/v_0

Git version: git version 2.44.0 Git vroot status: weekly.2024.10-3-ga867ed6a (6 commit(s) behind V master) .git/config present: true

CC version: cc (GCC) 13.2.1 20230801 thirdparty/tcc status: thirdparty-linux-amd64 40e5cbb5

[!NOTE] You can use the 👍 reaction to increase the issue's priority for developers.

Please note that only the 👍 reaction to the issue itself counts as a vote. Other reactions and those to comments will not be taken into account.

[hholst80]

@felipensp good-first-issue To invite newcomers to start working on vlib?

[HydroH]

Hi, I found there was logic on handling such characters, but they were commented out in a previous commit: https://github.com/vlang/v/commit/1bfcdaa2cca07b553c731dcfa4cebf0f91ad2ed9#diff-61ea029cc6cc2d797366920e750069365bfbdddee28865dd540928cbd6048c95R76 What is the reason behind this?

[hholst80]

Spawn ["sh", "-c", arg] with arg untouched.

I think we should strive to allow exactly the same thing as the native "sh -c" will allow. A little experimentation reveals that we should not strip away the line feeds rather keep them. Control flow characters are perfectly valid to send to sh -c arg as arg. A linefeed will be interpreted as a sequence of statements. One can probably send a shell script as-is as the arg just by keeping the control characters as-is.

sh-5.2# sh -c "$(echo -e "ps 1\n\n")"
  PID TTY      STAT   TIME COMMAND
    1 ?        S      0:00 /sbin/init
sh-5.2# sh -c "ps 1"
  PID TTY      STAT   TIME COMMAND
    1 ?        S      0:00 /sbin/init
sh-5.2# sh -c "$(echo -e "ps\n1\n")"
  PID TTY          TIME CMD
25947 pts/4    00:00:01 fish
26060 pts/4    00:00:00 sh
26105 pts/4    00:00:00 sh
26106 pts/4    00:00:00 ps
sh: line 2: 1: command not found
sh-5.2# sh -c "$(echo -e "echo 1\necho 2")"
1
2
sh-5.2# sh -c "$(echo -e "true\nfalse")"
sh-5.2# echo $?
1
sh-5.2# sh -c "$(echo -e "false\ntrue")"
sh-5.2# echo $?
0
sh-5.2#

Notice that sh responded sh: line 2: 1: command not found there. arg is actually a script. everything allowed in a script should be allowed in arg.

[hholst80]

The tl;dr from the above analysis is that we should feed the linefeeds etc as-is. No filtering should be done.

The issue I ran into is due is the redirection stuff we add on the top. I think we can remove that. popen does nothing of that sort and I think we do not want to add a bunch of logic on top of what the C popen/pclose does.

EDIT: I was too late to the party to have an opinion on that, as we already had tests that checked for the stderr redirection. ;-) Anyways, I reworked how redirection is done to be a separate statement up front. This is likely very fragile and will only work for simple "one liners". If raw Posix shell execution is required, use popen.