Evals that die under the debugger may or may not leak eval data depending on how they die.

[demerphq]

Description
If eval string is used when the debugger is enabled then we may or may not leak debugging meta data in the stash depending on how the eval fails.

When $^P is set to an integer with the 0x400 bit set (and maybe other bits) the debugger mode is enabled and when code is compiled a copy of the lines that were compiled is stashed away in %:: in a slot named "_<(eval $eval_ctr)", we make attempts to arrange that this slot is deleted if the eval fails. However there are a variety of ways that the compilation phase of eval may fail. If the failure triggers a Perl_croak() then we leak the debugging data and do not delete the entry.

Two easy ways to see this are to eval "BEGIN{die}", and another is trigger 10 or more syntax errors in the eval text, which will cause the parser to trigger a croak. Any error pathway that triggers a croak may cause this outcome and there are a handful of special cases where the parser /will/ croak.

Thus "1+" will fail compile in a way that cleans up "_<(eval $eval_ctr)", but "1+;1+;1+;1+;1+;1+;1+;1+;1+;1+;" will trigger a croak and the "_<(eval $eval_ctr)" will be left behind.

t/comp/retainedlines.t attempts to test this behavior, but it only uses very simple code to eval, and it doesnt trigger a croak, which exposes the issue. I noticed this because I am trying to make the parser stop after the first syntax error it encounters, and this is easiest done by croaking, but that exposes the underlying bug in this ticket which exists with or without the new logic.

Steps to Reproduce
This should output "0 0", it outputs "0 1" instead.

perl -le'for (1,10) { $^P=0x400; eval "1+;" x $_; my $count=0; /eval/ and $count++ for keys %::; push @ret,$count } print "@ret"'
0 1

Expected behavior
We should clean up the entries consistently. The output from the above script should be "0 0".

Or I guess alternatively we shouldn't care about these "stash droppings". Although that seems like a great way to make the debugger leak memory. On the other hand, its not clear to me if we clean up these droppings if the eval is successful, so I am not sure if we should care. It feels like we should, but maybe not.

Perl configuration

$ ./perl -Ilib -V
Summary of my perl5 (revision 5 version 37 subversion 4) configuration:
  Derived from: ebc2efe6583f7cf7ea29a7f94f7b7887a75ad3a7
  Ancestor: 18fa8a6f818cbe2838cfe9b1bfa0c5d9c311930c
  Platform:
    osname=linux
    osvers=5.14.0-1049-oem
    archname=x86_64-linux-thread-multi
    uname='linux oncidium 5.14.0-1049-oem #56-ubuntu smp fri aug 12 10:23:08 utc 2022 x86_64 x86_64 x86_64 gnulinux '
    config_args='-Dusethreads -Doptimize=-g -d -Dusedevel -Dcc=ccache gcc -Dld=gcc -DDEBUGGING'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=define
    usemultiplicity=define
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='gcc'
    ccflags ='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
    optimize='-g'
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='9.4.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/x86_64-linux-gnu /usr/lib /usr/lib64
    libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.31.so
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version='2.31'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-E'
    cccdlflags='-fPIC'
    lddlflags='-shared -g -L/usr/local/lib -fstack-protector-strong'


Characteristics of this binary (from libperl): 
  Compile-time options:
    DEBUGGING
    HAS_TIMES
    MULTIPLICITY
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_HASH_FUNC_SIPHASH13
    PERL_HASH_USE_SBOX32
    PERL_MALLOC_WRAP
    PERL_OP_PARENT
    PERL_PRESERVE_IVUV
    PERL_TRACK_MEMPOOL
    PERL_USE_DEVEL
    PERL_USE_SAFE_PUTENV
    USE_64_BIT_ALL
    USE_64_BIT_INT
    USE_ITHREADS
    USE_LARGE_FILES
    USE_LOCALE
    USE_LOCALE_COLLATE
    USE_LOCALE_CTYPE
    USE_LOCALE_NUMERIC
    USE_LOCALE_TIME
    USE_PERLIO
    USE_PERL_ATOF
    USE_REENTRANT_API
    USE_THREAD_SAFE_LOCALE
  Locally applied patches:
    uncommitted-changes
    b3a0a5e1356f495dc935458a9ee5b38085e130f8
    f1886e10a63297209ad9c385b054717c4a089d8b
    ebc2efe6583f7cf7ea29a7f94f7b7887a75ad3a7
  Built under linux
  Compiled at Aug 26 2022 14:32:36
  %ENV:
    PERLBREW_CONFIGURE_FLAGS="-de -Dcc=ccache\ gcc -Dld=gcc"
    PERLBREW_HOME="/home/yorton/.perlbrew"
    PERLBREW_MANPATH="/home/yorton/perl5/perlbrew/perls/perl-5.34.1/man"
    PERLBREW_PATH="/home/yorton/perl5/perlbrew/bin:/home/yorton/perl5/perlbrew/perls/perl-5.34.1/bin"
    PERLBREW_PERL="perl-5.34.1"
    PERLBREW_ROOT="/home/yorton/perl5/perlbrew"
    PERLBREW_SHELLRC_VERSION="0.88"
    PERLBREW_VERSION="0.88"
  @INC:
    lib
    /usr/local/lib/perl5/site_perl/5.37.4/x86_64-linux-thread-multi
    /usr/local/lib/perl5/site_perl/5.37.4
    /usr/local/lib/perl5/5.37.4/x86_64-linux-thread-multi
    /usr/local/lib/perl5/5.37.4

[demerphq]

Apparently this was broken in f9bddea according to a bisect by @wolfsage

[demerphq]

I think a few folks have been doing research, so there may be some duplication of effort here. But I have focusing on the fact that CATCH_GET seems to never be true when called from pp_entereval or pp_require. Both have docatch() wrappers which clear the flag. However if I change the clause in doeval_compile():

-    yystatus = (!in_require && CATCH_GET) ? S_try_yyparse(aTHX_ GRAMPROG) : yyparse(GRAMPROG);
+    yystatus = (!in_require || CATCH_GET)   ? S_try_yyparse(aTHX_ GRAMPROG) : yyparse(GRAMPROG);

Or if i remove the CATCH_GET entirely I end up with the following test fails:

Test Summary Report
-------------------
../cpan/IO-Compress/t/020isize.t                                   (Wstat: 139 (Signal: SEGV, dumped core) Tests: 0 Failed: 0)
  Non-zero wait status: 139
../cpan/IO-Compress/t/105oneshot-zip-store-only.t                  (Wstat: 139 (Signal: SEGV, dumped core) Tests: 0 Failed: 0)
  Non-zero wait status: 139
../cpan/autodie/t/import-into.t                                    (Wstat: 512 (exited 2) Tests: 0 Failed: 0)
  Non-zero exit status: 2
../cpan/version/t/11_taint.t                                       (Wstat: 512 (exited 2) Tests: 0 Failed: 0)
  Non-zero exit status: 2
../dist/Storable/t/leaks.t                                         (Wstat: 512 (exited 2) Tests: 0 Failed: 0)
  Non-zero exit status: 2
Files=2720, Tests=1187179, 336 wallclock secs (227.51 usr 14.93 sys + 1840.35 cusr 72.46 csys = 2155.25 CPU)
Result: FAIL
make: *** [makefile:856: test_harness] Error 6

Both however do fix the problem with t/comp/retained_lines.pl

[demerphq]

BTW, I have double checked, and in this expression:

yystatus = (!in_require && CATCH_GET) ? S_try_yyparse(aTHX_ GRAMPROG) : yyparse(GRAMPROG);

CATCH_GET is never true in our testsuite, so we never call S_try_yyparse().

We do end up in pp_entereval with CATCH_GET true however. (Someone thought it might never be true there as well.)

[bram-perl]

Some commits I found interesting to look at:

• commit 27e9045 (for issue Tied methods break when combined with eval() of failing compile-time code #6766)
• commit d7e3f70 (for issue eval 'UNITCHECK{die}' crashes inside FETCH #11804)

Note: I find the wording of those commits a bit misleading (but that might be just me). So best to approach with caution and don't blindly trust their message (and what they claims).

My first impression was that CATCH_GET in pp_entereval was never TRUE but that impression is wrong.

There are two cases in which CATCH_GET in pp_entereval can be true:

1. Something forced CATCH_GET to be true
2. The 'JMPENV' level is not yet set (i.e. there was no earlier JMPENV_PUSH/no earlier 'catchable runloop')

CATCH_GET forced to true

Running the test suite revealed at least one case where it was forced to be true.
Example code (it's trimmed from an actual test):

    print "Start\n";
    unshift @INC, sub {
        print "Running eval inside \@INC\n";
        eval q#"foo"#;
        print "(Leaving \@INC)\n";
        0;
    };
    print "First eval\n";
    eval q#"bar"#;
    print "Require\n";
    require strict;
    print "Final eval\n";
    eval q#"baz"#;
    print "The end.\n";

Running with enabling (and extending) logging in JMPENV_BOOTSTRAP, JMPENV_PUSH, JMPENV_POP, CATCH_SET:
Manually altered the output for readability (indentation, removed irrelevant JMPENVs of S_fold_constants_eval, ...)

    $ ./miniperl -Ilib gh-20161-a.pl
    JMPENV_BOOTSTRAP at perl.c:314 (perl_construct)

    JMPENV_PUSH level=1 at perl.c:1900 (perl_parse)
            JMPENV_PUSH level=2 at perl.c:5184 (Perl_call_list)
                    JMPENV_PUSH level=3 at perl.c:3111 (Perl_call_sv)
                    JMPENV_POP level=3 at perl.c:3156 (Perl_call_sv)
            JMPENV_POP level=2 at perl.c:5232 (Perl_call_list)
    JMPENV_POP level=1 at perl.c:1945 (perl_parse)

    JMPENV_PUSH level=1 at perl.c:2696 (perl_run)
            Start
            First eval
            Require
            JUMPLEVEL set catch 0 => 1 (for 7ffc029de100) at perl.c:3098 (Perl_call_sv)
            Running eval inside @INC
                    in Entereval: CATCH_GET is TRUE!!
                    JMPENV_PUSH level=2 at pp_ctl.c:3416 (S_docatch)
                    (Leaving @INC)
                    JMPENV_POP level=2 at pp_ctl.c:3450 (S_docatch)
            JUMPLEVEL set catch 1 => 0 (for 7ffc029de100) at perl.c:3101 (Perl_call_sv)
            JMPENV_PUSH level=2 at perl.c:5184 (Perl_call_list)
                    JMPENV_PUSH level=3 at perl.c:3111 (Perl_call_sv)
                    JMPENV_POP level=3 at perl.c:3156 (Perl_call_sv)
            JMPENV_POP level=2 at perl.c:5232 (Perl_call_list)
            Final eval
            The end.
    JMPENV_POP level=1 at perl.c:2732 (perl_run)

    JMPENV_PUSH level=1 at perl.c:663 (perl_destruct)
    JMPENV_POP level=1 at perl.c:669 (perl_destruct)

Looking at Perl_call_sv: it contains (and has contained it since atleast 1999):

if (!(flags & G_EVAL)) {
    CATCH_SET(TRUE);
    CALL_BODY_SUB((OP*)&myop);
    retval = PL_stack_sp - (PL_stack_base + oldmark);
    CATCH_SET(oldcatch);
}

So that caused pp_entereval to be called with CATCH_GET TRUE.

no earlier 'catchable runloop'

I'm not sure if that is possible, from testing I've done that doesn't appear to be possible but I might be wrong.
Commit message of d7e3f70 for #11084 gave me the impression that it was possible but that doesn't seem to be true.. (at least not in the example case)

Taken the example from #11084:

    $ ./miniperl -le'sub FETCH{ eval "UNITCHECK{die}"} sub TIESCALAR{bless[]} tie $x, ""; "$x"'

Running it (and tidying the output)

    JMPENV_BOOTSTRAP at perl.c:314 (perl_construct)

    JMPENV_PUSH level=1 at perl.c:1900 (perl_parse)
    JMPENV_POP level=1 at perl.c:1945 (perl_parse)

    JMPENV_PUSH level=1 at perl.c:2696 (perl_run)
            JUMPLEVEL set catch 0 => 1 (for 7ffc12c5fc00) at perl.c:3098 (Perl_call_sv)
            JUMPLEVEL set catch 1 => 0 (for 7ffc12c5fc00) at perl.c:3101 (Perl_call_sv)
            JUMPLEVEL set catch 0 => 1 (for 7ffc12c5fc00) at perl.c:3098 (Perl_call_sv)
            in Entereval: CATCH_GET is TRUE!!
            JMPENV_PUSH level=2 at pp_ctl.c:3416 (S_docatch)
                    JMPENV_PUSH level=3 at perl.c:5184 (Perl_call_list)
                            JMPENV_PUSH level=4 at perl.c:3111 (Perl_call_sv)
                            JMPENV_POP level=4 at perl.c:3156 (Perl_call_sv)
                    JMPENV_POP level=3 at perl.c:5204 (Perl_call_list)
            JMPENV_POP level=2 at pp_ctl.c:3450 (S_docatch)
            JUMPLEVEL set catch 1 => 0 (for 7ffc12c5fc00) at perl.c:3101 (Perl_call_sv)
    JMPENV_POP level=1 at perl.c:2732 (perl_run)

    JMPENV_PUSH level=1 at perl.c:663 (perl_destruct)
    JMPENV_POP level=1 at perl.c:669 (perl_destruct)

This makes the message in commit d7e3f70 a bit confusing..
Reading that commit message gave me the impression there was no jmpenv frame but there already
was one all the way up in perl_run.

What is true is that pp_entereval at that time didn't check CATCH_GET (which Perl_class_sv changed from false to true).
Which then caused exceptions to unwind all the way back to perl_run.

Also note: before commit d7e3f70 the code did end up in S_try_yyparse but it looks that wasn't enough to stop it from crashing.

    $ git checkout d7e3f70f30811328d2f6ae57e5892deccf64d0b2~1
    $ ./Configure ...
    $ make miniperl
    $ ./miniperl -le'sub FETCH{ eval "UNITCHECK{die}"} sub TIESCALAR{bless[]} tie $x, ""; "$x"'
    JUMPENV_PUSH level=1 at perl.c:1716 (perl_parse)
    JUMPENV_POP level=1 at perl.c:1752 (perl_parse)

    JUMPENV_PUSH level=1 at perl.c:2464 (perl_run)
            JUMPLEVEL set catch 0 => 1 (for 7ffdbb8b9900) at perl.c:2854 (Perl_call_sv)
            JUMPLEVEL set catch 1 => 0 (for 7ffdbb8b9900) at perl.c:2857 (Perl_call_sv)
            JUMPLEVEL set catch 0 => 1 (for 7ffdbb8b9900) at perl.c:2854 (Perl_call_sv)

            JUMPENV_PUSH level=2 at pp_ctl.c:3283 (S_try_yyparse)
            JUMPENV_POP level=2 at pp_ctl.c:3295 (S_try_yyparse)

            JUMPENV_PUSH level=2 at perl.c:5037 (Perl_call_list)
                    JUMPENV_PUSH level=3 at perl.c:2867 (Perl_call_sv)
                    JUMPENV_POP level=3 at perl.c:2912 (Perl_call_sv)
            JUMPENV_POP level=2 at perl.c:5057 (Perl_call_list)
            miniperl: pp_hot.c:3877: Perl_pp_leavesub: Assertion `((cx)->cx_u.cx_subst.sbu_type & 0xf) == 9' failed.
            Aborted

TLDR:

What I'm now wondering about: what would happen if pp_entereval would always call JMPENV_PUSH?
That would ensure that errors during eval don't unwind all the way back to perl_run..
Would it be a sane thing to do or not?
(And if so: why wasn't it done before? Are there side effects to consider?)

[bram-perl]

BTW, I have double checked, and in this expression:

yystatus = (!in_require && CATCH_GET) ? S_try_yyparse(aTHX_ GRAMPROG) : yyparse(GRAMPROG);

CATCH_GET is never true in our testsuite, so we never call S_try_yyparse().

I don't trust the testsuite to cover all possible cases so I took another peek at the code:
S_doeval_compile is called twice in pp_ctl.c;

    3972     static OP *
    3973     S_require_file(pTHX_ SV *sv)
    3974     {
    ...
    4496         assert(!CATCH_GET);
    ...
    4505         if (doeval_compile(gimme, NULL, PL_curcop->cop_seq, NULL))

and:

    4554     PP(pp_entereval)
    4555     {
    ....
    4656         assert(!CATCH_GET);
    ....
    4677         if (doeval_compile(gimme, runcv, seq, saved_hh)) {

so in both cases there is an assert(!CATCH_GET);

[iabyn]

On Sat, Aug 27, 2022 at 09:23:38AM -0700, Bram wrote: > BTW, I have double checked, and in this expression: > > ``` > yystatus = (!in_require && CATCH_GET) ? S_try_yyparse(aTHX_ GRAMPROG) : yyparse(GRAMPROG); > ``` >

I haven't looked closely at this yet, but its possible the CATCH_GET in S_doeval_compile is superflouous. Have a read of =head2 Exception handing in pod/perlinterp.pod for a detailed explanation (recently updated by myself) on what CATCH_GET is for,

…

-- But Pity stayed his hand. "It's a pity I've run out of bullets", he thought. -- "Bored of the Rings"

[demerphq]

On Sat, 27 Aug 2022, 21:52 iabyn, ***@***.***> wrote: On Sat, Aug 27, 2022 at 09:23:38AM -0700, Bram wrote: > > BTW, I have double checked, and in this expression: > > > > ``` > > yystatus = (!in_require && CATCH_GET) ? S_try_yyparse(aTHX_ GRAMPROG) : yyparse(GRAMPROG); > > ``` > > I haven't looked closely at this yet, but its possible the CATCH_GET in S_doeval_compile is superflouous. Have a read of =head2 Exception handing in pod/perlinterp.pod for a detailed explanation (recently updated by myself) on what CATCH_GET is for,

I tried removing it, which allows the code to use try_yyparse, and fixes the retainedlines test, unfortunately it causes other tests to segv or fail. I posted the tests that fail already. Yves

…

[demerphq]

On Sat, 27 Aug 2022 at 21:52, iabyn ***@***.***> wrote: On Sat, Aug 27, 2022 at 09:23:38AM -0700, Bram wrote: > > BTW, I have double checked, and in this expression: > > > > ``` > > yystatus = (!in_require && CATCH_GET) ? S_try_yyparse(aTHX_ GRAMPROG) : yyparse(GRAMPROG); > > ``` > > I haven't looked closely at this yet, but its possible the CATCH_GET in S_doeval_compile is superflouous.

Ok. But as I said before, I tried removing it, which effectively means using try_yyparse() for evals and yyparse() for requires. But that then leads to segvs and other test issues.

Have a read of =head2 Exception handing in pod/perlinterp.pod for a detailed explanation (recently updated by myself) on what CATCH_GET is for,

Ive read that whole section a few times and I still am not sure I am wiser. I think I get it, but one thing that strikes me is that the explanation doesn't seem to distinguish "compilation" from "execution", and that some of the hacks related to one might not be relevant for the other. I use quotes because I realize that compilation can become execution via BEGIN. We take pains to minimize the nested stack depth due to eval. So for instance the docs says: <QUOTE> As well as setting catches at the entry points to the perl interpreter, you might expect perl to also do a JMPENV_PUSH() in places like pp_entertry(), just before some trappable ops are executed. In fact perl doesn't normally do this. The drawback with doing it is that with nested or recursive code such as: sub foo { my ($i) = @_; return if $i < 0; eval { foo(--$i) } } Then the C stack would quickly overflow with pairs of entries like #N+3 Perl_runops() #N+2 Perl_pp_entertry() #N+1 Perl_runops() #N Perl_pp_entertry() Instead, perl puts its guards at the I<callers> of runops loops. Then as many nested subroutine calls and evals may be called as you like, all within the one runops loop. If an exception occurs, control passes back to the caller of the loop, which just immediately restarts a new loop with C<PL_restartop> being the next op to call. </QUOTE> But does that make sense for the *compilation* phase of eval STRING? BEGIN allows "compilation" to turn into execution, so I guess it would help: ./perl -le'sub f{ eval "BEGIN{ f() }" } f()' but that seems to segfault anyway so we probably have to deal with it some other way anyhow. (Working on it) If we leave insane things with BEGIN inside of eval aside, we shouldnt have the "nested eval problem" with compilation as after we finish compiling we can clean up any new frames and before we execute any code that might create another nesting. Which means that we actually have two kinds of "croak/die", that during compilation, where we want to return control to the compiling logic, and that during execution, where the existing semantics need to be preserved. I guess that is what try_yyparse() tries to achieve, but somehow it gets muddled up. If I enable it (by removing the CATCH_GET clause) cpan/IO-Compress/t/020isize.t segfaults just after the plan() call in the BEGIN block in the test file. Unfortunate the whole test infrastructure gets in the way and I haven't yet been able to reduce that one down to a simple case. In theory it is something like: BEGIN{ exit(0) }, but that alone doesn't trigger the segv. Anyway, I'm doing my best to debug this but I think it needs your eyes Dave. There is too much here I am new to.