search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.94k stars 554 forks source link

Stack overflow in yyl_try #17220

Closed dur-randir closed 4 years ago

dur-randir commented 4 years ago

This is a bug report for perl from afl@dorothy, generated with the help of perlbug 1.41 running under perl 5.31.5.

[Please describe your issue here]

While fuzzing perl v5.29.10-23-g7c0d7520a3 built with afl and run under libdislocator, I found the following program

my $f=" "x4096; $f = "0$f"; eval $f

to cause a stack overflow in yyl_try. This is a regression in blead, bisect points to 760fdf2808b7158c548dcb6208804ff8d4c97b30 is the first bad commit

commit 760fdf2808b7158c548dcb6208804ff8d4c97b30
Author: Aaron Crane <arc@cpan.org>
Date:   Sun Oct 20 13:36:47 2019 +0200

    toke.c: remove the "retry" label

    We can just use recursive calls instead — which compilers will compile to a
    goto anyay!

[Please do not change anything below this line] Flags: category=core severity=medium Site configuration information for perl 5.31.5:

Configured by root at Fri Oct 18 05:50:54 MSK 2019.

Summary of my perl5 (revision 5 version 31 subversion 5) configuration: Derived from: 859b78b Platform: osname=linux osvers=4.19.0-6-amd64 archname=x86_64-linux uname='linux dorothy 4.19.0-6-amd64 #1 smp debian 4.19.67-2+deb10u1 (2019-09-20) x86_64 gnulinux ' config_args='-des -Dusedevel -Dcc=gcc -DDEBUGGING -Doptimize=-O0 -g -ggdb3' hint=recommended useposix=true d_sigaction=define useithreads=undef usemultiplicity=undef use64bitint=define use64bitall=define uselongdouble=undef usemymalloc=n default_inc_excludes_dot=define bincompat5005=undef Compiler: cc='gcc' ccflags ='-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2' optimize='-O0 -g -ggdb3' cppflags='-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='' gccversion='8.3.0' gccosandvers='' intsize=4 longsize=8 ptrsize=8 doublesize=8 byteorder=12345678 doublekind=3 d_longlong=define longlongsize=8 d_longdbl=define longdblsize=16 longdblkind=3 ivtype='long' ivsize=8 nvtype='double' nvsize=8 Off_t='off_t' lseeksize=8 alignbytes=8 prototype=define Linker and Libraries: ld='gcc' ldflags =' -fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/8/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.28.so so=so useshrplib=false libperl=libperl.a gnulibc_version='2.28' Dynamic Linking: dlsrc=dl_dlopen.xs dlext=so d_dlsymun=undef ccdlflags='-Wl,-E' cccdlflags='-fPIC' lddlflags='-shared -O0 -g -ggdb3 -L/usr/local/lib -fstack-protector-strong'

Characteristics of this binary (from libperl): Compile-time options: DEBUGGING HAS_TIMES PERLIO_LAYERS PERL_COPY_ON_WRITE PERL_DONT_CREATE_GVSV PERL_MALLOC_WRAP PERL_OP_PARENT PERL_PRESERVE_IVUV PERL_USE_DEVEL USE_64_BIT_ALL USE_64_BIT_INT USE_LARGE_FILES USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE USE_LOCALE_NUMERIC USE_LOCALE_TIME USE_PERLIO USE_PERL_ATOF Locally applied patches: uncommitted-changes Built under linux Compiled at Oct 18 2019 06:02:50 %ENV: PERLBREW_BASHRC_VERSION="0.78" PERLBREW_HOME="/home/afl/.perlbrew" PERLBREW_MANPATH="/home/afl/perlbrew/perls/perl-5.20.2/man" PERLBREW_PATH="/home/afl/perlbrew/bin:/home/afl/perlbrew/perls/perl-5.20.2/bin" PERLBREW_PERL="perl-5.20.2" PERLBREW_ROOT="/home/afl/perlbrew" PERLBREW_VERSION="0.78" @inc: lib /usr/local/lib/perl5/site_perl/5.31.5/x86_64-linux /usr/local/lib/perl5/site_perl/5.31.5 /usr/local/lib/perl5/5.31.5/x86_64-linux /usr/local/lib/perl5/5.31.5

tonycoz commented 4 years ago

Unfortunately I don't see a simple way to fix this without reverting the following changes that also recurse into yyl_try().

tonycoz commented 4 years ago

Oops, thought @arc had been pinged from here, must have been from the other one.

arc commented 4 years ago

Oh, sorry, I've only just seen this. Might be worth reverting the whole branch until I can come up with something better; the -O0 is presumably defanging the otherwise-obvious tail-call compilation, but we can't break that.

dur-randir commented 4 years ago

I can crash this even with O2, just with a higher upper bound, using the following perl:

Compiler: cc='cc' ccflags ='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2' optimize='-O2' cppflags='-fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='' gccversion='8.3.0'

dur-randir commented 4 years ago

Disassembly shows 10 recursive call sites yyl_try -> yyl_try for this O2 binary.

xsawyerx commented 4 years ago

The commits above by @arc were supposed to close this ticket. I think GitHub didn't parse his commit text correctly, so I'm closing it myself.

@dur-randir, nice catch on the recursion stack overflow detection! Please reopen this ticket if you detect it still unfixed.

jkeenan commented 4 years ago

@xsawyerx: @arc pushed those commits to a branch, did he not? (A branch which I and perhaps others are smoking.) If so, they still have to be merged into blead. So the ticket should remain open a bit.

The commits above by @arc were supposed to close this ticket. I think GitHub didn't parse his commit text correctly, so I'm closing it myself.

@dur-randir, nice catch on the recursion stack overflow detection! Please reopen this ticket if you detect it still unfixed.

@xsawyerx: @arc pushed those commits to a branch, did he not? (A branch which I and perhaps others are smoking.) If so, they still have to be merged into blead. So the ticket should remain open a bit.

Thank you very much. Jim Keenan

xsawyerx commented 4 years ago

@xsawyerx: @arc pushed those commits to a branch, did he not? (A branch which I and perhaps others are smoking.) If so, they still have to be merged into blead. So the ticket should remain open a bit.

I'm okay with that.

jkeenan commented 4 years ago

@xsawyerx: @arc pushed those commits to a branch, did he not? (A branch which I and perhaps others are smoking.) If so, they still have to be merged into blead. So the ticket should remain open a bit.

I'm okay with that.

The smoke-test reports for the branch at http://perl.develop-help.com/?b=smoke-me%2Farc%2Fsmaller-toke-bis look good, i.e., no failures attributable to @arc's revisions; only failures which are also occurring in blead and which may be smoker-specific.

+1 from me to merge into blead.

dur-randir commented 4 years ago

After 2 days, i've seen no regressions.