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

Fix use of strdup on a NULL pointer #63

Merged
merged 1 commit into from
Jul 6, 2020

Conversation

JohnoKing
Copy link

@JohnoKing JohnoKing commented Jul 6, 2020

EDIT: The pull request message has been updated with a correction for using strdup on a null pointer.

The following set of commands can cause a memory fault when auditing is enabled, although it can also cause ksh to write '(null)' to the auditing file in place of a tty name:

$ [ -e /etc/ksh_audit ] || echo "/tmp/ksh_auditfile;$(id -u)" | sudo tee /etc/ksh_audit;  # Enable auditing
$ v=$(ksh 2>/dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')
$ cat /tmp/ksh_auditfile
1000;1594036971;(null); getopts a:bc: opt --man
1000;1594036971;(null); print $?

The crash can also occur in this regression test, although intermittently:

export ENV=/./dev/null
v=$($SHELL 2> /dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')
[[ $v == 2* ]] || err_exit 'getopts --man does not exit 2 for interactive shells'

$ bin/shtests builtins
#### Regression-testing /home/johno/Documents/GitRepos/Clones/ksh/arch/linux.i386-64/bin/ksh ####
test builtins begins at 2020-07-06+04:51:55
builtins.sh: line 652: 4640: Memory fault
	builtins.sh[653]: getopts --man does not exit 2 for interactive shells
test builtins failed at 2020-07-06+04:52:03 with exit code 1 [ 167 tests 1 error ]
test builtins(C.UTF-8) begins at 2020-07-06+04:52:03
builtins.sh: line 652: 4755: Memory fault
	builtins.sh[653]: getopts --man does not exit 2 for interactive shells
test builtins(C.UTF-8) failed at 2020-07-06+04:52:11 with exit code 1 [ 167 tests 1 error ]
test builtins(shcomp) begins at 2020-07-06+04:52:11
/tmp/ksh93.shtests.4496.23731/shcomp-builtins.ksh: line 652: 4875: Memory fault
	shcomp-builtins.ksh[653]: getopts --man does not exit 2 for interactive shells
test builtins(shcomp) failed at 2020-07-06+04:52:20 with exit code 1 [ 167 tests 1 error ]
Total errors: 3
CPU time       user:      system:
main:       0m00.00s     0m00.00s
tests:      0m00.23s     0m00.06s

This happens because strdup is used unconditionally on the pointer returned by ttyname, which can be NULL if stderr is closed. The string is then set to NULL, which causes the crash as ksh expects a valid pointer to be returned:

fcntl(fd,F_SETFD,FD_CLOEXEC);
hp->tty = strdup(ttyname(2));
hp->auditfp = sfnew((Sfio_t*)0,NULL,-1,fd,SF_WRITE);

extern char*
strdup(register const char* s)
{
register char* t;
register int n;
return (s && (t = oldof(0, char, n = strlen(s) + 1, 0))) ? (char*)memcpy(t, s, n) : (char*)0;
}

From https://pubs.opengroup.org/onlinepubs/9699919799/functions/ttyname.html#tag_16_628_04:

Upon successful completion, ttyname() shall return a pointer to a string. Otherwise, a null pointer shall be returned and errno set to indicate the error.

This bug was originally reported in att#1028. The fix (from att#1062) is to have strdup duplicate 'notty' if ttyname returns a null pointer, which results in the auditing file now recording 'notty' instead of '(null)':

$ v=$(ksh  2> /dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')  # This may cause a segfault
$ cat /tmp/ksh_auditfile
1000;1594036922;notty; getopts a:bc: opt --man
1000;1594036922;notty; print $?

The following set of commands can rarely cause a memory fault
when auditing is enabled, although most of the time it will
simply cause ksh to write '(null)' to the auditing file in place
of a tty name:

$ [ -e /etc/ksh_audit ] || echo "/tmp/ksh_auditfile;$(id -u)" | sudo tee /etc/ksh_audit;
$ v=$(ksh  2> /dev/null +o rc -ic $'getopts a:bc: opt --man\nprint $?')
$ cat /tmp/ksh_auditfile
1000;1593599493;(null); getopts a:bc: opt --man

This happens because strdup is used unconditionally on the pointer
returned by 'ttyname', which can be NULL if stderr is closed. This
then causes 'hp->tty' to be set to null, as strdup returns NULL.
See att#1028

src/cmd/ksh93/edit/history.c:
- Make strdup duplicate 'notty' instead of NULL to prevent
  crashes.
@McDutchie McDutchie merged commit 9a9da2c into ksh93:master Jul 6, 2020
@JohnoKing JohnoKing deleted the fix-strdup-crash branch July 10, 2020 19:00
JohnoKing referenced this pull request Mar 20, 2021
Red Hat erratum: https://bugzilla.redhat.com/1221766
"Previously, the gcc utility optimized out a non-NULL test in the
ksh implementation of the strdup() function. This caused an
unexpected termination when ksh was executed in a clean chroot
environment. With this update, ksh compilation parameters have been
updated to prevent optimizing out a non-NULL test, and ksh no
longer crashes in clean chroot environments."

The optimizer bug occurs in that function's single-line body:

  return (s && (t = oldof(0, char, n = strlen(s) + 1, 0))) ? (char*)memcpy(t, s, n) : (char*)0;

So it must be the test for non-NULL 's' that fails. And 's' is
declared in the function definition, as follows:

  extern char*
  strdup(register const char* s)

So that makes me wonder if we can work around the bug by simply
removing the 'const' (and the 'register' while we're at it).
However, I have no easy way to verify that at the moment. The Red
Hat patch instead tells gcc to disable optimization for this
function using a #pragma directive.

I have no idea if that gcc optimiser bug has been fixed in the
meantime, but experience from c258a04 has shown that we cannot
trust that it has been fixed (that other optimizer bug is at least
a decade old and still not fixed). So, in it goes, until someone
shows evidence that we no longer need it.

Original patch:
https://src.fedoraproject.org/rpms/ksh/blob/642af4d6/f/ksh-20120801-badgcc.patch

src/lib/libast/string/strdup.c:
- Tell GCC to disable all optimisations for strdup().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants