1
0
Fork 0
mirror of git://git.code.sf.net/p/cdesktopenv/code synced 2025-03-09 15:50:02 +00:00

Yet more fixes for subshell directory handling (re: feaf718f)

There were still problems left after the previous commit. On at
least one system (QNX i386), the following regression test crashed:

src/cmd/ksh93/test/subshell.c
900     got=$( { "$SHELL" -c '(cd /; (cd /)); print -r -- "PWD=$PWD"'; } 2>&1 )

A backtrace done on the core dunp pointed to the free() call here:

src/cmd/ksh93/bltins/cd_pwd.c
90      if(oldpwd && oldpwd!=shp->pwd && oldpwd!=e_dot)
91              free(oldpwd);

Analysis: The interaction between $PWD, sh.pwd aka shp->pwd, and
the path_pwd() function is a mess. path_pwd() usually returns a
freeable value, but not always. sh.pwd is sometimes a pointer to
the value of $PWD, but not always (e.g. when you unset PWD or
assign to it). Instead of debugging the exact cause of the crash, I
think it is better to make this work in a more consistent way.

As of this commit:
1. sh.pwd keeps its own copy of the PWD, independently of the PWD
   variable. The old value must always be freed immediately before
   assigning a new one. This is simple and consistent, reducing the
   chance of bugs at negligible cost.
2. The PWD variable is no longer given the NV_NOFREE attribute
   because its value no longer points to sh.pwd. It is now a
   variable like any other.

src/cmd/ksh93/sh/path.c: path_pwd():
- Do not give PWDNOD the NV_NOFREE attribute.
- Give sh.pwd its own copy of the PWD by strdup'ing PWDNOD's value.

src/cmd/ksh93/bltins/cd_pwd.c: b_cd():
- Since sh.pwd is now consistently freed before giving it a new
  value and at no other time, oldpwd must not be freed any longer
  and can become a regular non-static variable.
- If the PWD needs reinitialising, call path_pwd() to do it.

src/cmd/ksh93/sh/subshell.c: sh_subshell():
- Systems with fchdir(2): Always restore the PWD upon exiting a
  non-subshare subshell. The check to decide whether or not to
  restore it was unsafe: it was not restored if the current PWD
  pointer and value was identical to the saved one, but a directory
  can be deleted and recreated under the same name.
- Systems without fchdir(2) (if any exist):
  . Entry: Fork if the PWD is nonexistent or has no x permission.
  . Restore: Only chdir back if the subshell PWD was changed.
  That's probably the best we can do. It remains inherently unsafe.
  We should probably just require fchdir(2) at some point.
This commit is contained in:
Martijn Dekker 2021-04-20 05:39:10 +01:00
parent feaf718f16
commit 61e0f90460
3 changed files with 41 additions and 43 deletions

View file

@ -57,7 +57,7 @@ int b_cd(int argc, char *argv[],Shbltin_t *context)
register Shell_t *shp = context->shp; register Shell_t *shp = context->shp;
int saverrno=0; int saverrno=0;
int rval,flag=0; int rval,flag=0;
static char *oldpwd; char *oldpwd;
Namval_t *opwdnod, *pwdnod; Namval_t *opwdnod, *pwdnod;
if(sh_isoption(SH_RESTRICTED)) if(sh_isoption(SH_RESTRICTED))
{ {
@ -87,8 +87,6 @@ int b_cd(int argc, char *argv[],Shbltin_t *context)
errormsg(SH_DICT,ERROR_usage(2),"%s",optusage((char*)0)); errormsg(SH_DICT,ERROR_usage(2),"%s",optusage((char*)0));
UNREACHABLE(); UNREACHABLE();
} }
if(oldpwd && oldpwd!=shp->pwd && oldpwd!=e_dot)
free(oldpwd);
oldpwd = path_pwd(shp,0); oldpwd = path_pwd(shp,0);
opwdnod = sh_scoped(shp,OLDPWDNOD); opwdnod = sh_scoped(shp,OLDPWDNOD);
pwdnod = sh_scoped(shp,PWDNOD); pwdnod = sh_scoped(shp,PWDNOD);
@ -229,25 +227,25 @@ success:
while(--flag>0 && dir[flag]=='/') while(--flag>0 && dir[flag]=='/')
dir[flag] = 0; dir[flag] = 0;
nv_putval(pwdnod,dir,NV_RDONLY); nv_putval(pwdnod,dir,NV_RDONLY);
nv_onattr(pwdnod,NV_EXPORT);
if(shp->pwd)
free((void*)shp->pwd);
shp->pwd = sh_strdup(pwdnod->nvalue.cp);
} }
else else
{ {
/* pathcanon() failed to canonicalize the directory, which happens when 'cd' is invoked from a /* pathcanon() failed to canonicalize the directory, which happens when 'cd' is invoked from a
nonexistent PWD with a relative path as the argument. Reinitialize $PWD as it will be wrong. */ nonexistent PWD with a relative path as the argument. Reinitialize $PWD as it will be wrong. */
char *cp = getcwd(NIL(char*),0); if(shp->pwd)
if(cp) free((void*)shp->pwd);
{ shp->pwd = NIL(const char*);
nv_putval(pwdnod,cp,NV_RDONLY); path_pwd(shp,0);
free(cp); if(*shp->pwd != '/')
}
else
{ {
errormsg(SH_DICT,ERROR_system(1),e_direct); errormsg(SH_DICT,ERROR_system(1),e_direct);
UNREACHABLE(); UNREACHABLE();
} }
} }
nv_onattr(pwdnod,NV_NOFREE|NV_EXPORT);
shp->pwd = pwdnod->nvalue.cp;
nv_scan(sh_subtracktree(1),rehash,(void*)0,NV_TAGGED,NV_TAGGED); nv_scan(sh_subtracktree(1),rehash,(void*)0,NV_TAGGED,NV_TAGGED);
path_newdir(shp,shp->pathlist); path_newdir(shp,shp->pathlist);
path_newdir(shp,shp->cdpathlist); path_newdir(shp,shp->cdpathlist);

View file

@ -256,6 +256,7 @@ char *path_pwd(Shell_t *shp,int flag)
{ {
/* Check if $HOME is a path to the PWD; this ensures $PWD == $HOME /* Check if $HOME is a path to the PWD; this ensures $PWD == $HOME
at login, even if $HOME is a path that contains symlinks */ at login, even if $HOME is a path that contains symlinks */
char tofree = 0;
cp = nv_getval(sh_scoped(shp,HOME)); cp = nv_getval(sh_scoped(shp,HOME));
if(!(cp && *cp=='/' && test_inode(cp,e_dot))) if(!(cp && *cp=='/' && test_inode(cp,e_dot)))
{ {
@ -263,17 +264,19 @@ char *path_pwd(Shell_t *shp,int flag)
cp = getcwd(NIL(char*),0); cp = getcwd(NIL(char*),0);
if(!cp) if(!cp)
return((char*)e_dot); return((char*)e_dot);
tofree++;
} }
/* Store in PWD variable */ /* Store in PWD variable */
if(shp->subshell) if(shp->subshell)
pwdnod = sh_assignok(pwdnod,1); pwdnod = sh_assignok(pwdnod,1);
nv_offattr(pwdnod,NV_NOFREE);
nv_putval(pwdnod,cp,NV_RDONLY); nv_putval(pwdnod,cp,NV_RDONLY);
if(tofree)
free(cp);
} }
nv_onattr(pwdnod,NV_NOFREE|NV_EXPORT); nv_onattr(pwdnod,NV_EXPORT);
/* Set shell PWD */ /* Set shell PWD */
shp->pwd = (char*)(pwdnod->nvalue.cp); shp->pwd = sh_strdup(pwdnod->nvalue.cp);
return(cp); return((char*)shp->pwd);
} }
/* /*

View file

@ -82,7 +82,6 @@ static struct subshell
pid_t subpid; /* child process id */ pid_t subpid; /* child process id */
Sfio_t* saveout;/* saved standard output */ Sfio_t* saveout;/* saved standard output */
char *pwd; /* present working directory */ char *pwd; /* present working directory */
const char *shpwd; /* saved pointer to sh.pwd */
void *jobs; /* save job info */ void *jobs; /* save job info */
mode_t mask; /* saved umask */ mode_t mask; /* saved umask */
short tmpfd; /* saved tmp file descriptor */ short tmpfd; /* saved tmp file descriptor */
@ -213,7 +212,6 @@ void sh_subfork(void)
else else
{ {
/* this is the child part of the fork */ /* this is the child part of the fork */
/* setting subpid to 1 causes subshell to exit when reached */
sh_onstate(SH_FORKED); sh_onstate(SH_FORKED);
subshell_data = 0; subshell_data = 0;
shp->subshell = 0; shp->subshell = 0;
@ -561,9 +559,6 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
path_get(shp,e_dot); path_get(shp,e_dot);
shp->pathinit = 0; shp->pathinit = 0;
} }
#if _lib_fchdir
sp->pwdfd = -1;
#endif /* _lib_fchdir */
if(!shp->pwd) if(!shp->pwd)
path_pwd(shp,0); path_pwd(shp,0);
sp->bckpid = shp->bckpid; sp->bckpid = shp->bckpid;
@ -584,8 +579,8 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
if(!shp->subshare) if(!shp->subshare)
{ {
struct subshell *xp; struct subshell *xp;
sp->shpwd = shp->pwd;
#if _lib_fchdir #if _lib_fchdir
sp->pwdfd = -1;
for(xp=sp->prev; xp; xp=xp->prev) for(xp=sp->prev; xp; xp=xp->prev)
{ {
if(xp->pwdfd>0 && xp->pwd && strcmp(xp->pwd,shp->pwd)==0) if(xp->pwdfd>0 && xp->pwd && strcmp(xp->pwd,shp->pwd)==0)
@ -687,6 +682,17 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
#if _lib_fchdir #if _lib_fchdir
if(sp->pwdfd < 0 && !shp->subshare) /* if we couldn't get a file descriptor to our PWD ... */ if(sp->pwdfd < 0 && !shp->subshare) /* if we couldn't get a file descriptor to our PWD ... */
sh_subfork(); /* ...we have to fork, as we cannot fchdir back to it. */ sh_subfork(); /* ...we have to fork, as we cannot fchdir back to it. */
#else
if(!shp->subshare)
{
if(sp->pwd && access(sp->pwd,X_OK)<0)
{
free(sp->pwd);
sp->pwd = NIL(char*);
}
if(!sp->pwd)
sh_subfork();
}
#endif /* _lib_fchdir */ #endif /* _lib_fchdir */
sh_offstate(SH_INTERACTIVE); sh_offstate(SH_INTERACTIVE);
sh_exec(t,flags); sh_exec(t,flags);
@ -842,32 +848,21 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
free((void*)savsig); free((void*)savsig);
} }
shp->options = sp->options; shp->options = sp->options;
if(shp->pwd != sp->pwd && (!shp->pwd || !sp->pwd || strcmp(sp->pwd,shp->pwd)))
{
/* restore the present working directory */ /* restore the present working directory */
Namval_t *pwdnod = sh_scoped(shp,PWDNOD);
#if _lib_fchdir #if _lib_fchdir
if(fchdir(sp->pwdfd) < 0) if(sp->pwdfd > 0 && fchdir(sp->pwdfd) < 0)
#else #else
if(!sp->pwd || chdir(sp->pwd) < 0) if(sp->pwd && strcmp(sp->pwd,shp->pwd) && chdir(sp->pwd) < 0)
#endif /* _lib_fchdir */ #endif /* _lib_fchdir */
{ {
saveerrno = errno; saveerrno = errno;
fatalerror = 2; fatalerror = 2;
} }
shp->pwd=sp->pwd; else if(sp->pwd && strcmp(sp->pwd,shp->pwd))
path_newdir(shp,shp->pathlist); path_newdir(shp,shp->pathlist);
if(nv_isattr(pwdnod,NV_NOFREE)) if(shp->pwd)
pwdnod->nvalue.cp = (const char*)sp->pwd; free((void*)shp->pwd);
}
else if(sp->shpwd != shp->pwd)
{
shp->pwd = sp->pwd; shp->pwd = sp->pwd;
if(PWDNOD->nvalue.cp==sp->shpwd)
PWDNOD->nvalue.cp = sp->pwd;
}
else
free((void*)sp->pwd);
#if _lib_fchdir #if _lib_fchdir
if(sp->pwdclose) if(sp->pwdclose)
close(sp->pwdfd); close(sp->pwdfd);
@ -940,6 +935,8 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
UNREACHABLE(); UNREACHABLE();
case 2: case 2:
/* reinit PWD as it will be wrong */ /* reinit PWD as it will be wrong */
if(shp->pwd)
free((void*)shp->pwd);
shp->pwd = NIL(const char*); shp->pwd = NIL(const char*);
path_pwd(shp,0); path_pwd(shp,0);
errno = saveerrno; errno = saveerrno;