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:
1bfcdaa#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.