Skip to content

Commit

Permalink
Fix POSIX mode IFS behaviour if xtrace on (re: 9e2a8c6, 8f14514)
Browse files Browse the repository at this point in the history
Reproducer script:

  IFS=$'\t\t' val=$'\tone\t\ttwo\tthree\t'
  set -o posix; set $val; print -n $#
  set +o posix; set $val; print -n $#
  set -o posix; set $val; print -n $#
  set +o posix; set $val; print $#

The output is normally correct but is wrong when xtrace is on:

  $ arch/darwin.arm64-64/bin/ksh test6.sh
  3535
  $ arch/darwin.arm64-64/bin/ksh -x test6.sh 2>/dev/null
  5353

Analysis: In sh_argopts() in args.c, whenever we're going to change
the posix (SH_POSIX) option, sh_invalidate_ifs() is called to
trigger a regeneration of the IFS state table (in get_ifs(),
init.c); this is necessary to enable or disable the effect of the
doubled-up whitespace character. The 'set' command is then traced
with a sh_trace() call before the new options take effect. The
trace involves expanding PS4, which involes reading IFS, which
causes get_ifs() to regenerate the IFS state table before the posix
option change takes effect.

src/cmd/ksh93/sh/args.c: sh_argopts():
- Instead of calling sh_invalidate_ifs() immediately, remember the
  need to call it in a flag variable.
- Use that flag to delay the actual sh_invalidate_ifs() call until
  after sh_trace() has been called.

Re-reading the sh_invalidate_ifs() function caused me to realise
that this function has a bug, too. For np pointing to IFS, it
assumes that the IFS discipline IFS_disc is pointed to by
np->nvfun. This is not correct if another discipline is added to
it, e.g. by defining an IFS.get() shell discipline function. Doing
that causes an invalid read in sh_invalidate_ifs() and incorrect
behaviour.

src/cmd/ksh93/sh/init.c: sh_invalidate_ifs():
- Move the function to after the IFS_disc declaration.
- Use nv_hasdisc() (nvdisc.c) to find the pointer to IFS_disc
  starting from np->nvfun, which is a linked list.

src/cmd/ksh93/tests/posix.sh:
- Set a dummy IFS.get() to test the above fix.
  • Loading branch information
McDutchie committed Apr 5, 2024
1 parent a74ded6 commit 36ebc5d
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 16 deletions.
7 changes: 7 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@ This documents significant changes in the 1.0 branch of ksh 93u+m.
For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0
Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.

2024-03-05:

- Fixed a corner case bug causing incorrect field splitting behaviour of a
repeated whitespace character in the IFS variable. The behaviour was
incorrect when the posix option was changed while xtrace was on, and when a
shell discipline function was defined for IFS. Bug introduced on 2022-03-05.

2024-03-03:

- Fixed incorrect behaviour of {n1..n2..incr} range brace expansions, which
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */
#define SH_RELEASE_SVER "1.0.9-beta" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2024-04-03" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2024-04-05" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_CPYR "(c) 2020-2024 Contributors to ksh " SH_RELEASE_FORK

/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
Expand Down
10 changes: 7 additions & 3 deletions src/cmd/ksh93/sh/args.c
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ int sh_argopts(int argc,char *argv[])
#endif
Shopt_t newflags;
int defaultflag=0, setflag=0, action=0, trace=(int)sh_isoption(SH_XTRACE);
int invalidate_ifs = 0;
Namval_t *np = NULL;
const char *cp;
int verbose, f;
Expand Down Expand Up @@ -175,7 +176,7 @@ int sh_argopts(int argc,char *argv[])
{
off_option(&newflags,o);
if(o==SH_POSIX)
sh_invalidate_ifs();
invalidate_ifs = 1;
}
}
}
Expand Down Expand Up @@ -251,7 +252,7 @@ int sh_argopts(int argc,char *argv[])
off_option(&newflags,SH_BRACEEXPAND);
#endif
on_option(&newflags,SH_LETOCTAL);
sh_invalidate_ifs();
invalidate_ifs = 1;
}
on_option(&newflags,o);
off_option(&sh.offoptions,o);
Expand All @@ -269,7 +270,7 @@ int sh_argopts(int argc,char *argv[])
on_option(&newflags,SH_BRACEEXPAND);
#endif
off_option(&newflags,SH_LETOCTAL);
sh_invalidate_ifs();
invalidate_ifs = 1;
}
if(o==SH_XTRACE)
trace = 0;
Expand All @@ -294,6 +295,9 @@ int sh_argopts(int argc,char *argv[])
}
if(trace)
sh_trace(argv,1);
/* Invalidating the IFS state table must be done after sh_trace, because xtrace reads IFS */
if(invalidate_ifs)
sh_invalidate_ifs();
argc -= opt_info.index;
argv += opt_info.index;
if(action==PRINT)
Expand Down
25 changes: 13 additions & 12 deletions src/cmd/ksh93/sh/init.c
Original file line number Diff line number Diff line change
Expand Up @@ -554,18 +554,6 @@ static void put_ifs(Namval_t* np,const char *val,int flags,Namfun_t *fp)
}
}

/* Invalidate IFS state table */
void sh_invalidate_ifs(void)
{
Namval_t *np = sh_scoped(IFSNOD);
if(np)
{
struct ifs *ip = (struct ifs*)np->nvfun;
if(ip)
ip->ifsnp = 0;
}
}

/*
* This is the lookup function for IFS
* It keeps the sh.ifstable up to date
Expand Down Expand Up @@ -1015,6 +1003,19 @@ static const Namdisc_t SH_VERSION_disc = { 0, 0, get_version, nget_version };


static const Namdisc_t IFS_disc = { sizeof(struct ifs), put_ifs, get_ifs };

/* Invalidate IFS state table */
void sh_invalidate_ifs(void)
{
Namval_t *np = sh_scoped(IFSNOD);
if(np)
{
struct ifs *ip = (struct ifs*)nv_hasdisc(np, &IFS_disc);
if(ip)
ip->ifsnp = 0;
}
}

const Namdisc_t RESTRICTED_disc = { sizeof(Namfun_t), put_restricted };
static const Namdisc_t CDPATH_disc = { sizeof(Namfun_t), put_cdpath };
#if SHOPT_VSH || SHOPT_ESH
Expand Down
2 changes: 2 additions & 0 deletions src/cmd/ksh93/tests/posix.sh
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ got=$("$SHELL" -c 'typeset -p testint')
set --posix

# disables the special handling of repeated isspace class characters in the IFS variable;
IFS.get() { :; } # tests if sh_invalidate_ifs() in init.c correctly gets IFS_disc
IFS=$'x\t\ty' val=$'\tun\t\tduo\ttres\t'
got=$(set $val; echo "$#")
exp=3
Expand All @@ -131,6 +132,7 @@ got=$(set --default; set $val; echo "$#")
[[ $got == "$exp" ]] || err_exit "repeated IFS whitespace char (default): incorrect number of fields" \
"(expected $(printf %q "$exp"), got $(printf %q "$got"))"
IFS=$' \t\n' # default
unset -f IFS.get

# causes file descriptors > 2 to be left open when invoking another program;
exp='ok'
Expand Down

0 comments on commit 36ebc5d

Please sign in to comment.