bug involving the handling of paths marked PATH_SKIP

[krader1961]

While working on #1445 I was surprised to find that the fix worked on almost all platforms, both my local ones and on Travis CI. One platform where it failed was Ubuntu. After many hours adding strategic DPRINTF() calls I found the bug is in path_opentype(). Specifically, how it handles path entries tagged PATH_SKIP because they do not exist (or can't be stat()'d for some reason) or are duplicates of other $PATH entries. This happens on Ubuntu when using the $PATH in the script below because /bin is a symlink to /usr/bin which means /bin is flagged to be skipped. That it is the final entry in $PATH is important.

This bug exists in the ksh93u+ release as well as git master. You can trigger the bug by saving the following to /tmp/path_skip:

mkdir -p /tmp/usr/bin
print '#!/bin/sh' >/tmp/usr/bin/cd
print 'builtin cd "$@"' >>/tmp/usr/bin/cd
prefix=/tmp/ksh.$$

FPATH=$prefix/functions
mkdir -p "$FPATH"
print 'function cd { echo "Function cd called with |$*|"; command cd "$@"; }' >$prefix/functions/cd
typeset -fu cd

PATH="$PATH:/tmp/usr/bin:/tmp/bin" # fails
#PATH="$PATH:/tmp/usr/bin" # works

set -x
cd /usr
pwd

Running ksh /tmp/path_skip xyz will fail with this output:

+ cd /usr
+ builtin cd xyz
builtin: xyz: not found
/Users/krader/path_skip: function, built-in or type definition for cd not found in /tmp/usr/bin/cd

The content of the /tmp/usr/bin/cd file created by the above script is from Fedora 28 but an equivalent script is found on most platforms. Notice that what ksh has done is source'd that script. The builtin: xyz: not found error is because

a) builtin in ksh has different behavior than in bash, and

b) ksh did not set $@ to the args (i.e., /usr) that were passed to the command. Instead it passed the shell's top-level argv.

We may not be able to change (a) above since doing so without breaking backward compatibility is probably not possible. But (b) looks like a bug to me. On the other hand it's possible this seemingly incorrect behavior will never occur if the core bug this issue documents is fixed.

P.S., As I was writing this it occurred to me to test all the ksh93u+ binaries I have ready access to. All of them, with the exception of OpenSuse 42, failed. It turns out OpenSuse has a "fix" for this bug:

--- ./src/cmd/ksh93/sh/path.c.orig      2014-10-09 15:50:51.198269322 +0000
+++ ./src/cmd/ksh93/sh/path.c   2014-10-09 15:51:16.351159405 +0000
@@ -517,8 +517,8 @@ static int  path_opentype(Shell_t *shp,co
        do
        {
                pp = path_nextcomp(shp,oldpp=pp,name,0);
-               while(oldpp && (oldpp->flags&PATH_SKIP))
-                       oldpp = oldpp->next;
+               if (oldpp && (oldpp->flags&PATH_SKIP))
+                       continue;
                if(fun && (!oldpp || !(oldpp->flags&PATH_FPATH)))
                        continue;
                if((fd = sh_open(path_relative(shp,stakptr(PATH_OFFSET)),O_RDONLY,0)) >= 0)

The scare-quotes are intentional because it only papers over the bug and doesn't address the more fundamental problems with the structure of path_opentype().

[krader1961]

I forgot to mention another curious thing. In the reproduction script if you insert type -a cd just before the cd command that "fixes" the problem. Apparently the type -a causes a different code path to be executed that correctly loads the function; thus bypassing the bug in path_opentype(). This is a great example for why it is so important to boil down a reproduction script to just the essential elements, if at all possible. And, when exploring what changes alter the behavior, being careful to make as few changes as possible each time you test a new variant of the initial conditions.