Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ksh fails to start if current directory is not accessible #567

Closed
siteshwar opened this issue Jun 1, 2018 · 1 comment
Closed

Ksh fails to start if current directory is not accessible #567

siteshwar opened this issue Jun 1, 2018 · 1 comment
Assignees
Labels

Comments

@siteshwar
Copy link
Contributor

Description of problem:
Ksh fails to start if current directory is not accessible

Ksh version:
ksh-20180526+git.945.4ed72ac1-0.fc27.x86_64

How reproducible:
Always

Steps to reproduce:

  1. mkdir /tmp/foo
  2. chmod o-rwx /tmp/foo
  3. useradd test
  4. cd /tmp/foo
  5. su -c "ksh" test

Actual results:
ksh: Can't obtain directory fd. [Permission denied]

and ksh fails to start.

Expected results:
ksh should start.

Additional info:
Here is the code from src/cmd/ksh93/sh/init.c that throws this error:

    // This should _never_ happen, guaranteed by design and goat sacrifice.
    if (shp->pwdfd < 0) errormsg(SH_DICT, ERROR_system(1), "Can't obtain directory fd.");

We should not need another goat sacrifice to fix it.

@siteshwar siteshwar self-assigned this Jun 1, 2018
@krader1961
Copy link
Contributor

Note that this works with ksh93u. It's also not necessary to create a dummy user account. Just do this instead:

  1. mkdir /tmp/foo
  2. cd /tmp/foo
  3. chmod 0 .
  4. ksh

@siteshwar siteshwar added the bug label Jun 2, 2018
siteshwar added a commit that referenced this issue Jun 7, 2018
If ksh is started in a directory that is not accessible to it, it will
exit with an error. This code makes invalid assumption and should be
disabled.

Resolves: #567
siteshwar added a commit that referenced this issue Jun 8, 2018
JohnoKing added a commit to JohnoKing/ksh that referenced this issue Dec 19, 2024
In ksh93v- a sh.pwdfd variable was introduced, obstensibly for
implementing 'cd -f', 'pwd -f' and '${.sh.pwdfd}' as new features. I
might backport these in the future, but those aren't the focus of this
commit. Rather, what caught my interest was the effect of this change on
subshell performance (results from shbench with 20 iterations, compiled
with CCFLAGS='-O2 -fPIC'):

----------------------------------------------------------------------------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93u+m-dev       /tmp/ksh93v-2012-08-24  /tmp/ksh93v-2014-12-24  /tmp/ksh93u+redhat
----------------------------------------------------------------------------------------------------------------------------------
fibonacci.ksh  0.230 [0.227-0.235]     0.216 [0.211-0.222]     0.269 [0.266-0.273]     0.247 [0.242-0.251]     0.264 [0.260-0.269]
parens.ksh     0.208 [0.204-0.212]     0.173 [0.170-0.182]     0.147 [0.145-0.149]     0.125 [0.121-0.127]     0.140 [0.137-0.144]
----------------------------------------------------------------------------------------------------------------------------------

As can be seen above, 93u+ and 93u+m perform worse in the parens
benchmark than the ksh93v- release which implements the change, as well
as a copy of ksh93u+ compiled with Red Hat's patches for cd (fdstatus,
rmdirfix, cdfix and cdfix2 applied in that order). However, 93u+ and
93u+m perform better in the fibonacci benchmark.

93v- is faster in the parens benchmark because it obtains the file
descriptor for the current working directory via openat(2) in
sh_diropenat, then caches it in sh.pwdfd. This fd is then reused in
sh_subshell(), which allows 93v- to avoid taxing open(2) and fcntl(2)
syscalls.

However, the 93v- implementation has a flaw. Unlike in 93u+, it always
duplicates sh.pwdfd with fcntl, even when it's a superfluous operation.
This is the cause of the performance regression in the fibonacci
benchmark.

To increase ksh93u+m's performance beyond both 93u+ and 93v-, I have
backported 93v-'s code, but with a crucial change:
- sh_subshell() no longer duplicates sh.pwdfd with fcntl during
  virtual subshell scope initialization. Rather, a sh_pwdupdate
  function is used in tandem with sh_diropenat in b_cd() to
  do all of this when the directory is being changed.
  This fixes the regression in the fibonacci benchmark.
  sh_pwdupdate will be called whenever sh.pwdfd changes to
  prevent file descriptor leaks while also avoiding erroneous close
  operations on the parent shell's file descriptor for the PWD.

Other changes applied include:
- The removal of support for systems which lack fchdir. ksh93v- did this
  twelve years ago in the oldest archived alpha release, which I think
  is a more than long enough deprecation window.
- If sh.pwdfd is -1 (i.e., the shell was initialized in a nonexistent
  directory), don't fork preemptively. It's more efficient to let cd
  check sp->pwdfd via a short function before attempting to change the
  directory. This allows subshells run in nonexistent directories to
  avoid forking, provided cd is never invoked.
- Removed <=1995 hackery from nv_open and env_init. During SH_INIT
  we should simply always invoke path_pwd(); giving $PWD NV_TAGGED is
  not just unnecessary, but possibly harmful when one attempts to use
  'typeset -t' with the PWD variable.
- With the removal of that code, sh.pwd is guaranteed to get set during
  shell initialization (if it wasn't already). As such, the code no
  longer needs to cope with a NULL pwd and many such checks have
  been removed.
  if statement covering sh.pwd==NULL in sh_subshell() is certainly a
  noop now and can be safely deleted.
- sh.pwd no longer has a pointless and misleading const modifier. This
  allows for the elimination of many casts involving sh.pwd.
- sh.pwdfd is reset every time the pwd is (re)initialized in
  path_pwd().
- libast/features/fcntl.c: Account for systems which don't implement
  O_SEARCH and/or O_DIRECTORY. (I avoided backporting the ksh93v-
  feature test code because it's gianormous and filled with far too
  much compatibility hackery. If anyone wants to backport the mess
  that are the libast syscall intercepts, be my guest. In any case
  ksh doesn't need any of that to function correctly.)
- Goat sacrifices are hereby banned from 93u+m (for the curious,
  please see att#567).

With these changes ksh93u+m now handily beats the previous ksh releases
in the fibonacci and parens benchmarks (ksh2020 included for comparison):

----------------------------------------------------------------------------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93v-2014-12-24  /tmp/ksh2020            /tmp/ksh93u+m-dev       /tmp/ksh93u+m-patch
----------------------------------------------------------------------------------------------------------------------------------
fibonacci.ksh  0.230 [0.224-0.236]     0.247 [0.244-0.252]     0.369 [0.364-0.375]     0.217 [0.213-0.222]     0.202 [0.198-0.206]
parens.ksh     0.207 [0.203-0.213]     0.124 [0.121-0.127]     0.165 [0.162-0.169]     0.173 [0.171-0.176]     0.082 [0.080-0.085]
----------------------------------------------------------------------------------------------------------------------------------
JohnoKing added a commit to JohnoKing/ksh that referenced this issue Dec 19, 2024
In ksh93v- a sh.pwdfd variable was introduced, ostensibly for
implementing 'cd -f', 'pwd -f' and '${.sh.pwdfd}' as new features. I
might backport these in the future, but those aren't the focus of this
commit. Rather, what caught my interest was the effect of this change on
subshell performance (results from shbench with 20 iterations, compiled
with CCFLAGS='-O2 -fPIC'):

----------------------------------------------------------------------------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93u+m-dev       /tmp/ksh93v-2012-08-24  /tmp/ksh93v-2014-12-24  /tmp/ksh93u+redhat
----------------------------------------------------------------------------------------------------------------------------------
fibonacci.ksh  0.230 [0.227-0.235]     0.216 [0.211-0.222]     0.269 [0.266-0.273]     0.247 [0.242-0.251]     0.264 [0.260-0.269]
parens.ksh     0.208 [0.204-0.212]     0.173 [0.170-0.182]     0.147 [0.145-0.149]     0.125 [0.121-0.127]     0.140 [0.137-0.144]
----------------------------------------------------------------------------------------------------------------------------------

As can be seen above, 93u+ and 93u+m perform worse in the parens
benchmark than the ksh93v- release which implements the change, as well
as a copy of ksh93u+ compiled with Red Hat's patches for cd (fdstatus,
rmdirfix, cdfix and cdfix2 applied in that order). However, 93u+ and
93u+m perform better in the fibonacci benchmark.

93v- is faster in the parens benchmark because it obtains the file
descriptor for the current working directory via openat(2) in
sh_diropenat, then caches it in sh.pwdfd. This fd is then reused in
sh_subshell(), which allows 93v- to avoid taxing open(2) and fcntl(2)
syscalls.

However, the 93v- implementation has a flaw. Unlike in 93u+, it always
duplicates sh.pwdfd with fcntl, even when it's a superfluous operation.
This is the cause of the performance regression in the fibonacci
benchmark.

To increase ksh93u+m's performance beyond both 93u+ and 93v-, I have
backported 93v-'s code, but with a crucial change:
- sh_subshell() no longer duplicates sh.pwdfd with fcntl during
  virtual subshell scope initialization. Rather, a sh_pwdupdate
  function is used in tandem with sh_diropenat in b_cd() to
  do all of this when the directory is being changed.
  This fixes the regression in the fibonacci benchmark.
  sh_pwdupdate will be called whenever sh.pwdfd changes to
  prevent file descriptor leaks while also avoiding erroneous close
  operations on the parent shell's file descriptor for the PWD.

Other changes applied include:
- The removal of support for systems which lack fchdir. ksh93v- did this
  twelve years ago in the oldest archived alpha release, which I think
  is a more than long enough deprecation window.
- If sh.pwdfd is -1 (i.e., the shell was initialized in a nonexistent
  directory), don't fork preemptively. It's more efficient to let cd
  check sp->pwdfd via a short function before attempting to change the
  directory. This allows subshells run in nonexistent directories to
  avoid forking, provided cd is never invoked.
- Removed <=1995 hackery from nv_open and env_init. During SH_INIT
  we should simply always invoke path_pwd(); giving $PWD NV_TAGGED is
  not just unnecessary, but possibly harmful when one attempts to use
  'typeset -t' with the PWD variable.
- With the removal of that code, sh.pwd is guaranteed to get set during
  shell initialization (if it wasn't already). As such, the code no
  longer needs to cope with a NULL pwd and many such checks have
  been removed.
- sh.pwd no longer has a pointless and misleading const modifier. This
  allows for the elimination of many casts involving sh.pwd.
- sh.pwdfd is reset every time the pwd is (re)initialized in
  path_pwd().
- libast/features/fcntl.c: Account for systems which don't implement
  O_SEARCH and/or O_DIRECTORY. (I avoided backporting the ksh93v-
  feature test code because it's ginormous and filled with far too
  much compatibility hackery. If anyone wants to backport the mess
  that are the libast syscall intercepts, be my guest. In any case
  ksh doesn't need any of that to function correctly.)
- Goat sacrifices are hereby banned from 93u+m (for the curious,
  please see att#567).

With these changes ksh93u+m now handily beats the previous ksh releases
in the fibonacci and parens benchmarks (ksh2020 included for comparison):

----------------------------------------------------------------------------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93v-2014-12-24  /tmp/ksh2020            /tmp/ksh93u+m-dev       /tmp/ksh93u+m-patch
----------------------------------------------------------------------------------------------------------------------------------
fibonacci.ksh  0.230 [0.224-0.236]     0.247 [0.244-0.252]     0.369 [0.364-0.375]     0.217 [0.213-0.222]     0.202 [0.198-0.206]
parens.ksh     0.207 [0.203-0.213]     0.124 [0.121-0.127]     0.165 [0.162-0.169]     0.173 [0.171-0.176]     0.082 [0.080-0.085]
----------------------------------------------------------------------------------------------------------------------------------
McDutchie pushed a commit to ksh93/ksh that referenced this issue Jan 3, 2025
…#805)

In ksh93v- a sh.pwdfd variable was introduced, ostensibly for
implementing 'cd -f', 'pwd -f' and '${.sh.pwdfd}' as new features.
I might backport these in the future, but those aren't the focus of
this commit. Rather, what caught my interest was the effect of this
change on subshell performance (results from shbench[*] with 20
iterations, compiled with CCFLAGS='-O2 -fPIC'):

-------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93u+m-dev
-------------------------------------------------------------
fibonacci.ksh  0.230 [0.227-0.235]     0.216 [0.21f1-0.222]
parens.ksh     0.208 [0.204-0.212]     0.173 [0.170-0.182]
-------------------------------------------------------------
name           /tmp/ksh93v-2012-08-24  /tmp/ksh93v-2014-12-24
-------------------------------------------------------------
fibonacci.ksh  0.269 [0.266-0.273]     0.247 [0.242-0.251]
parens.ksh     0.147 [0.145-0.149]     0.125 [0.121-0.127]
-------------------------------------------------------------
name           /tmp/ksh93u+redhat
-------------------------------------------------------------
fibonacci.ksh  0.264 [0.260-0.269]
parens.ksh     0.140 [0.137-0.144]
-------------------------------------------------------------

As can be seen above, 93u+ and 93u+m perform worse in the parens
benchmark than the ksh93v- release which implements the change, as
well as a copy of ksh93u+ compiled with Red Hat's patches for cd
(fdstatus, rmdirfix, cdfix and cdfix2 applied in that order).
However, 93u+ and 93u+m perform better in the fibonacci benchmark.

93v- is faster in the parens benchmark because it obtains the file
descriptor for the current working directory via openat(2) in
sh_diropenat, then caches it in sh.pwdfd. This fd is then reused in
sh_subshell(), which allows 93v- to avoid taxing open(2) and
fcntl(2) syscalls.

However, the 93v- implementation has a flaw. Unlike in 93u+, it
always duplicates sh.pwdfd with fcntl, even when it's a superfluous
operation. This is the cause of the performance regression in the
fibonacci benchmark.

To increase ksh93u+m's performance beyond both 93u+ and 93v-, I
have backported 93v-'s code, but with a crucial change:
- sh_subshell() no longer duplicates sh.pwdfd with fcntl during
  virtual subshell scope initialization. Rather, a sh_pwdupdate
  function is used in tandem with sh_diropenat in b_cd() to do all
  of this when the directory is being changed. This fixes the
  regression in the fibonacci benchmark. sh_pwdupdate will be
  called whenever sh.pwdfd changes to prevent file descriptor leaks
  while also avoiding erroneous close operations on the parent
  shell's file descriptor for the PWD.

Other changes applied include:
- The removal of support for systems which lack fchdir. ksh93v- did
  this twelve years ago in the oldest archived alpha release, which
  I think is a more than long enough deprecation window.
- If sh.pwdfd is -1 (i.e., the shell was initialized in a
  nonexistent directory), don't fork preemptively. It's more
  efficient to let cd check sp->pwdfd via a short function before
  attempting to change the directory. This allows subshells run in
  nonexistent directories to avoid forking, provided cd is never
  invoked.
- Removed <=1995 hackery from nv_open and env_init. During SH_INIT
  we should simply always invoke path_pwd(); giving $PWD NV_TAGGED
  is not just unnecessary, but possibly harmful when one attempts
  to use 'typeset -t' with the PWD variable.
- With the removal of that code, sh.pwd is guaranteed to get set
  during shell initialization (if it wasn't already). As such, the
  code no longer needs to cope with a NULL pwd and many such checks
  have been removed.
- sh.pwd no longer has a pointless and misleading const modifier.
  This allows for the elimination of many casts involving sh.pwd.
- sh.pwdfd is reset every time the pwd is (re)initialized in
  path_pwd().
- libast/features/fcntl.c: Account for systems which don't
  implement O_SEARCH and/or O_DIRECTORY. (I avoided backporting the
  ksh93v- feature test code because it's ginormous and filled with
  far too much compatibility hackery. If anyone wants to backport
  the mess that are the libast syscall intercepts, be my guest. In
  any case ksh doesn't need any of that to function correctly.)
- Goat sacrifices are hereby banned from 93u+m (for the curious,
  please see att#567).

With these changes ksh93u+m now handily beats the previous ksh
releases in the fibonacci and parens benchmarks (ksh2020 included
for comparison):

-------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93v-2014-12-24  
-------------------------------------------------------------
fibonacci.ksh  0.230 [0.224-0.236]     0.247 [0.244-0.252]
parens.ksh     0.207 [0.203-0.213]     0.124 [0.121-0.127]
-------------------------------------------------------------
name           /tmp/ksh2020            /tmp/ksh93u+m-dev
-------------------------------------------------------------
fibonacci.ksh  0.369 [0.364-0.375]     0.217 [0.213-0.222]
parens.ksh     0.165 [0.162-0.169]     0.173 [0.171-0.176]
-------------------------------------------------------------
name           /tmp/ksh93u+m-patch
-------------------------------------------------------------
fibonacci.ksh  0.202 [0.198-0.206]
parens.ksh     0.082 [0.080-0.085]
-------------------------------------------------------------

[*] shbench: http://fossil.0branch.com/csb
McDutchie pushed a commit to ksh93/ksh that referenced this issue Jan 3, 2025
…#805)

In ksh93v- a sh.pwdfd variable was introduced, ostensibly for
implementing 'cd -f', 'pwd -f' and '${.sh.pwdfd}' as new features.
I might backport these in the future, but those aren't the focus of
this commit. Rather, what caught my interest was the effect of this
change on subshell performance (results from shbench[*] with 20
iterations, compiled with CCFLAGS='-O2 -fPIC'):

-------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93u+m-dev
-------------------------------------------------------------
fibonacci.ksh  0.230 [0.227-0.235]     0.216 [0.21f1-0.222]
parens.ksh     0.208 [0.204-0.212]     0.173 [0.170-0.182]
-------------------------------------------------------------
name           /tmp/ksh93v-2012-08-24  /tmp/ksh93v-2014-12-24
-------------------------------------------------------------
fibonacci.ksh  0.269 [0.266-0.273]     0.247 [0.242-0.251]
parens.ksh     0.147 [0.145-0.149]     0.125 [0.121-0.127]
-------------------------------------------------------------
name           /tmp/ksh93u+redhat
-------------------------------------------------------------
fibonacci.ksh  0.264 [0.260-0.269]
parens.ksh     0.140 [0.137-0.144]
-------------------------------------------------------------

As can be seen above, 93u+ and 93u+m perform worse in the parens
benchmark than the ksh93v- release which implements the change, as
well as a copy of ksh93u+ compiled with Red Hat's patches for cd
(fdstatus, rmdirfix, cdfix and cdfix2 applied in that order).
However, 93u+ and 93u+m perform better in the fibonacci benchmark.

93v- is faster in the parens benchmark because it obtains the file
descriptor for the current working directory via openat(2) in
sh_diropenat, then caches it in sh.pwdfd. This fd is then reused in
sh_subshell(), which allows 93v- to avoid taxing open(2) and
fcntl(2) syscalls.

However, the 93v- implementation has a flaw. Unlike in 93u+, it
always duplicates sh.pwdfd with fcntl, even when it's a superfluous
operation. This is the cause of the performance regression in the
fibonacci benchmark.

To increase ksh93u+m's performance beyond both 93u+ and 93v-, I
have backported 93v-'s code, but with a crucial change:
- sh_subshell() no longer duplicates sh.pwdfd with fcntl during
  virtual subshell scope initialization. Rather, a sh_pwdupdate
  function is used in tandem with sh_diropenat in b_cd() to do all
  of this when the directory is being changed. This fixes the
  regression in the fibonacci benchmark. sh_pwdupdate will be
  called whenever sh.pwdfd changes to prevent file descriptor leaks
  while also avoiding erroneous close operations on the parent
  shell's file descriptor for the PWD.

Other changes applied include:
- The removal of support for systems which lack fchdir. ksh93v- did
  this twelve years ago in the oldest archived alpha release, which
  I think is a more than long enough deprecation window.
- If sh.pwdfd is -1 (i.e., the shell was initialized in a
  nonexistent directory), don't fork preemptively. It's more
  efficient to let cd check sp->pwdfd via a short function before
  attempting to change the directory. This allows subshells run in
  nonexistent directories to avoid forking, provided cd is never
  invoked.
- Removed <=1995 hackery from nv_open and env_init. During SH_INIT
  we should simply always invoke path_pwd(); giving $PWD NV_TAGGED
  is not just unnecessary, but possibly harmful when one attempts
  to use 'typeset -t' with the PWD variable.
- With the removal of that code, sh.pwd is guaranteed to get set
  during shell initialization (if it wasn't already). As such, the
  code no longer needs to cope with a NULL pwd and many such checks
  have been removed.
- sh.pwd no longer has a pointless and misleading const modifier.
  This allows for the elimination of many casts involving sh.pwd.
- sh.pwdfd is reset every time the pwd is (re)initialized in
  path_pwd().
- libast/features/fcntl.c: Account for systems which don't
  implement O_SEARCH and/or O_DIRECTORY. (I avoided backporting the
  ksh93v- feature test code because it's ginormous and filled with
  far too much compatibility hackery. If anyone wants to backport
  the mess that are the libast syscall intercepts, be my guest. In
  any case ksh doesn't need any of that to function correctly.)
- Goat sacrifices are hereby banned from 93u+m (for the curious,
  please see att#567).

With these changes ksh93u+m now handily beats the previous ksh
releases in the fibonacci and parens benchmarks (ksh2020 included
for comparison):

-------------------------------------------------------------
name           /tmp/ksh93u+2012-08-01  /tmp/ksh93v-2014-12-24
-------------------------------------------------------------
fibonacci.ksh  0.230 [0.224-0.236]     0.247 [0.244-0.252]
parens.ksh     0.207 [0.203-0.213]     0.124 [0.121-0.127]
-------------------------------------------------------------
name           /tmp/ksh2020            /tmp/ksh93u+m-dev
-------------------------------------------------------------
fibonacci.ksh  0.369 [0.364-0.375]     0.217 [0.213-0.222]
parens.ksh     0.165 [0.162-0.169]     0.173 [0.171-0.176]
-------------------------------------------------------------
name           /tmp/ksh93u+m-patch
-------------------------------------------------------------
fibonacci.ksh  0.202 [0.198-0.206]
parens.ksh     0.082 [0.080-0.085]
-------------------------------------------------------------

[*] shbench: http://fossil.0branch.com/csb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants