Skip to content

Commit

Permalink
libast: Work around gcc optimiser bug for strdup() (rhbz#1221766)
Browse files Browse the repository at this point in the history 
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().
  • Loading branch information
@McDutchie
McDutchie committed Sep 29, 2020
1 parent 1477b5f commit 7afb30e
Showing 1 changed file with 9 additions and 0 deletions.
9 changes: 9 additions & 0 deletions src/lib/libast/string/strdup.c
View file
Expand Up @@ -50,6 +50,13 @@ __STDPP__directive pragma pp:nohide strdup
#define extern __EXPORT__
#endif

/*
* Work around a null-test optimization bug in GCC.
* https://bugzilla.redhat.com/1221766
*/
#pragma GCC push_options
#pragma GCC optimize ("O0")

extern char*
strdup(register const char* s)
{
Expand All @@ -58,3 +65,5 @@ strdup(register const char* s)

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

#pragma GCC pop_options

1 comment on commit 7afb30e

@JohnoKing
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relevant Red Hat bug report for this patch is rhbz#1269088 (which does have a reproducer). I looked at the stack trace in the thread and noticed the strdup that triggers the crash calls ttyname:

// ttyname(2) fails.
(gdb) f 2
#2  0x000000000041d447 in sh_histinit (sh_context=<value optimized out>) at /usr/src/debug/ksh-20120801/src/cmd/ksh93/edit/history.c:398
398					hp->tty = strdup(ttyname(2));

This leads me to believe the crash is the same as the one I fixed in #63, although that doesn't necessarily mean Red Hat's patch is useless. GCC still over-optimizes strdup, as made plain by the fact GCC produces a -Wnonnull-compare warning when compiling strdup.c:

/home/johno/GitRepos/KornShell/ksh/src/lib/libast/string/strdup.c: In function 'strdup':
/home/johno/GitRepos/KornShell/ksh/src/lib/libast/string/strdup.c:66:84: warning: 'nonnull' argument 's' compared to NULL [-Wnonnull-compare]
   66 |  return (s && (t = oldof(0, char, n = strlen(s) + 1, 0))) ? (char*)memcpy(t, s, n) : (char*)0;
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~

Please sign in to comment.