`command -x` fails to handle long arguments

[siteshwar]

Description of problem:
command -x fails to handle long arguments and crashes in _spawnveg() function.

Ksh version:
2017.0.0-devel-2089-ga16a0486

How reproducible:
Always

Steps to reproduce:

1. Run below script from build directory:

# test command -x option
SHELL=src/cmd/ksh93/ksh

function longline
{
    integer i
    for((i=0; i < $1; i++))
    do
        print argument$i
    done
}

integer sum=0 n=100000
if ! ${SHELL:-ksh} -c 'print $#' count $(longline $n) > /dev/null  2>&1
then
    for i in $(command -p command -x ${SHELL:-ksh} -c 'print $#;[[ $1 != argument0 ]]' count $(longline $n) 2> /dev/null)
    do
        ((sum += $i))
   done

   (( sum == n )) || echo "command -x processed only $sum arguments"
   command -p command -x ${SHELL:-ksh} -c 'print $#;[[ $1 == argument0 ]]' count $(longline $n) > /dev/null  2>&1
   [[ $? != 1 ]] && echo 'incorrect exit status for command -x'
fi

Actual results:
ksh subprocesses crash while running this test.

Expected results:
ksh subprocesses should not crash.

Additional info:
I found this bug while trying to improve tests for command builtin. My changes can be seen in this branch.

[krader1961]

This is because the abstractions for launching a new process are suboptimal if not outright FUBAR. Consider path_spawn():

ast/src/cmd/ksh93/sh/path.c

Lines 1050 to 1055 in 423eb28

	#if USE_SPAWN
	if (spawn > 1) {
	spawnvex_add(shp->vex, SPAWN_pgrp, spawn >> 1, 0, 0);
	}
	spawnvex_add(shp->vex, SPAWN_noexec, 0, vexexec, shp);
	#endif // USE_SPAWN

Followed by this block in path_spawn():

ast/src/cmd/ksh93/sh/path.c

Lines 1134 to 1138 in 423eb28

	if (spawn /* && !sh_isoption(shp,SH_PFSH) */) {
	pid = _spawnveg(shp, opath, &argv[0], envp, spawn >> 1);
	} else {
	pid = path_pfexecve(shp, opath, &argv[0], envp, spawn);
	}

Constrast that with this block in path_xargs() which is the cause of the failure:

ast/src/cmd/ksh93/sh/path.c

Lines 225 to 227 in 423eb28

	if (saveargs || av < avlast || (exitval && !spawn)) {
	if ((pid = _spawnveg(shp, path, argv, envp, 0)) < 0) return -1;
	job_post(shp, pid, 0);

Since USE_SPAWN is false (i.e., zero) that causes the dummy implementation that calls abort() to be invoked. I instrumented the code and the test case causes _spawnveg() to be called when spawn is false (i.e., zero) in the above block of code. There are two places in path_xargs() which does that. Whereas in path_spawn() it doesn't assume _spawnveg() is useful.

Note that _spawnveg() is radically different comparing the ksh93u+ and ksh93v- source code. And those differences are the proximate source of this bug. I say "proximate source" because the real problem is due to the poor abstractions; i.e., poor design.

[krader1961]

The only system I am using where ksh93u+ works "correctly" with regards to this issue is FreeBSD 6.2. All my Linux VMs running this test using the distro ksh93u+ results in an infinite loop. Which isn't surprising since Linux emulates posix_spawn() with a combination of vfork() + clone(). This feature seems to require a real posix_spawn() to work correctly. I don't (yet) know why this fails on macOS since it seems to have a real posix_spawn().

OpenIndiana (a Illumos clone which is a Solaris fork) has ksh93t+ which also goes into an infinite loop.

It looks like command -x is broken almost everywhere. Furthermore, based solely on the man page I challenge anyone to correctly explain what command -x does. I didn't really grasp its intended behavior until I saw it actually work correctly on FreeBSD.

P.S., A much simpler reproducer is this:

function longline
{
    typeset -i i
    for ((i=0; i < $1; i++))
    do
        echo argument$i
    done
}

command -x /bin/echo prefix $(longline 200000) suffix
echo command -x exit status: $?

The larger value to longline compared to the original reproduction step is because my Ubuntu VM has a significantly larger ARG_MAX value. Without that increase the script "passes" on my Ubuntu VM. But that is simply because the total size of all process arguments is under that system's default ARG_MAX value.

[krader1961]

Note that POSIX does not document a command -x option. So the simplest fix is to simply remove this feature from ksh. This feature can probably be implemented in a fashion that works on multiple platforms; including those without a real posix_spawn() or where its use has been disabled at build time. However, doing so will be difficult. Passing exec() errno values to the parent shell in a manner that avoids ambiguity with the exit status of the program to be run is difficult.

[krader1961]

Unsurprisingly my trivial fix to use fork() plus path_pfexecve() if USE_SPAWN is false fails. It causes both the O.P. script and my simpler script to incorrectly succeed without correctly partitioning the argument list. Which is not surprising given my earlier analysis that this feature requires a real posix_spawn() that sets errno to E2BIG in the parent process.

[krader1961]

I augmented my fix to propagate the path_pfexec() errno to the parent process using a pipe so that the E2BIG errno was propagated to the path_spawn() function which checks for that errno. That enhancement does not make this feature work. In fact after pondering, and instrumenting, the code I don't see how it could work correctly even if posix_spawn() is used.

@siteshwar What prompted you to open this issue? Was it a RedHat customer issue? Is there some reason to believe this has worked correctly with older versions of the code? If so can we get access to that source to compare it to the current source?

[McDutchie]

For the record, this bug is now fixed in ksh 93u+m. See ksh93@acf84e9.

[jelmd]

@McDutchie - Not handling any errors and returning an "unexpected" code seems not to be correct. So IMHO this commit smells.
BTW: Are you sure, that this is only used with "command -x ..."? Furthermore: If you think, that the alignment fixes the problem, why is changing the context of the call needed at all?

[McDutchie]

What makes you think it's not handling any errors? Not handling any errors is precisely what it did before this commit. An infinite retry loop was the result.

Are you sure, that this is only used with "command -x ..."?

Yes.

Furthermore: If you think, that the alignment fixes the problem, why is changing the context of the call needed at all?

Sorry, but I have no idea what you're talking about. No "context" has been changed, whatever that means.

[jelmd]

@McDutchie

What makes you think it's not handling any errors?

Hmmm, IMHO it is pretty obvious: the removal of line 1244 and not adding something, which takes care of it.

Not handling any errors is precisely what it did before this commit. An infinite retry loop was the result.

You are completely wrong here. As anyone can see, the error handling happens between line 1173 .. 1240. It might not catch everything or handle the error correctly/sensitive enough, but at least there is an error handling. The mentioned commit replaces error handling with "if the path_xargs() call fails, emit a generic error message and return an arbitrary return code". That's, anything else, but not error handling. In my world, error handling means: checking what error occurred and taking the appropriate related action[s]. The commit does not even have a look at the error and thus no wonder, why it fails to fix the problem.

So IMHO the right question to ask is: Why does a loop happen? Obviously because path_xargs() fails with the same error it should actually fix (and obviously this is part of the error handling BTW). So as long as path_xargs() does not error out with E2BIG, the loop problem goes away.

Are you sure, that this is only used with "command -x ..."?

Yes.

OK - than it probably does not affect too many people ;-)

Furthermore: If you think, that the alignment fixes the problem, why is changing the context of the call needed at all?

Sorry, but I have no idea what you're talking about.

The commit comment says, adding some alignment bytes fixes path_xargs() . But if this fixes path_xargs()], why does one need to change the lines around the call?

No "context" has been changed, whatever that means.

"context" means the lines around the call on line 1242. Here especially the replacement of line 1244 with a very very questionable kludge (i.e. does not fix the problem which it ought to handle, but simply closes its eyes and runs away).

A fix should make command -x ... work reliable and not give up ASAP. Otherwise it is completely useless, because it still cannot be used. So a quick and dirty fix could be to half the assumed available stack space (ARG_MAX) and try again as long as path_xargs()] errors out with E2BIG. A proper fix is to research how the OSs actually handle execve() + friends and to calculate the remaining stack space for args correctly.

BTW: Because on vacation and a rainy day I had a closer look at path_spawn() and path_xargs() incl. the mentioned commit: path_xargs() looked and still looks absolutely buggy. When the remaining space gets determined, neither the trailing \0 nor the last character gets counted. And at least for Solaris I can say for sure, that the alignment thing is a wrong assumption. Solaris determines its remaining stack space by prepending any env, arg and aux string \0 terminated at the bottom of the stack and appends its length (an int) to the top - so no alignment of 16 but 5 extra bytes for each arg, env or aux var (aux var are max 4 strings usually with max. less than 1024 chars for all, so one does not need to find the details for it - (line 162)[https://github.com/att/ast/blob/master/src/cmd/ksh93/sh/path.c#L162] takes care of it). To be precise finally there is some padding used to align the userstack, but because the alignment is 16, these are not more than 15 bytes once (lines 162 takes care of it). I do not know, what other OSs do, but because BSD, Solaris and MacOS have the same roots, my guess is, that they do similar things - alignment of each arg and env by 16 bytes sounds to me too wasteful ...

So wondering how you did your tests of the commit, why you not found, that it doesn't work [at least not on Solaris]?

BTW: Does anyone have some pointers to OpenGroked OS sources for *BSD, Linux?