Skip to content

Commit

Permalink
Fix argv rewrite on invoking hashbangless script (rhbz#1047506)
Browse files Browse the repository at this point in the history
The fixargs() function is invoked when ksh needs to run a script
without a #!/hashbang/path. Instead of letting the kernel invoke a
shell, ksh exfile()s the script itself from sh_main(). In the
forked child, it calls fixargs() to set the argument list in the
environment to the args of the new script, so that 'ps' and
/proc/PID/cmdline show the expected output.

But fixargs() is broken because, on systems other than HP-UX (on
which ksh uses pstat(2)), ksh simply inserts a terminating zero.
The arguments list is not a zero-terminated C string. Unix systems
expect the entire arguments buffer to be zeroed out, otherwise 'ps'
and /proc/*/cmdline will have fragments of previous command lines
in the output.

The Red Hat patch for this bug is:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-argvfix.patch

However, that fix is incomplete because 'command_len' was also
hardcoded to be limited to 64 characters (!), which still gave
invalid 'ps' output if the erased command line was longer.

src/cmd/ksh93/sh/main.c: fixargs():

- Remove CMD_LENGTH macro which was defined as 64.

- Remove code that limited the erasure of the arguments buffer to
  CMD_LENGTH characters. That code also had quite a dodgy strdup()
  call -- it copies arguments to the heap, but they are never freed
  (or even used), so it's a memory leak. Also, none of this is
  ever done if the length is calculated using pstat(2) on HP-UX,
  which is a clear indication that it's unnecessary.
  (I think this code block must have been some experiment they
  forgot to remove. One reason why I think so is that a 64 byte
  arguments limit never made sense, even in the 1980s when they
  wrote ksh on 80-column CRT displays. Another indication of this
  is that fixing it didn't require adding anything; the code to do
  the right thing was already there, it was just being overridden.)

- Zero out the full arguments length as in the Red Hat patch.

src/cmd/ksh93/tests/basic.sh:

- Add test. It's sort of involved because 'ps' is one of the least
  portable commands in practice, in spite of standardisation.
  • Loading branch information
McDutchie committed Sep 25, 2020
1 parent 651bbd5 commit cefe087
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 17 deletions.
7 changes: 7 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@ For full details, see the git log at: https://github.com/ksh93/ksh

Any uppercase BUG_* names are modernish shell bug IDs.

2020-09-25:

- When ksh invoked a shell script that does not have a leading
#!/hashbang/path, 'ps' and /proc/<PID>/cmdline showed corrupted output if
the new script's command line was shorter than that of the invoking script.
This has been fixed by wiping the arguments buffer correctly.

2020-09-24:

- An omission made it impossible to turn off brace expansion within command
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 @@ -17,4 +17,4 @@
* David Korn <dgk@research.att.com> *
* *
***********************************************************************/
#define SH_RELEASE "93u+m 2020-09-24"
#define SH_RELEASE "93u+m 2020-09-25"
26 changes: 10 additions & 16 deletions src/cmd/ksh93/sh/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,6 @@
# include <nc.h>
#endif /* _hdr_nc */

#define CMD_LENGTH 64

/* These routines are referenced by this module */
static void exfile(Shell_t*, Sfio_t*,int);
static void chkmail(Shell_t *shp, char*);
Expand Down Expand Up @@ -340,6 +338,7 @@ int sh_main(int ac, char *av[], Shinit_f userinit)
}
else
{
/* beenhere > 0: We're in a forked child, about to execute a script without a hashbang path. */
fdin = shp->infd;
fixargs(shp->st.dolv,1);
}
Expand Down Expand Up @@ -697,6 +696,13 @@ static void chkmail(Shell_t *shp, char *files)
#if defined(_lib_fork) && !defined(_NEXT_SOURCE)
/*
* fix up command line for ps command
*
* This function is invoked when ksh needs to run a script without a
* #!/hashbang/path. Instead of letting the kernel invoke a shell, ksh
* exfile()s the script itself from sh_main(). In the forked child, it calls
* fixargs() to set the argument list in the environment to the args of the
* new script, so that 'ps' and /proc/PID/cmdline show the expected output.
*
* mode is 0 for initialization
*/
static void fixargs(char **argv, int mode)
Expand Down Expand Up @@ -727,19 +733,6 @@ static void fixargs(char **argv, int mode)
buff = argv[0];
while(cp = *argv++)
command_len += strlen(cp)+1;
if(environ && *environ==buff+command_len)
{
for(argv=environ; cp = *argv; cp++)
{
if(command_len > CMD_LENGTH)
{
command_len = CMD_LENGTH;
break;
}
*argv++ = strdup(cp);
command_len += strlen(cp)+1;
}
}
command_len -= 1;
return;
}
Expand All @@ -754,7 +747,8 @@ static void fixargs(char **argv, int mode)
offset += size;
buff[offset++] = ' ';
}
buff[offset-1] = 0;
offset--;
memset(&buff[offset], 0, command_len - offset);
# ifdef PSTAT
un.pst_command = stakptr(0);
pstat(PSTAT_SETCMD,un,0,0,0);
Expand Down
24 changes: 24 additions & 0 deletions src/cmd/ksh93/tests/basic.sh
Original file line number Diff line number Diff line change
Expand Up @@ -692,5 +692,29 @@ actual=$(exptest foo)
[[ $actual == "$expect" ]] || err_exit 'Corruption of multibyte char following expansion of single-char name' \
"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
# ======
# ksh didn't rewrite argv correctly (rhbz#1047506)
# When running a script without a #! hashbang path, ksh attempts to replace argv with the arguments
# of the script. However, fixargs() didn't wipe out the rest of previous arguments after the last
# \0. This caused an erroneous record in /proc/<PID>/cmdline and the output of the ps command.
if actual=$(UNIX95=1 ps -o args= -p "$$" 2>&1) # UNIX95=1 makes this work on HP-UX
# Some 'ps' implementations add leading and/or trailing whitespace. Remove.
while [[ $actual == [[:space:]]* ]]; do actual=${actual#?}; done
while [[ $actual == *[[:space:]] ]]; do actual=${actual%?}; done
[[ $actual == "$SHELL $0" ]] # this is how shtests invokes this script
then expect='./atest 1 2'
echo 'sleep 10; exit 0' >atest
chmod 755 atest
./atest 1 2 &
actual=$(UNIX95=1 ps -o args= -p "$!")
kill "$!"
while [[ $actual == [[:space:]]* ]]; do actual=${actual#?}; done
while [[ $actual == *[[:space:]] ]]; do actual=${actual%?}; done
[[ $actual == "$expect" ]] || err_exit "ksh didn't rewrite argv correctly" \
"(expected $(printf %q "$expect"), got $(printf %q "$actual"))"
else err_exit "warning: skipping argv rewrite test due to noncompliant 'ps' utility (got $(printf %q "$actual"))"
let Errors--
fi
# ======
exit $((Errors<125?Errors:125))

0 comments on commit cefe087

Please sign in to comment.