Process substitution as file name to redirection is not compiled by shcomp #165

McDutchie · 2021-01-27T23:53:04Z

The commands within a process substitution (<(...) an >(...)) are simply not included in parse trees dumped by shcomp. This can be verified with a command like hexdump -C. As a result, process substitutions don't work when running a bytecode-compiled shell script.

The bug/omission is likely to be somewhere in src/cmd/ksh93/sh/tdump.c which is the code for dumping a parse tree into a file. It may be that src/cmd/ksh93/sh/trestore.c also needs changing.

This problem is also in 93u+, 93v- and ksh2020. Another one of those amazing bugs that have been there for decades. shcomp is basically not usable before this is fixed, unless you know the script does not contain any process substitutions.

The text was updated successfully, but these errors were encountered:

JohnoKing · 2021-04-04T20:35:09Z

So far I've only been able to reproduce this bug if the process substitution is combined with a redirection. This makes me wonder if the bug is similar to #2.

JohnoKing · 2021-04-05T08:08:49Z

From what I've been able to debug, the bug/omission is probably in one of these functions:

ksh/src/cmd/ksh93/sh/tdump.c

Lines 157 to 227 in 56b530c

    
           static int p_arg(register const struct argnod *arg) 
        
           { 
        
           	register int n; 
        
           	struct fornod *fp; 
        
           	while(arg) 
        
           	{ 
        
           		if((n = strlen(arg->argval)) || (arg->argflag&~(ARG_APPEND|ARG_MESSAGE|ARG_QUOTED))) 
        
           			fp=0; 
        
           		else 
        
           		{ 
        
           			fp=(struct fornod*)arg->argchn.ap; 
        
           			n = strlen(fp->fornam)+1; 
        
           		} 
        
           		sfputu(outfile,n+1); 
        
           		if(fp) 
        
           		{ 
        
           			sfputc(outfile,0); 
        
           			outstring(outfile,fp->fornam,n-1); 
        
           		} 
        
           		else 
        
           			outstring(outfile,arg->argval,n); 
        
           		sfputc(outfile,arg->argflag); 
        
           		if(fp) 
        
           		{ 
        
           			sfputu(outfile,fp->fortyp); 
        
           			p_tree(fp->fortre); 
        
           		} 
        
           		else if(n==0 && (arg->argflag&ARG_EXP) && arg->argchn.ap) 
        
           			p_tree((Shnode_t*)arg->argchn.ap); 
        
           		arg = arg->argnxt.ap; 
        
           	} 
        
           	return(sfputu(outfile,0)); 
        
           } 
        
           static int p_redirect(register const struct ionod *iop) 
        
           { 
        
           	while(iop) 
        
           	{ 
        
           		if(iop->iovname) 
        
           			sfputl(outfile,iop->iofile|IOVNM); 
        
           		else 
        
           			sfputl(outfile,iop->iofile); 
        
           		p_string(iop->ioname); 
        
           		if(iop->iodelim) 
        
           		{ 
        
           			p_string(iop->iodelim); 
        
           			sfputl(outfile,iop->iosize); 
        
           			sfseek(sh.heredocs,iop->iooffset,SEEK_SET); 
        
           			sfmove(sh.heredocs,outfile, iop->iosize,-1); 
        
           		} 
        
           		else 
        
           			sfputu(outfile,0); 
        
           		if(iop->iovname) 
        
           			p_string(iop->iovname); 
        
           		iop = iop->ionxt; 
        
           	} 
        
           	return(sfputl(outfile,-1)); 
        
           } 
        
           static int p_comarg(register const struct comnod *com) 
        
           { 
        
           	p_redirect(com->comio); 
        
           	p_arg(com->comset); 
        
           	if(!com->comarg) 
        
           		sfputl(outfile,-1); 
        
           	else if(com->comtyp&COMSCAN) 
        
           		p_arg(com->comarg); 
        
           	else 
        
           		p_comlist((struct dolnod*)com->comarg); 
        
           	return(sfputu(outfile,com->comline)); 
        
           }

When shcomp handles a process substitution without a redirection, it reaches p_comarg. To handle the process substitution, p_comarg calls p_arg, which eventually calls p_tree to handle the commands in the process substitution:

ksh/src/cmd/ksh93/sh/tdump.c

Lines 222 to 223 in 56b530c

    
           else if(com->comtyp&COMSCAN) 
        
           	p_arg(com->comarg);

ksh/src/cmd/ksh93/sh/tdump.c

Lines 184 to 185 in 56b530c

    
           else if(n==0 && (arg->argflag&ARG_EXP) && arg->argchn.ap) 
        
           	p_tree((Shnode_t*)arg->argchn.ap);

Afaict shcomp does handle process substitutions correctly, unless one is combined with a redirection. In the bugged scenario, the p_arg call in the else if block is never reached. One idea I had for fixing this issue is to add a check for this scenario, but that alone doesn't fix the issue (it just causes a memory fault):

--- a/src/cmd/ksh93/sh/tdump.c
+++ b/src/cmd/ksh93/sh/tdump.c
@@ -219,7 +219,7 @@ static int p_comarg(register const struct comnod *com)
 	p_arg(com->comset);
 	if(!com->comarg)
 		sfputl(outfile,-1);
-	else if(com->comtyp&COMSCAN)
+	else if((com->comtyp&COMSCAN) || (com->comio && (com->comio->iofile & IOPROCSUB)))
 		p_arg(com->comarg);
 	else
 		p_comlist((struct dolnod*)com->comarg);

JohnoKing · 2021-04-06T06:25:19Z

Some more information relevant to this bug: when shcomp handles a redirection, the file given to the redirection is handled with p_string:

ksh/src/cmd/ksh93/sh/tdump.c

Lines 197 to 201 in 6b9703f

    
           else 
        
           	sfputl(outfile,iop->iofile); 
        
           p_string(iop->ioname); 
        
           if(iop->iodelim) 
        
           {

When a normal file is passed to a redirection, iop->ioname points to the name of the file. For process substitutions iop->ioname is a newline:

$ cat ./hosts.sh
cat < /etc/hosts
$ hexdump -C <(shcomp ./hosts.sh)
00000000  0b 13 08 00 04 00 00 c0  00 0b 2f 65 74 63 2f 68  |........../etc/h|
00000010  6f 73 74 73 00 40 00 01  04 63 61 74 00 01        |osts.@...cat..|
0000001e

$ cat ./procsub.sh
cat < <(echo foo)
$ hexdump -C <(shcomp ./procsub.sh)
00000000  0b 13 08 00 04 00 00 a0  80 00 03 0a 02 00 40 00  |..............@.|
00000010  01 04 63 61 74 00 01                              |..cat..|
00000017

McDutchie · 2021-04-06T12:40:43Z

I've studied the code in io.c for executing process substitutions with redirections:

ksh/src/cmd/ksh93/sh/io.c

Lines 1176 to 1187 in 6b9703f

    
           if((iof&IOPROCSUB) && !(iof&IOLSEEK)) 
        
           { 
        
           	struct argnod *ap = (struct argnod*)stakalloc(ARGVAL+strlen(iop->ioname)); 
        
           	memset(ap, 0, ARGVAL); 
        
           	if(iof&IOPUT) 
        
           		ap->argflag = ARG_RAW; 
        
           	else if(shp->subshell) 
        
           		sh_subtmpfile(shp->comsub); 
        
           	ap->argchn.ap = (struct argnod*)fname; 
        
           	ap = sh_argprocsub(shp,ap); 
        
           	fname = ap->argval; 
        
           }

So, iop->ioname may start with '\n\0' and appear to be a string containing a newline, but it's actually a pointer to the parse tree for the process substitution and should be handled with a typecast to struct argnod*.

Based on that code, I've come up with the following provisional patch. It needs an extra hack to avoid the segfault in p_arg(). It does cause the command to be included in the shcomp output. Unfortunately, when trying to run it, ksh segfaults.

Note that the sh_argprocsub() call is not included because that causes a file name like /dev/fd/4 to be included in the shcomp output instead of the process substitution's parse tree.

--- a/src/cmd/ksh93/sh/tdump.c
+++ b/src/cmd/ksh93/sh/tdump.c
@@ -219,6 +219,16 @@ static int p_comarg(register const struct comnod *com)
 	p_arg(com->comset);
 	if(!com->comarg)
 		sfputl(outfile,-1);
+	else if(com->comio && (com->comio->iofile & IOPROCSUB))
+	{
+		struct argnod *ap = (struct argnod*)stakalloc(ARGVAL+strlen(com->comio->ioname));
+		memset(ap, 0, ARGVAL);
+		if(com->comio->iofile & IOPUT)
+			ap->argflag = ARG_RAW;
+		ap->argchn.ap = (struct argnod*)com->comio->ioname;
+		((struct fornod*)ap->argchn.ap)->fornam = Empty; /* avoid segfault when calling strlen() in p_arg */
+		p_arg(ap);
+	}
 	else if(com->comtyp&COMSCAN)
 		p_arg(com->comarg);
 	else

This test script now compiles like this:

$ cat test.ksh
echo line 1
cat < <(echo line 2)
$ arch/*/bin/shcomp test.ksh >test.kshc
$ hexdump -C test.kshc
00000000  0b 13 08 00 04 00 00 40  00 03 05 65 63 68 6f 05  |.......@...echo.|
00000010  6c 69 6e 65 02 31 00 01  00 a0 80 00 03 0a 02 00  |line.1..........|
00000020  40 00 02 00 00 84 0a 00  40 00 03 05 65 63 68 6f  |@.......@...echo|
00000030  05 6c 69 6e 65 02 32 00  02 00 02                 |.line.2....|
0000003b
$ arch/*/bin/ksh test.kshc
line 1
Memory fault

Backtrace of the crash:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_platform.dylib            0x00007fff5fa1113c _platform_strchr$VARIANT$Base + 28
1   ksh                                 0x0000000100a34486 sh_exec + 4918 (xec.c:1224)
2   ksh                                 0x00000001009bbde6 exfile + 3254 (main.c:586)
3   ksh                                 0x00000001009bcd30 sh_main + 3392 (main.c:358)
4   ksh                                 0x00000001009a1b36 main + 38 (pmain.c:45)
5   libdyld.dylib                       0x00007fff5f8283d5 start + 1

So it crashes here, in strchr():

ksh/src/cmd/ksh93/sh/xec.c

Line 1224 in 6b9703f

if(!np && !strchr(com0,'/'))

That's as far as I got for now, I hope this is some progress.

JohnoKing · 2021-04-06T20:16:42Z

Implementing some handling for IOPROCSUB in trestore.c fixes the segfault, although process substitutions don't seem to work in the resulting script (also, in the patch below I've moved the IOPROCSUB handling in tdump.c to p_redirect). An improved patch probably should use the result of r_arg:

--- a/src/cmd/ksh93/sh/tdump.c
+++ b/src/cmd/ksh93/sh/tdump.c
@@ -197,6 +197,16 @@ static int p_redirect(register const struct ionod *iop)
 		else
 			sfputl(outfile,iop->iofile);
 		p_string(iop->ioname);
+		if(iop->iofile & IOPROCSUB)
+		{
+			struct argnod *ap = (struct argnod*)stakalloc(ARGVAL+strlen(iop->ioname));
+			memset(ap, 0, ARGVAL);
+			if(iop->iofile & IOPUT)
+				ap->argflag = ARG_RAW;
+			ap->argchn.ap = (struct argnod*)iop->ioname;
+			((struct fornod*)ap->argchn.ap)->fornam = Empty; /* avoid segfault when calling strlen() in p_arg */
+			p_arg(ap);
+		}
 		if(iop->iodelim)
 		{
 			p_string(iop->iodelim);
--- a/src/cmd/ksh93/sh/trestore.c
+++ b/src/cmd/ksh93/sh/trestore.c
@@ -225,6 +225,16 @@ static struct ionod *r_redirect(Shell_t* shp)
 			iopold->ionxt = iop;
 		iop->iofile = l;
 		iop->ioname = r_string(shp->stk);
+		if(iop->iofile & IOPROCSUB)
+		{
+			struct argnod *ap = (struct argnod*)stkalloc(shp->stk,ARGVAL+strlen(iop->ioname));
+			memset(ap, 0, ARGVAL);
+			if(iop->iofile & IOPUT)
+				ap->argflag = ARG_RAW;
+			ap->argchn.ap = (struct argnod*)iop->ioname;
+			((struct fornod*)ap->argchn.ap)->fornam = Empty;
+			r_arg(shp);
+		}
 		if(iop->iodelim = r_string(shp->stk))
 		{
 			iop->iosize = sfgetl(infile);

$ cat ./procsub.sh
echo line 1
cat < <(echo line 2)
echo line 3
$ arch/*/bin/ksh <(arch/*/bin/shcomp ./procsub.sh)
line 1
line 3
$ hexdump -C <(arch/*/bin/shcomp ./procsub.sh)
00000000  0b 13 08 00 04 00 00 40  00 03 05 65 63 68 6f 05  |.......@...echo.|
00000010  6c 69 6e 65 02 31 00 01  00 a0 80 00 03 0a 02 02  |line.1..........|
00000020  00 00 84 0a 00 40 00 03  05 65 63 68 6f 05 6c 69  |.....@...echo.li|
00000030  6e 65 02 32 00 02 00 00  40 00 01 04 63 61 74 00  |ne.2....@...cat.|
00000040  02 00 40 00 03 05 65 63  68 6f 05 6c 69 6e 65 02  |..@...echo.line.|
00000050  33 00 03                                          |3..|
00000053

This bug was first reported at att#9. The <>; operator doesn't work correctly if it's used as the last command of a -c script. Reproducer: $ echo test > a; ksh -c 'echo x 1<>; a'; cat a x st This bug is caused by ksh running the last command of -c scripts with execve(2) instead of posix_spawn(3) or fork(2). The <>; operator is noted by the man page as being incompatible with the exec builtin (see also the ksh93u+ man page), so it's not surprising this bug occurs when ksh runs a command using execve: > <>;word cannot be used with the exec and redirect built-ins. The ksh2020 fix simply removed the code required for ksh to use this optimization at all. It's not a performance friendly fix and only papers over the bug, so this commit provides a better fix. src/cmd/ksh93/sh/xec.c: - The IOREWRITE flag is set when handling the <>; operator, so to fix this bug avoid exec'ing the last command if it uses <>;. See also commit 17ebfbf, which fixed another issue related to the execve optimization. src/cmd/ksh93/tests/io.sh: - Enable a regression test that was failing because of this bug. - Add the reproducer from att#9 as a regression test. src/cmd/ksh93/tests/options.sh: - This bugfix was causing the options regression test to segfault when run under shcomp. The cause is the same as ksh93#165, so as a workaround avoid parsing the command substitution with shcomp. This workaround should also avoid the other problem detailed in ksh93#274.

…re also

The <>; operator doesn't work correctly if it's used as the last command of a -c script. Reproducer: $ echo test > a; ksh -c 'echo x 1<>; a'; cat a x st This bug is caused by ksh running the last command of -c scripts with execve(2) instead of posix_spawn(3) or fork(2). The <>; operator is noted by the man page as being incompatible with the exec builtin (see also the ksh93u+ man page), so it's not surprising this bug occurs when ksh runs a command using execve: > <>;word cannot be used with the exec and redirect built-ins. The ksh2020 fix simply removed the code required for ksh to use this optimization at all. It's not a performance friendly fix and only papers over the bug, so this commit provides a better fix. This bug was first reported at: att#9 In addition, this commit re-enables the execve(2) optimization for the last command for scripts loaded from a file. It was enabled in in older ksh versions, and was only disabled in interactive shells: https://github.com/ksh93/ast-open-history/blob/2011-06-30/src/cmd/ksh93/sh/main.c#L593-L599 It was changed on 2011-12-24 to only be used for -c scripts: https://github.com/ksh93/ast-open-history/blob/2011-12-24/src/cmd/ksh93/sh/main.c#L593-L599 We think there is no good reason why scripts loaded from a file should be optimised less than scripts loaded from a -c argument. They're both scripts; there's no essential difference between them. So this commit reverts that change. If there is a bug left in the optimization after this fix, this revert increases the chance of exposing it so that it can be fixed. src/cmd/ksh93/sh/xec.c: - The IOREWRITE flag is set when handling the <>; operator, so to fix this bug, avoid exec'ing the last command if it uses <>;. See also commit 17ebfbf, which fixed another issue related to the execve optimization. src/cmd/ksh93/tests/io.sh: - Enable a regression test that was failing because of this bug. - Add the reproducer from att#9 as a regression test. src/cmd/ksh93/sh/main.c: - Only avoid the non-forking optimization in interactive shells. src/cmd/ksh93/tests/signal.sh: - Add an extra comment to avoid the non-forking optimization in the regression test for rhbz#1469624. - If the regression test for rhbz#1469624 fails, show the incorrect exit status in the error message. src/cmd/ksh93/tests/builtins.sh, src/cmd/ksh93/tests/options.sh: - This bugfix was causing the options regression test to segfault when run under shcomp. The cause is the same as <#165>, so as a workaround, avoid parsing process substitutions with shcomp until that is fixed. This workaround should also avoid the other problem detailed in <#274>. Resolves: #274

McDutchie · 2021-04-15T18:04:57Z

TODO: after fixing this bug, change these regression tests back to using process substitutions.

ksh/src/cmd/ksh93/tests/builtins.sh

Lines 988 to 1005 in 6701bb3

    
           # Builtins should handle unrecognized options correctly 
        
           # Note: To workaround https://github.com/ksh93/ksh/issues/165, put the list 
        
           # of builtins in a file, then read from that. 
        
           builtin > "$tmp/builtin-list" 
        
           while IFS= read -r bltin <&3 
        
           do	case $bltin in 
        
           	echo | test | true | false | \[ | : | getconf | */getconf | uname | */uname | catclose | catgets | catopen | Dt* | _Dt* | X* | login | newgrp ) 
        
           		continue ;; 
        
           	/*/*)	expect="Usage: ${bltin##*/} " 
        
           		actual=$({ PATH=${bltin%/*}; "${bltin##*/}" --this-option-does-not-exist; } 2>&1) ;; 
        
           	*/*)	err_exit "strange path name in 'builtin' output: $(printf %q "$bltin")" 
        
           		continue ;; 
        
           	*)	expect="Usage: $bltin " 
        
           		actual=$({ "${bltin}" --this-option-does-not-exist; } 2>&1) ;; 
        
           	esac 
        
           	[[ $actual == *"$expect"* ]] || err_exit "$bltin should show usage info on unrecognized options" \ 
        
           			"(expected string containing $(printf %q "$expect"), got $(printf %q "$actual"))" 
        
           done 3< "$tmp/builtin-list"

ksh/src/cmd/ksh93/tests/options.sh

Lines 541 to 545 in 6701bb3

    
           # Note: To workaround erratic behavior caused by https://github.com/ksh93/ksh/issues/165, 
        
           # avoid parsing the process substitutions with shcomp by running 
        
           # the tests with eval. 
        
           eval "$SHELL --posix < <(echo 'exit 0')" || err_exit "ksh fails to handle --posix during startup" 
        
           eval "$SHELL -o posix < <(echo 'exit 0')" || err_exit "ksh fails to handle -o posix during startup"

McDutchie · 2021-04-21T20:12:28Z

If we can't fix this before releasing the beta then we can at least temporarily include this to prevent the generation of broken compiled scripts:

--- a/src/cmd/ksh93/sh/tdump.c
+++ b/src/cmd/ksh93/sh/tdump.c
@@ -219,6 +219,8 @@ static int p_comarg(register const struct comnod *com)
 	p_arg(com->comset);
 	if(!com->comarg)
 		sfputl(outfile,-1);
+	else if(com->comio && (com->comio->iofile & IOPROCSUB))
+		errormsg(SH_DICT,ERROR_exit(1),"process substitution as file name to redirection is not yet implemented for shcomp");
 	else if(com->comtyp&COMSCAN)
 		p_arg(com->comarg);
 	else

JohnoKing · 2021-04-21T20:27:10Z

This is minor, but error messages should be followed by UNREACHABLE() when they don't return:

--- a/src/cmd/ksh93/sh/tdump.c
+++ b/src/cmd/ksh93/sh/tdump.c
@@ -219,6 +219,11 @@ static int p_comarg(register const struct comnod *com)
 	p_arg(com->comset);
 	if(!com->comarg)
 		sfputl(outfile,-1);
+	else if(com->comio && (com->comio->iofile & IOPROCSUB))
+	{
+		errormsg(SH_DICT,ERROR_exit(1),"process substitution as file name to redirection is not yet implemented for shcomp");
+		UNREACHABLE();
+	}
 	else if(com->comtyp&COMSCAN)
 		p_arg(com->comarg);
 	else

On another note, I tested the patch and it causes the io.sh regression tests to fail under shcomp:

test io(shcomp) begins at 2021-04-21+13:16:12
/tmp/ksh93.shtests.3926.633/shcomp-io.ksh.orig: line 48: process substitution as file name to redirection is not yet implemented for shcomp
test io(shcomp) failed to compile at 2021-04-21+13:16:12 with exit code 1 [ 1 test 1 error ]

McDutchie · 2021-04-21T21:30:13Z

The line number in the error message is wrong, it's the line number of the last alias command -- which is explained by the fact that shcomp executes alias commands as it parses them.

Updated patch to give the correct line number:

--- a/src/cmd/ksh93/sh/tdump.c
+++ b/src/cmd/ksh93/sh/tdump.c
@@ -219,6 +219,12 @@ static int p_comarg(register const struct comnod *com)
 	p_arg(com->comset);
 	if(!com->comarg)
 		sfputl(outfile,-1);
+	else if(com->comio && (com->comio->iofile & IOPROCSUB))
+	{
+		error_info.line = com->comline;
+		errormsg(SH_DICT,ERROR_exit(1),"process substitution as file name to redirection is not yet implemented for shcomp");
+		UNREACHABLE();
+	}
 	else if(com->comtyp&COMSCAN)
 		p_arg(com->comarg);
 	else

This shows that the culprit is this command:

$ cat test.ksh
redirect 3<# ((40*8))
$ arch/darwin.i386-64/bin/shcomp test.ksh >/dev/null
test.ksh: line 1: process substitution as file name to redirection is not yet implemented for shcomp

That's clearly wrong. Our detection of a process substitution is broken.

McDutchie · 2021-04-21T22:07:13Z

The io.c code in this comment provided the clue. We need to exclude the IOLSEEK bit flag. This fixes it:

--- a/src/cmd/ksh93/sh/tdump.c
+++ b/src/cmd/ksh93/sh/tdump.c
@@ -219,6 +219,12 @@ static int p_comarg(register const struct comnod *com)
 	p_arg(com->comset);
 	if(!com->comarg)
 		sfputl(outfile,-1);
+	else if(com->comio && (com->comio->iofile & IOPROCSUB) && !(com->comio->iofile & IOLSEEK))
+	{
+		error_info.line = com->comline;
+		errormsg(SH_DICT,ERROR_exit(1),"process substitution as file name to redirection is not yet implemented for shcomp");
+		UNREACHABLE();
+	}
 	else if(com->comtyp&COMSCAN)
 		p_arg(com->comarg);
 	else

Now it seems to work:

$ cat test.ksh
redirect 3<# ((40*8))
cat < <(echo procsub)
$ arch/*/bin/shcomp test.ksh >/dev/null
test.ksh: line 2: process substitution as file name to redirection is not yet implemented for shcomp
$ bin/shtests -c io
#### Regression-testing /usr/local/src/ksh93/ksh/arch/darwin.i386-64/bin/ksh ####
test io(shcomp) begins at 2021-04-21+23:06:49
test io(shcomp) passed at 2021-04-21+23:06:52 [ 136 tests 0 errors ]
Total errors: 0
CPU time       user:      system:
main:      0m00.008s    0m00.020s
tests:     0m00.779s    0m00.866s

McDutchie · 2021-04-21T22:11:55Z

Hmm, no, it's still broken. Only the first redirection is checked for a process substitution. If any regular redirection precedes a redirection with a process substitution, the latter is ignored again.

This makes sense. The fix should probably be implemented in p_redirect which is called from p_comarg.

McDutchie · 2021-04-21T22:16:26Z

New patch:

--- a/src/cmd/ksh93/sh/tdump.c
+++ b/src/cmd/ksh93/sh/tdump.c
@@ -194,6 +194,11 @@ static int p_redirect(register const struct ionod *iop)
 	{
 		if(iop->iovname)
 			sfputl(outfile,iop->iofile|IOVNM);
+		else if((iop->iofile & IOPROCSUB) && !(iop->iofile & IOLSEEK))
+		{
+			errormsg(SH_DICT,ERROR_exit(1),"process substitution as file name to redirection is not yet implemented for shcomp");
+			UNREACHABLE();
+		}
 		else
 			sfputl(outfile,iop->iofile);
 		p_string(iop->ioname);
@@ -215,6 +220,7 @@ static int p_redirect(register const struct ionod *iop)
 
 static int p_comarg(register const struct comnod *com)
 {
+	error_info.line = com->comline;
 	p_redirect(com->comio);
 	p_arg(com->comset);
 	if(!com->comarg)

Now it does seem to be detected correctly:

$ cat test.ksh
redirect 3<# ((40*8))
cat >&2 < <(echo procsub)
$ arch/*/bin/shcomp test.ksh >/dev/null
test.ksh: line 2: process substitution as file name to redirection is not yet implemented for shcomp

McDutchie · 2021-04-21T23:13:12Z

I think I've fixed it. Once I started to understand some of this code, it was ridiculously simple. A process substitution as the argument to a redirection is encoded as a complete parse tree that is used as the file name for the redirection.

--- a/src/cmd/ksh93/sh/tdump.c
+++ b/src/cmd/ksh93/sh/tdump.c
@@ -196,7 +196,10 @@ static int p_redirect(register const struct ionod *iop)
 			sfputl(outfile,iop->iofile|IOVNM);
 		else
 			sfputl(outfile,iop->iofile);
-		p_string(iop->ioname);
+		if((iop->iofile & IOPROCSUB) && !(iop->iofile & IOLSEEK))
+			p_tree((Shnode_t*)iop->ioname);	/* process substitution as file name to redirection */
+		else
+			p_string(iop->ioname);		/* file name, descriptor, etc. */
 		if(iop->iodelim)
 		{
 			p_string(iop->iodelim);
--- a/src/cmd/ksh93/sh/trestore.c
+++ b/src/cmd/ksh93/sh/trestore.c
@@ -224,7 +224,10 @@ static struct ionod *r_redirect(Shell_t* shp)
 		else
 			iopold->ionxt = iop;
 		iop->iofile = l;
-		iop->ioname = r_string(shp->stk);
+		if((l & IOPROCSUB) && !(l & IOLSEEK))
+			iop->ioname = (char*)r_tree(shp);  /* process substitution as file name to redirection */
+		else
+			iop->ioname = r_string(shp->stk);  /* file name, descriptor, etc. */
 		if(iop->iodelim = r_string(shp->stk))
 		{
 			iop->iosize = sfgetl(infile);

After:

$ cat test.ksh                         
cat >&2 < <(echo procsub)
$ arch/*/bin/shcomp test.ksh >test.kshc 
$ arch/*/bin/ksh test.kshc
procsub

The commands within a process substitution used as an argument to a redirection (e.g. < <(...) or > >(...)) are simply not included in parse trees dumped by shcomp. This can be verified with a command like hexdump -C. As a result, these process substitutions do not work when running a bytecode-compiled shell script. The fix is surprisingly simple. A process substitution is encoded as a complete parse tree. When used with a redirection, that parse tree is used as the file name for the redirection. All we need to do is treat the "file name" as a parse tree instead of a string if flags indicate a process substitution. A process substitution is detected by the struct ionod field 'iofile'. Checking the IOPROCSUB bit flag is not enough. We also need to exclude the IOLSEEK flag as that form of redirection may use the IOARITH flag which has the same bit value as IOPROCSUB (see include/shnodes.h). src/cmd/ksh93/sh/tdump.c: p_redirect(): - Call p_tree() instead of p_string() for a process substitution. src/cmd/ksh93/sh/trestore.c: r_redirect(): - Call r_tree() instead of r_string() for a process substitution. src/cmd/ksh93/include/version.h: - Bump the shcomp binary header version as this change is not backwards compatible; previous trestore.c versions don't know how to read the newly compiled process substitutions and would crash. src/cmd/ksh93/tests/io.sh: - Add test. src/cmd/ksh93/tests/builtins.sh, src/cmd/ksh93/tests/options.sh: - Revert shcomp workarounds. (re: 6701bb3) Resolves: #165

JohnoKing · 2021-04-22T08:50:48Z

~~I think you closed this issue a little too soon. Output process substitutions with redirections still don't work with shcomp:~~ edit: I was testing against the wrong git branch, see next comments below.

$ cat /tmp/bar
echo ok > >(sed 's/ok/good/g')
wait
$ arch/*/bin/ksh <(arch/*/bin/shcomp /tmp/bar)
$ arch/*/bin/ksh /tmp/bar                                 
good

McDutchie · 2021-04-22T10:40:20Z

I think you closed this issue a little too soon. Output process substitutions with redirections still don't work with shcomp:
$ cat /tmp/bar
echo ok > >(sed 's/ok/good/g')
wait
$ arch/*/bin/ksh <(arch/*/bin/shcomp /tmp/bar)
$ arch/*/bin/ksh /tmp/bar                                 
good

Are you sure? Your reproducer above works for me on macOS and Linux. An output process substitution is now also tested in io.sh and it passes on the CI runners.

You're using a process substitution to run the reproducer, which may confuse things. What shell and version are you using to invoke the reproducer? Do process substitutions work on it?

JohnoKing · 2021-04-22T10:46:45Z

Nevermind. I thought I was testing against the latest commit, but I was using the wrong git branch (which was one commit behind). I'm now using the correct git commit and process substitutions work fine with shcomp now. Feel free to close this issue.

McDutchie · 2021-04-22T11:20:28Z

Excellent. Thanks for keeping an eye on things.

McDutchie added the bug Something is not working label Jan 27, 2021

McDutchie mentioned this issue Apr 3, 2021

Fix high priority bugs before releasing 1.0.0 beta 1 #252

Closed

McDutchie changed the title ~~Process substitution is not compiled by shcomp~~ Process substitution as file name to redirection is not compiled by shcomp Apr 6, 2021

McDutchie mentioned this issue Apr 13, 2021

shcomp test option issue with invalid variable name #274

Closed

JohnoKing mentioned this issue Apr 15, 2021

Fix the <>; redirection operator in -c scripts #277

Merged

JohnoKing added a commit to JohnoKing/ksh that referenced this issue Apr 15, 2021

To workaround another segfault caused by ksh93#165, avoid procsubs he…

86900f4

…re also

McDutchie closed this as completed in 6e037da Apr 22, 2021

McDutchie reopened this Apr 22, 2021

McDutchie closed this as completed Apr 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Process substitution as file name to redirection is not compiled by shcomp #165

Process substitution as file name to redirection is not compiled by shcomp #165

McDutchie commented Jan 27, 2021

JohnoKing commented Apr 4, 2021

JohnoKing commented Apr 5, 2021

JohnoKing commented Apr 6, 2021

McDutchie commented Apr 6, 2021 •

edited

Loading

JohnoKing commented Apr 6, 2021 •

edited

Loading

McDutchie commented Apr 15, 2021

McDutchie commented Apr 21, 2021

JohnoKing commented Apr 21, 2021

McDutchie commented Apr 21, 2021

McDutchie commented Apr 21, 2021

McDutchie commented Apr 21, 2021

McDutchie commented Apr 21, 2021

McDutchie commented Apr 21, 2021 •

edited

Loading

JohnoKing commented Apr 22, 2021 •

edited

Loading

McDutchie commented Apr 22, 2021

JohnoKing commented Apr 22, 2021 •

edited

Loading

McDutchie commented Apr 22, 2021

Process substitution as file name to redirection is not compiled by shcomp #165

Process substitution as file name to redirection is not compiled by shcomp #165

Comments

McDutchie commented Jan 27, 2021

JohnoKing commented Apr 4, 2021

JohnoKing commented Apr 5, 2021

JohnoKing commented Apr 6, 2021

McDutchie commented Apr 6, 2021 • edited Loading

JohnoKing commented Apr 6, 2021 • edited Loading

McDutchie commented Apr 15, 2021

McDutchie commented Apr 21, 2021

JohnoKing commented Apr 21, 2021

McDutchie commented Apr 21, 2021

McDutchie commented Apr 21, 2021

McDutchie commented Apr 21, 2021

McDutchie commented Apr 21, 2021

McDutchie commented Apr 21, 2021 • edited Loading

JohnoKing commented Apr 22, 2021 • edited Loading

McDutchie commented Apr 22, 2021

JohnoKing commented Apr 22, 2021 • edited Loading

McDutchie commented Apr 22, 2021

McDutchie commented Apr 6, 2021 •

edited

Loading

JohnoKing commented Apr 6, 2021 •

edited

Loading

McDutchie commented Apr 21, 2021 •

edited

Loading

JohnoKing commented Apr 22, 2021 •

edited

Loading

JohnoKing commented Apr 22, 2021 •

edited

Loading