Skip to content

panic: restartop in perl_run #19680

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

Closed
AnFunctionArray opened this issue May 1, 2022 · 12 comments
Closed

panic: restartop in perl_run #19680

AnFunctionArray opened this issue May 1, 2022 · 12 comments

Comments

@AnFunctionArray
Copy link
Contributor

AnFunctionArray commented May 1, 2022

Module: perl

Description

When I run "test" =~ m{(?{eval {exists $test[-1]{test}}})}sxx (fails all the time)

Steps to Reproduce
Run.

Expected behavior
Not panic.

Perl configuration

Summary of my perl5 (revision 5 version 35 subversion 12) configuration:
  Commit id: 99db5f9692dfa6466693dce901a6e805243181fc
  Platform:
    osname=darwin
    osvers=21.4.0
    archname=darwin-2level
    uname='darwin alexanders-macbook-air.local 21.4.0 darwin kernel version 21.4.0: fri mar 18 00:47:26 pdt 2022; root:xnu-8020.101.4~15release_arm64_t8101 arm64 '
    config_args='-des -Dusedevel'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='cc'
    ccflags ='-fno-common -DPERL_DARWIN -mmacosx-version-min=12.3 -fno-strict-aliasing -pipe -fstack-protector-strong -I/opt/local/include -DPERL_USE_SAFE_PUTENV'
    optimize='-O3'
    cppflags='-fno-common -DPERL_DARWIN -mmacosx-version-min=12.3 -fno-strict-aliasing -pipe -fstack-protector-strong -I/opt/local/include'
    ccversion=''
    gccversion='Apple LLVM 13.1.6 (clang-1316.0.21.2.3)'
    gccosandvers=''
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=8
    longdblkind=0
    ivtype='long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='cc'
    ldflags =' -mmacosx-version-min=12.3 -fstack-protector-strong -L/opt/local/lib'
    libpth=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/13.1.6/lib /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/lib /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib /opt/local/lib /usr/lib
    libs= 
    perllibs=
    libc=
    so=dylib
    useshrplib=false
    libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=bundle
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags=' '
    lddlflags=' -mmacosx-version-min=12.3 -bundle -undefined dynamic_lookup -L/opt/local/lib -fstack-protector-strong'


Characteristics of this binary (from libperl): 
  Compile-time options:
    HAS_TIMES
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_MALLOC_WRAP
    PERL_OP_PARENT
    PERL_PRESERVE_IVUV
    PERL_USE_DEVEL
    PERL_USE_SAFE_PUTENV
    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
  Built under darwin
  Compiled at Apr 21 2022 21:24:15
  @INC:
    /usr/local/lib/perl5/site_perl/5.35.12/darwin-2level
    /usr/local/lib/perl5/site_perl/5.35.12
    /usr/local/lib/perl5/5.35.12/darwin-2level
    /usr/local/lib/perl5/5.35.12
@jkeenan
Copy link
Contributor

jkeenan commented May 1, 2022 via email

@richardleach
Copy link
Contributor

Using this tree: https://github.com/AnFunctionArray/regularc/tree/7287e18833598aae53a23c23141f60ee15a527e7 (clone recursively for examples)

Run the following test command:

perl ./regexes/parse.pl ./regularcbulk/tests/test.test.c

At least at first glance, ./regexes/parse.pl seems to have a lot going on, which is likely to deter volunteers from looking into this issue. Two things that would help:

  • A much simplified test case that triggers the bug
  • At least telling us how far through parse.pl, and ideally in which subroutine and/or module, the panic occurs

@AnFunctionArray
Copy link
Contributor Author

I found it:

Try:

"test" =~ m{(?{eval {exists $test[-1]{test}}})}sxx

@AnFunctionArray AnFunctionArray changed the title Sporadic behavior - occasional panic: restartop in perl_run panic: restartop in perl_run May 3, 2022
@hvds
Copy link
Contributor

hvds commented May 3, 2022

Confirmed with blead:

% ./miniperl -e '"test" =~ m{(?{eval {exists $test[-1]{test}}})}'
panic: restartop in perl_run
% 

Until some point between 5.22 and 5.30 it also then went on to assert in debugging builds:

% /opt/v5.22.1-d0/bin/perl -e '"test" =~ m{(?{eval {exists $test[-1]{test}}})}'
panic: restartop in perl_run
perl: perl.c:539: perl_destruct: Assertion `PL_scopestack_ix == 1' failed.
Aborted (core dumped)
% 

At 5.18 it doesn't complain; I'll try bisecting on that, but my first guess is that a change in 5.20 just exposed an even older bug.

It seems remotely possible this could be related to #19390, where we get a SEGV from:

  perl -wle 'use strict; use re "eval"; "" =~ m{(?{ eval q{ $x = 1 } })}'

@richardleach
Copy link
Contributor

@hvds, is it relevant that something different happens with a couple of spaces in the mix? (i.e. is there a parsing bug?)

# perl -le '"test" =~ m{( ?{eval {exists $test[-1]{test}}} )}'
Modification of non-creatable array value attempted, subscript -1 at -e line 1.

@haarg
Copy link
Contributor

haarg commented May 3, 2022

With the spaces, it stops being an embedded code block. $test[-1]{test} just becomes an interpolated variable. Autovivifying an empty array with a -1 index gives that error.

$ perl -le '$test[-1]{test}'
Modification of non-creatable array value attempted, subscript -1 at -e line 1.

@hvds
Copy link
Contributor

hvds commented May 3, 2022

With the spaces, it stops being an embedded code block. $test[-1]{test} just becomes an interpolated variable. Autovivifying an empty array with a -1 index gives that error.

$ perl -le '$test[-1]{test}'
Modification of non-creatable array value attempted, subscript -1 at -e line 1.

Ah, I hadn't noticed that it would be an error, that suggests it's much more likely to be related to #19390.

@richardleach
Copy link
Contributor

With the spaces, it stops being an embedded code block.

Thanks. I've never used re code blocks and wasn't sure whether whitespacing was allowed or not.

@hvds
Copy link
Contributor

hvds commented May 3, 2022

Manual bisect points to 1022336 as the point we first see the problem (strange, since the immediately preceding commit cdec98f looks more likely to be relevant; however reverting that one in blead doesn't fix the problem).

Unfortunately, simply reverting the main element of that commit as below causes miniperl to hang both in write_buildcustomize and in the test case - some following commits (e427370, 72621f8, c899ae2) built on the theme and there may have been other work later that assumed this work.

@iabyn does anything spring to mind?

--- a/op.c
+++ b/op.c
@@ -17573,7 +17573,7 @@ Perl_rpeep(pTHX_ OP *o)
         case OP_LINESEQ:
         case OP_SCOPE:
         nothin:
-            if (oldop) {
+            if (oldop && o->op_next) {
                 oldop->op_next = o->op_next;
                 o->op_opt = 0;
                 continue;

@iabyn
Copy link
Contributor

iabyn commented May 4, 2022 via email

iabyn added a commit that referenced this issue May 28, 2022
GH #19680

Normally in code like

    eval {.... };

then even if the eval is the last statement in the file or sub, the
OP_LEAVETRY isn't the last op in the execution path: it's followed
by an OP_LEAVE or OP_LEAVESUB or whatever, which will be the op to
resume execution from after an exception is caught.

However, if the eval is the *last* thing within a regex code block:

    /(?{ ...; eval {....}; })/

then the op_next pointer of the OP_LEAVETRY op is actually NULL.

This confused S_docatch(), which wrongly assumed that a NULL
PL_restartop indicated that the caught exception should be rethrown,
popping execution back to the outer perl_run() call and hence leading to
the confused panic warning:

    "panic: restartop in perl_run"

The fix is to to separate out the "do we need to re-throw" test,
(PL_restartjmpenv != PL_top_env), from the "no more ops so no need to
re-enter the runops loop" test, (!PL_restartop).
@iabyn
Copy link
Contributor

iabyn commented May 28, 2022 via email

iabyn added a commit that referenced this issue Jun 20, 2022
GH #19680

Normally in code like

    eval {.... };

then even if the eval is the last statement in the file or sub, the
OP_LEAVETRY isn't the last op in the execution path: it's followed
by an OP_LEAVE or OP_LEAVESUB or whatever, which will be the op to
resume execution from after an exception is caught.

However, if the eval is the *last* thing within a regex code block:

    /(?{ ...; eval {....}; })/

then the op_next pointer of the OP_LEAVETRY op is actually NULL.

This confused S_docatch(), which wrongly assumed that a NULL
PL_restartop indicated that the caught exception should be rethrown,
popping execution back to the outer perl_run() call and hence leading to
the confused panic warning:

    "panic: restartop in perl_run"

The fix is to to separate out the "do we need to re-throw" test,
(PL_restartjmpenv != PL_top_env), from the "no more ops so no need to
re-enter the runops loop" test, (!PL_restartop).
iabyn added a commit that referenced this issue Jun 20, 2022
Fix "panic: restartop in perl_run" (Issue #19680),
Then do tweaks and doc improvements to perl's internal exception handling.
@iabyn
Copy link
Contributor

iabyn commented Jun 20, 2022

Fixed by 5fd637c,
within the branch the branch 2c8b8b7 [MERGE] exception issues

@iabyn iabyn closed this as completed Jun 20, 2022
iabyn added a commit that referenced this issue Jul 9, 2022
GH #19390

This issue is similar to GH #19680 (fixed by v5.37.0-272-g5fd637ce8c),
but exiting the eval via a syntax error rather than via raising an
exception.

In this case, the NULL op_next of the OP_ENTEREVAL was passed to a new
RUNOPS loop, which would normally SEGV (or on debugging builds, warn
"NULL OP IN RUN").
scottchiefbaker pushed a commit to scottchiefbaker/perl5 that referenced this issue Nov 3, 2022
GH Perl#19680

Normally in code like

    eval {.... };

then even if the eval is the last statement in the file or sub, the
OP_LEAVETRY isn't the last op in the execution path: it's followed
by an OP_LEAVE or OP_LEAVESUB or whatever, which will be the op to
resume execution from after an exception is caught.

However, if the eval is the *last* thing within a regex code block:

    /(?{ ...; eval {....}; })/

then the op_next pointer of the OP_LEAVETRY op is actually NULL.

This confused S_docatch(), which wrongly assumed that a NULL
PL_restartop indicated that the caught exception should be rethrown,
popping execution back to the outer perl_run() call and hence leading to
the confused panic warning:

    "panic: restartop in perl_run"

The fix is to to separate out the "do we need to re-throw" test,
(PL_restartjmpenv != PL_top_env), from the "no more ops so no need to
re-enter the runops loop" test, (!PL_restartop).
scottchiefbaker pushed a commit to scottchiefbaker/perl5 that referenced this issue Nov 3, 2022
GH Perl#19390

This issue is similar to GH Perl#19680 (fixed by v5.37.0-272-g5fd637ce8c),
but exiting the eval via a syntax error rather than via raising an
exception.

In this case, the NULL op_next of the OP_ENTEREVAL was passed to a new
RUNOPS loop, which would normally SEGV (or on debugging builds, warn
"NULL OP IN RUN").
steve-m-hay pushed a commit that referenced this issue Apr 10, 2023
GH #19680

Normally in code like

    eval {.... };

then even if the eval is the last statement in the file or sub, the
OP_LEAVETRY isn't the last op in the execution path: it's followed
by an OP_LEAVE or OP_LEAVESUB or whatever, which will be the op to
resume execution from after an exception is caught.

However, if the eval is the *last* thing within a regex code block:

    /(?{ ...; eval {....}; })/

then the op_next pointer of the OP_LEAVETRY op is actually NULL.

This confused S_docatch(), which wrongly assumed that a NULL
PL_restartop indicated that the caught exception should be rethrown,
popping execution back to the outer perl_run() call and hence leading to
the confused panic warning:

    "panic: restartop in perl_run"

The fix is to to separate out the "do we need to re-throw" test,
(PL_restartjmpenv != PL_top_env), from the "no more ops so no need to
re-enter the runops loop" test, (!PL_restartop).

(cherry picked from commit 5fd637c)
steve-m-hay pushed a commit that referenced this issue Apr 10, 2023
GH #19390

This issue is similar to GH #19680 (fixed by v5.37.0-272-g5fd637ce8c),
but exiting the eval via a syntax error rather than via raising an
exception.

In this case, the NULL op_next of the OP_ENTEREVAL was passed to a new
RUNOPS loop, which would normally SEGV (or on debugging builds, warn
"NULL OP IN RUN").

(cherry picked from commit 16e43ef)
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

No branches or pull requests

6 participants