Project

General

Profile

Actions

Bug #13557

closed

pf[k]sh fails to properly resolve paths

Added by Andy Fiddaman 7 months ago. Updated 7 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
cmd - userland programs
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

Following the update to ksh93u+ in #13405, the ksh profile shells fail to properly resolve paths. This bug appears to mostly manifest in a non-global zone, and only if the profile shell is started with an absolute path.

Reproducible test case from Brian Bennett:

#!/usr/bin/pfksh

# This will fail
ls -1 / | head -1

# This will succeed
/bin/ls -1 / | head -1

In an OmniOS bloody global zone, it works:

bloody% ./bahksh
HVMamirpool
HVMamirpool

and in a NGZ, it fails:

root@sparse:~# /bahksh
pfexec: unable to execute : No such file or directory
bahksh

and, interactively, it only fails if the interpreter is started with an absolute path:

root@sparse:~# pfksh
u@h:w$ ls -l | head -1
total 0
u@h:w$ /bin/ls -l | head -1
total 0

root@sparse:~# /usr/bin/pfksh
u@h:w$ ls -l | head -1
pfexec: unable to execute : No such file or directory
u@h:w$ /bin/ls -l | head -1
total 0
Actions #1

Updated by Electric Monk 7 months ago

  • Gerrit CR set to 1268
Actions #2

Updated by Andy Fiddaman 7 months ago

  • Description updated (diff)
Actions #3

Updated by Andy Fiddaman 7 months ago

In the original code, there are a number of problems contributing to this bad behaviour.

First, the path_xattr() function is supposed to update any buffer passed in as its third argument with the resolved path as a side effect of checking if a path is listed in exec_attr without any attributes. It fails because it resolves the path using a local variable and does not touch the passed-in path.

Second, if path resolution fails (or is never run) - i.e. path_xattr() returns -1 - then path_pfexecve() ploughs on anyway, trying to use pfexec to launch whatever junk is in the resolvedpath stack variable. This happens for any command which is not found via the exec_attr lookup.

Thirdly, the path_pfexecve() function uses vfork() and it should not. The only safe thing to do after vfork() is an exec, and this code does a lot more than that.

Actions #4

Updated by Andy Fiddaman 7 months ago

Following discussion on IRC, rather than trying to fix the shell's built-in profile shell handling, I am going to completely disable it. Paths such as /usr/bin/pfksh are hardlinks to /usr/bin/pfexec so are already using the New implementation of pfexec(1) and all of the profile shells.

Actions #5

Updated by Andy Fiddaman 7 months ago

Having rebuilt ksh without the SHOPT_PFOPT flag, I've tested pfsh in the global and non-global zone and everything works as expected.

For example:

bloody# su - bob
The illumos Project     gate-ig_13557_pfksh-fe34df72a4d February 2021
bob@bloody:~$
bob@bloody:~$
bob@bloody:~$ profiles
File System Management
SMB Management
VSCAN Management
SMBFS Management
Basic Solaris User
All
bob@bloody:~$
bob@bloody:~$ /usr/bin/mkdir /root/fff
mkdir: Failed to make directory "/root/fff"; Permission denied
bob@bloody:~$
bob@bloody:~$ pfsh
bob@bloody:~$ /usr/bin/mkdir /root/fff
bob@bloody:~$ /usr/bin/touch /root/fff
touch: cannot stat /root/fff: Permission denied
bob@bloody:~$

As with the previous ksh version, builtins still get in the way (which was one of the improvements in the newer ksh, but given the state of the pfsh code, I don't want to pursue that option)

bob@build:~$ pfsh
bob@build:~$ mkdir /root/fff
mkdir: /root/fff: [Permission denied]
bob@build:~$ /usr/bin/mkdir /root/fff
bob@build:~$
Actions #6

Updated by Electric Monk 7 months ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100

git commit 944411b5616a6d498985ec4579bd6448b8baacfe

commit  944411b5616a6d498985ec4579bd6448b8baacfe
Author: Andy Fiddaman <omnios@citrus-it.co.uk>
Date:   2021-02-20T23:36:01.000Z

    13557 pf[k]sh fails to properly resolve paths
    Reviewed by: Rich Lowe <richlowe@richlowe.net>
    Reviewed by: Dominik Hassler <hadfl@omnios.org>
    Approved by: Gordon Ross <gordon.w.ross@gmail.com>

Actions #7

Updated by Brian Bennett 7 months ago

Sorry I didn't get a chance to comment on this earlier, but this patch still doesn't work for me, but the behavior is different from the original.

[root@87740fdf-65ff-6aeb-a5d0-baa4db5b7ecb ~]# cat test.sh 
#!/usr/bin/pfksh

/bin/echo This will fail
ls -1 / | /usr/bin/head

/bin/echo  This will succeed
/bin/ls -1 / | /usr/bin/head
[root@87740fdf-65ff-6aeb-a5d0-baa4db5b7ecb ~]# ./test.sh 
This will fail
pfexec: unable to execute ???????4??????ҁ#????`
: No such file or directory
This will succeed
bin
checkpoints
dev
etc
home
lib
opt
proc
root
sbin
Actions #8

Updated by Andy Fiddaman 7 months ago

Is that with the final patch that got committed?

I could replicate your original problem in a non-global zone, but the latest patch works for me in the GZ and NGZ (and disables the shell's built-in profile shell handling entirely).

I have managed to replicate this now and will get this fixed in gate. The patch that was committed to gate was missing a couple of small pieces.

Actions #9

Updated by Electric Monk 7 months ago

git commit 6859ab0054a96093394fe85f055fac9cf93b38ee

commit  6859ab0054a96093394fe85f055fac9cf93b38ee
Author: Andy Fiddaman <omnios@citrus-it.co.uk>
Date:   2021-02-21T21:57:06.000Z

    13557 pf[k]sh fails to properly resolve paths (missing diffs)
    Reviewed by: Rich Lowe <richlowe@richlowe.net>
    Reviewed by: Dominik Hassler <hadfl@omnios.org>
    Approved by: Gordon Ross <gordon.w.ross@gmail.com>

Actions #10

Updated by Brian Bennett 7 months ago

Confirmed, with the second patch it now works.

Thanks!

Actions

Also available in: Atom PDF