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

clang-15: May produce invalid code when -O1 (or higher) is used with -fzero-call-used-regs=all #57692

Closed
nvinson opened this issue Sep 12, 2022 · 14 comments · Fixed by llvm/llvm-project-release-prs#159

Comments

@nvinson
Copy link

nvinson commented Sep 12, 2022

This report is copied from https://bugs.gentoo.org/869839

Description:

When building binaries using -O1 (or higher) and the -fzero-call-used-regs=all the resultant object files may create a broken binary when linked to.

Reproducible:

Always

Steps to Reproduce:

  1. Create a file named get_progname.c
  2. Paste the following into get_progname.c:
     #include <string.h>
     #include <stdio.h>
     #include <stdlib.h>

    char *ssh_get_progname(char *argv0)
    {
        char *p, *q;
        extern char *__progname;

        p = __progname;
        if ((q = strdup(p)) == NULL) {
            perror("strdup");
            exit(1);
        }
        return q;
    }
  1. Execute clang -O1 -ggdb -fzero-call-used-regs=all -c get_progname.c
  2. Create a file named test.c
  3. Paste the following into test.c:
   #include <stdio.h>
   #include <stdlib.h>
   #include <string.h>
   #include <syslog.h>

   extern char *__progname;

   char *ssh_get_progname(char *);

   int main(int argc, char **argv)
   {
       __progname = ssh_get_progname(argv[0]);
       openlog(argv[0], 1, LOG_USER);

       return 0;
   }
  1. Execute clang -O1 -ggdb -fzero-call-used-regs=all -c test.c
  2. Execute clang -o test test.o get_progname.o
  3. run ./test

Actual Results:

./test segfaults due to argv[0] being incorrectly set to NULL during execution

Expected Results:

./test should run and exit successfully.

Additional Info:

Portage 3.0.35 (python 3.10.7-final-0, default/linux/amd64/17.1/desktop, gcc-12.2.0, glibc-2.35-r8, 5.18.14-gentoo x86_64)
=================================================================
System uname: [Linux-5.18.14-gentoo-x86_64-Intel-R-_Core-TM-_i7-4771_CPU_@_3.50GHz-with-glibc2.35](mailto:Linux-5.18.14-gentoo-x86_64-Intel-R-_Core-TM-_i7-4771_CPU_@_3.50GHz-with-glibc2.35)
KiB Mem:    32557860 total,   7752032 free
KiB Swap:    2097148 total,   2045904 free
Timestamp of repository gentoo: Mon, 12 Sep 2022 06:57:44 +0000
Head commit of repository gentoo: 3661d51653661eea567088e6e80385d8a14432c4

Timestamp of repository brother-overlay: Sun, 04 Sep 2022 19:46:50 +0000
Head commit of repository brother-overlay: 2c208f0df5aa5f9f967c361f0e3ad514e50422d0

Head commit of repository magpie: 10f020019b514442260050d216608c67910099cb

sh bash 5.1_p16-r2
ld GNU ld (Gentoo 2.39 p4) 2.39.0
app-misc/pax-utils:        1.3.5::gentoo
app-shells/bash:           5.1_p16-r2::gentoo
dev-java/java-config:      2.3.1::gentoo
dev-lang/perl:             5.36.0::gentoo
dev-lang/python:           3.10.7::gentoo, 3.11.0_rc1_p2::gentoo
dev-lang/rust:             1.63.0::magpie
dev-util/cmake:            3.24.1::gentoo
dev-util/meson:            0.63.2::gentoo
sys-apps/baselayout:       2.8-r2::gentoo
sys-apps/openrc:           0.45.2::gentoo
sys-apps/sandbox:          2.29::gentoo
sys-devel/autoconf:        2.13-r2::gentoo, 2.71-r2::gentoo
sys-devel/automake:        1.16.5::gentoo
sys-devel/binutils:        2.39-r2::gentoo
sys-devel/binutils-config: 5.4.1::gentoo
sys-devel/clang:           14.0.6-r1::gentoo, 15.0.0::gentoo
sys-devel/gcc:             12.2.0::gentoo
sys-devel/gcc-config:      2.5-r1::gentoo
sys-devel/libtool:         2.4.7::gentoo
sys-devel/lld:             15.0.0::gentoo
sys-devel/llvm:            14.0.6-r2::gentoo, 15.0.0::gentoo
sys-devel/make:            4.3::gentoo
sys-kernel/linux-headers:  5.19::gentoo (virtual/os-headers)
sys-libs/glibc:            2.35-r8::gentoo
Repositories:

gentoo
    location: /var/db/repos/gentoo
    sync-type: git
    sync-uri: https://github.com/gentoo-mirror/gentoo.git
    priority: -1000
    sync-git-verify-commit-signature: true

brother-overlay
    location: /var/db/repos/brother-overlay
    sync-type: git
    sync-uri: https://github.com/gentoo-mirror/brother-overlay.git
    masters: gentoo

magpie
    location: /var/db/repos/magpie
    sync-type: git
    sync-uri: https://github.com/nvinson/magpie.git
    masters: gentoo

ACCEPT_KEYWORDS="amd64 ~amd64"
ACCEPT_LICENSE="* -@EULA"
AR="llvm-ar"
CBUILD="x86_64-pc-linux-gnu"
CC="clang"
CFLAGS="-march=native -O2 -pipe -D_FORTIFY_SOURCE=3 -fstack-protector-strong -flto=thin"
CHOST="x86_64-pc-linux-gnu"
CONFIG_PROTECT="/etc /opt/brother/scanner/brscan4/brsanenetdevice4.cfg /usr/lib64/libreoffice/program/sofficerc /usr/share/gnupg/qualified.txt"
CONFIG_PROTECT_MASK="/etc/ca-certificates.conf /etc/dconf /etc/env.d /etc/fonts/fonts.conf /etc/gconf /etc/gentoo-release /etc/revdep-rebuild /etc/sandbox.d /etc/terminfo
 /etc/texmf/language.dat.d /etc/texmf/language.def.d /etc/texmf/updmap.d /etc/texmf/web2c"
CXX="clang++"
CXXFLAGS="-march=native -O2 -pipe -D_FORTIFY_SOURCE=3 -fstack-protector-strong -flto=thin"
DISTDIR="/var/cache/portage/distfiles"
EMERGE_DEFAULT_OPTS="--quiet-build"
ENV_UNSET="CARGO_HOME DBUS_SESSION_BUS_ADDRESS DISPLAY GOBIN GOPATH PERL5LIB PERL5OPT PERLPREFIX PERL_CORE PERL_MB_OPT PERL_MM_OPT XAUTHORITY XDG_CACHE_HOME XDG_CONFIG_HO
ME XDG_DATA_HOME XDG_RUNTIME_DIR"
FCFLAGS="-march=native -O2 -pipe"
FEATURES="assume-digests binpkg-docompress binpkg-dostrip binpkg-logs binpkg-multi-instance buildpkg-live config-protect-if-modified distlocks ebuild-locks fixlafiles ipc
-sandbox merge-sync multilib-strict network-sandbox news parallel-fetch pid-sandbox preserve-libs protect-owned qa-unresolved-soname-deps sandbox sfperms strict unknown-f
eatures-warn unmerge-logs unmerge-orphans userfetch userpriv usersandbox usersync xattr"
FFLAGS="-march=native -O2 -pipe"
GENTOO_MIRRORS="http://distfiles.gentoo.org/"
LANG="en_US.utf8"
LDFLAGS="-Wl,-O1 -Wl,--as-needed -fuse-ld=lld -rtlib=compiler-rt -unwindlib=libunwind"
MAKEOPTS="-j8"
NM="llvm-nm"
PKGDIR="/var/cache/binpkgs"
PORTAGE_CONFIGROOT="/"
PORTAGE_RSYNC_OPTS="--recursive --links --safe-links --perms --times --omit-dir-times --compress --force --whole-file --delete --stats --human-readable --timeout=180 --ex
clude=/distfiles --exclude=/local --exclude=/packages --exclude=/.git"
PORTAGE_TMPDIR="/var/tmp"
RANLIB="llvm-ranlib"
SHELL="/bin/zsh"
USE="X a52 aac acl acpi alsa amd64 branding bzip2 cairo cdda cdr clang cleartype cli corefonts crypt cups dbus dri dts dvd dvdr elogind encode exif flac fortran gdbm gif 
glamor gpm gtk gui iconv icu ipv6 jpeg lcms libglvnd libnotify libtirpc mad mng mp3 mp4 mpeg multilib ncurses nls nptl ogg opengl openmp pam pango pcre pdf png policykit 
ppds qt5 readline sdl seccomp spell split-usr ssl startup-notification svg theora tiff truetype udev udisks unicode upower usb vaapi vorbis vpx wxwidgets x264 xattr xcb x
ml xv xvid zlib" ABI_X86="64" ADA_TARGET="gnat_2020" APACHE2_MODULES="authn_core authz_core socache_shmcb unixd actions alias auth_basic authn_alias authn_anon authn_dbm 
authn_default authn_file authz_dbm authz_default authz_groupfile authz_host authz_owner authz_user autoindex cache cgi cgid dav dav_fs dav_lock deflate dir disk_cache env
 expires ext_filter file_cache filter headers include info log_config logio mem_cache mime mime_magic negotiation rewrite setenvif speling status unique_id userdir usertr
ack vhost_alias" CALLIGRA_FEATURES="karbon sheets words" COLLECTD_PLUGINS="df interface irq load memory rrdtool swap syslog" CPU_FLAGS_X86="aes avx avx2 fma3 mmx mmxext p
opcnt sse sse2 sse3 sse4 sse4_1 sse4_2 ssse3" ELIBC="glibc" GPSD_PROTOCOLS="ashtech aivdm earthmate evermore fv18 garmin garmintxt gpsclock greis isync itrax mtk3301 nmea
 ntrip navcom oceanserver oldstyle oncore rtcm104v2 rtcm104v3 sirf skytraq superstar2 timing tsip tripmate tnt ublox ubx" INPUT_DEVICES="libinput" KERNEL="linux" L10N="en
-US en" LCD_DEVICES="bayrad cfontz cfontz633 glk hd44780 lb216 lcdm001 mtxorb ncurses text" LIBREOFFICE_EXTENSIONS="presenter-console presenter-minimizer" LUA_SINGLE_TARG
ET="lua5-1" LUA_TARGETS="lua5-1" OFFICE_IMPLEMENTATION="libreoffice" PHP_TARGETS="php7-4 php8-0" POSTGRES_TARGETS="postgres12 postgres13" PYTHON_SINGLE_TARGET="python3_10
" PYTHON_TARGETS="python3_10" RUBY_TARGETS="ruby27" USERLAND="GNU" VIDEO_CARDS="intel i965" XTABLES_ADDONS="quota2 psd pknock lscan length2 ipv4options ipset ipp2p iface 
geoip fuzzy condition tee tarpit sysrq proto steal rawnat logmark ipmark dhcpmac delude chaos account"
Unset:  ADDR2LINE, ARFLAGS, AS, ASFLAGS, CCLD, CONFIG_SHELL, CPP, CPPFLAGS, CTARGET, CXXFILT, ELFEDIT, EXTRA_ECONF, F77FLAGS, FC, GCOV, GPROF, INSTALL_MASK, LC_ALL, LD, L
EX, LFLAGS, LIBTOOL, LINGUAS, MAKE, MAKEFLAGS, OBJCOPY, OBJDUMP, PORTAGE_BINHOST, PORTAGE_BUNZIP2_COMMAND, PORTAGE_COMPRESS, PORTAGE_COMPRESS_FLAGS, PORTAGE_RSYNC_EXTRA_O
PTS, READELF, RUSTFLAGS, SIZE, STRINGS, STRIP, YACC, YFLAGS
@nvinson
Copy link
Author

nvinson commented Sep 12, 2022

This was found when building openssh-9.0 (gentoo version: 9.0_p1-r3) with clang-15. After building openssh, utilities such as ssh-keygen started segfaulting as described above.

Additionally, __progname is initialized by glibc during program startup. It is a GNU extension.

@efriedma-quic
Copy link
Collaborator

CC @isanbard

gentoo-bot pushed a commit to gentoo/gentoo that referenced this issue Sep 12, 2022
@mgorny mgorny added this to the LLVM 15.0.1 Release milestone Sep 14, 2022
@tru tru moved this to Needs Triage in LLVM Release Status Sep 14, 2022
@tru
Copy link
Collaborator

tru commented Sep 15, 2022

@efriedma-quic @isanbard Is this something that we should fix before 15.0.1? That window is closing quickly.

@thesamesam
Copy link
Member

thesamesam commented Sep 15, 2022

@tru By the way, OpenSSH uses this by default when the argument is available, unless --without-hardening is passed. So it's not just user-inflicted pain (even if that should work too).

@nikic
Copy link
Contributor

nikic commented Sep 15, 2022

It looks like this flag was only added in LLVM 15 via https://reviews.llvm.org/D110869.

@nikic
Copy link
Contributor

nikic commented Sep 15, 2022

So this is how the used zeroing sequence looks like:

	fldz
	fldz
	fldz
	fldz
	fldz
	fldz
	fldz
	fldz
	fstp	%st(0)
	fstp	%st(0)
	fstp	%st(0)
	fstp	%st(0)
	fstp	%st(0)
	fstp	%st(0)
	fstp	%st(0)
	fstp	%st(0)
	xorl	%ebp, %ebp
	xorl	%ebx, %ebx
	xorl	%ecx, %ecx
	xorl	%edi, %edi
	xorl	%edx, %edx
	xorl	%esi, %esi
	xorl	%r8d, %r8d
	xorl	%r9d, %r9d
	xorl	%r10d, %r10d
	xorl	%r11d, %r11d
	xorl	%r12d, %r12d
	xorl	%r13d, %r13d
	xorl	%r14d, %r14d
	xorl	%r15d, %r15d
	xorps	%xmm0, %xmm0
	xorps	%xmm1, %xmm1
	xorps	%xmm2, %xmm2
	xorps	%xmm3, %xmm3
	xorps	%xmm4, %xmm4
	xorps	%xmm5, %xmm5
	xorps	%xmm6, %xmm6
	xorps	%xmm7, %xmm7
	xorps	%xmm8, %xmm8
	xorps	%xmm9, %xmm9
	xorps	%xmm10, %xmm10
	xorps	%xmm11, %xmm11
	xorps	%xmm12, %xmm12
	xorps	%xmm13, %xmm13
	xorps	%xmm14, %xmm14
	xorps	%xmm15, %xmm15

This looks very wrong to me, because it includes a whole bunch of callee-saved registers. In this case %ebx is the problematic one, but %ebp and %r12-15 are also callee-saved and should not be clobbered.

@nikic
Copy link
Contributor

nikic commented Sep 15, 2022

I believe the problem is that

// Don't clear registers that are reset before exiting.
for (const CalleeSavedInfo &CSI : MF.getFrameInfo().getCalleeSavedInfo())
for (MCRegister Reg : TRI.sub_and_superregs_inclusive(CSI.getReg()))
RegsToZero.reset(Reg);
only excludes callee save registers save inside this function, while it must exclude all callee save registers.

@nikic nikic self-assigned this Sep 15, 2022
@nikic nikic moved this from Needs Triage to Needs Fix in LLVM Release Status Sep 15, 2022
@nikic
Copy link
Contributor

nikic commented Sep 15, 2022

Candidate patch: https://reviews.llvm.org/D133946

@mgorny
Copy link
Member

mgorny commented Sep 16, 2022

@nvinson, @thesamesam, could you test whether this patch fixes OpenSSH?

@nikic nikic closed this as completed in b430980 Sep 16, 2022
@nikic nikic reopened this Sep 16, 2022
@nikic
Copy link
Contributor

nikic commented Sep 16, 2022

/cherry-pick b430980

@nikic nikic moved this from Needs Fix to Needs Pull Request in LLVM Release Status Sep 16, 2022
@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2022

/branch llvm/llvm-project-release-prs/issue57692

llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Sep 16, 2022
…PR57692)

Callee save registers must be preserved, so -fzero-call-used-regs
should not be zeroing them. The previous implementation only did
not zero callee save registers that were saved&restored inside the
function, but we need preserve all of them.

Fixes llvm/llvm-project#57692.

Differential Revision: https://reviews.llvm.org/D133946

(cherry picked from commit b430980)
@llvmbot
Copy link
Member

llvmbot commented Sep 16, 2022

/pull-request llvm/llvm-project-release-prs#159

@nikic nikic moved this from Needs Pull Request to Needs Review in LLVM Release Status Sep 16, 2022
@nickdesaulniers
Copy link
Member

cc @gwelymernans

@nvinson
Copy link
Author

nvinson commented Sep 18, 2022

@nvinson, @thesamesam, could you test whether this patch fixes OpenSSH?

The issues I originally saw with openssh is that I couldn't log in and the ssh-keygen utility would crash when run. I've rebuilt llvm with the patch and rebuilt openssh. I wasable to log in via ssh after rebuilding and the ssh-keygen utility ran correctly.

This patch solves the problems I was seeing with openssh.

tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Sep 19, 2022
…PR57692)

Callee save registers must be preserved, so -fzero-call-used-regs
should not be zeroing them. The previous implementation only did
not zero callee save registers that were saved&restored inside the
function, but we need preserve all of them.

Fixes llvm/llvm-project#57692.

Differential Revision: https://reviews.llvm.org/D133946

(cherry picked from commit b430980)
@tru tru moved this from Needs Review to Done in LLVM Release Status Sep 19, 2022
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
…PR57692)

Callee save registers must be preserved, so -fzero-call-used-regs
should not be zeroing them. The previous implementation only did
not zero callee save registers that were saved&restored inside the
function, but we need preserve all of them.

Fixes llvm/llvm-project#57692.

Differential Revision: https://reviews.llvm.org/D133946

(cherry picked from commit b4309800e9dc53a84222a6b57c8615d4a3084988)
veselypeta pushed a commit to veselypeta/cherillvm that referenced this issue May 10, 2024
…PR57692)

Callee save registers must be preserved, so -fzero-call-used-regs
should not be zeroing them. The previous implementation only did
not zero callee save registers that were saved&restored inside the
function, but we need preserve all of them.

Fixes llvm/llvm-project#57692.

Differential Revision: https://reviews.llvm.org/D133946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment