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

Programming more defensively to avoid SEGV #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mohawk2
Copy link

@mohawk2 mohawk2 commented Nov 22, 2018

In certain circumstances, doing a set_subname has caused SEGV while running under Devel::Cover. I have not dug very deeply into exactly why, but I can show the repro case if needed. The immediate reason is that in that scenario, GvSTASH(oldgv) returns NULL, yet HvNAME is called on it.

This patch makes the block only continue with the attempt to "under debugger, provide information about sub location" if oldhv is not NULL.

@karenetheridge
Copy link
Member

This should also be ported to Sub::Name as well.

@leonerd
Copy link
Contributor

leonerd commented Nov 22, 2018

(as per IRC but here again for completeness) Can you provide a test case, or otherwise an example of what situations cause the bug that this code is fixing?

@mohawk2
Copy link
Author

mohawk2 commented Nov 22, 2018

Minimal repro:

  • git clone git@github.com:preaction/Yancy.git
  • Put the text below in t/standalone-segv.t
  • perl -MDevel::Cover -Ilib -It/lib t/standalone-segv.t
use Test::Mojo;
use Local::Test qw( init_backend );
my ( $url ) = init_backend({});
Test::Mojo->new('Yancy', { backend => $url, collections => {} })->get_ok('/');
Test::Mojo->new('Yancy', { backend => $url, collections => {} })->get_ok('/yancy');

@mohawk2
Copy link
Author

mohawk2 commented Nov 22, 2018

Additional info, if relevant: the bit that blows up is Mojo::Util::monkey_patch called in Mojo::Template::process.

@mohawk2
Copy link
Author

mohawk2 commented Nov 22, 2018

@karenetheridge Doing so now!

@Leont
Copy link
Member

Leont commented Nov 22, 2018

In certain circumstances, doing a set_subname has caused SEGV while running under Devel::Cover.

Can you post a stacktrace of that crash?

@rurban
Copy link
Contributor

rurban commented Nov 23, 2018

This fix is wrong.
I fixed this in cperl by checking for !oldhv or oldhv == PL_defstash, and then I'm using a new empty string for the stash name. "__ANON__" would also be okay, but this codepath is only for the DB::sub hook.
Same problem exists for the new stash also.

rurban added a commit to rurban/Scalar-List-Utils that referenced this pull request Nov 23, 2018
See Dual-Life#74 for
the testcase (Devel::Cover providing no stash)
rurban added a commit to perl11/cperl that referenced this pull request Nov 23, 2018
in set_subname.
See Dual-Life/Scalar-List-Utils#74 for
the testcase (Devel::Cover providing no stash)
@Leont
Copy link
Member

Leont commented Nov 23, 2018

The fix by @rurban does look more logical to me, though I'm still wondering how a CV ends up with a stashless GV

@mohawk2
Copy link
Author

mohawk2 commented Dec 11, 2018

@Leont belatedly:

#0  XS_Sub__Util_set_subname (cv=<optimised out>) at ListUtil.xs:1608
#1  0x000055f495ea5dd6 in Perl_pp_entersub () at pp_hot.c:5232
#2  0x000055f495e61c52 in Perl_runops_debug () at dump.c:2536
#3  0x000055f495dd535e in S_run_body (oldscope=1) at perl.c:2689
#4  perl_run (my_perl=<optimised out>) at perl.c:2617
#5  0x000055f495d9eda2 in main (argc=<optimised out>, argv=<optimised out>, 
    env=<optimised out>) at perlmain.c:122

Do you want me to change my fix to be the same as @rurban 's? Or can you just cherry-pick his? I'm keen for SEGVs to not happen, I don't mind whose code makes it so :-)

@Leont
Copy link
Member

Leont commented Dec 11, 2018

In either case, this really needs a test.

@mohawk2
Copy link
Author

mohawk2 commented Dec 11, 2018

It certainly does. Sadly I don't know what triggers the behaviour it's protecting from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants