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
Unknow address reference in Perl_hv_common #16028
Comments
From @Mipu94Attached file below triggered crash because reference to unknow address in Some info about this bug on gdb-peda: [----------------------------------registers-----------------------------------] -- |
From @tonycozOn Tue, 20 Jun 2017 00:41:01 -0700, tadinhsung@gmail.com wrote:
Simplifies to: my $unknown_pat = qr/__ANON__::/; package SubHelper; On a debugging build: miniperl: hv.c:353: Perl_hv_common: Assertion `((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV' failed. The stash passed to hv_fetch is a regexp: miniperl: hv.c:360: Perl_hv_common: Assertion `((svtype)((hv)->sv_flags & 0xff)) == SVt_PVHV' failed. Program received signal SIGABRT, Aborted. Bisects to: commit bee7c57 sv.c: Don’t fiddle with AMAGIC in sv_bless but I suspect this just a case of a freed SV being overwritten. I don't think this is a security issue. Tony |
The RT System itself - Status changed from 'new' to 'open' |
From @Mipu94Sorry, i'm not really good on perl :( . But when i debug perl( RAX: 0x6210002149c8 --> 0x0 Obviously, at address 0x4b03c4 register edi was loaded from address If attacker can init value for section gdb-peda$ pdisass 0x4b0ba0 at address 0x4b0ba7 variable [rsp+0x28](orig_entry) can control, 2017-06-21 9:02 GMT+07:00 Tony Cook via RT <perl5-security-report@perl.org>:
-- |
From @iabynOn Tue, Jun 20, 2017 at 12:41:02AM -0700, sung wrote:
The code can be reduced to package F { my $sub = sub { F::x(); } ; delete ${'::'}{'F::'}; my $pat = qr/X/; eval { $sub->() }; the call to the class method 'blah->c' involves a lookup of the stash delete ${'::'}{'F::'}; frees the stash and leaves the cops pointing at a freed or reallocated SV This doesn't seem like a security issue, since malicious code that has the If no-one objects, I'll move this to the public queue soon. -- |
From @demerphqOn 21 June 2017 at 16:57, Dave Mitchell <davem@iabyn.com> wrote:
No objection, but i have a question. I chased this down a bit as well, Here is what I see when I trace the code, we end up calling if (!stash && !find_default_stash(&stash, name, len, is_utf8, add, Which looks like this in the gdb trace: S_find_default_stash (my_perl=0xaee010, stash=0x7fffffffd770, When I dump the stash object its REFCNT is 1, which doesnt seem to And, if I revert FC's patch the bug goes away. FC's patch amounts to this: if (Gv_AMG(stash)) Which BTW, results in the else branch being called, however, it Gv_AMG(stash); then the segfault also goes away. This combined with the fact that Yves -- |
From @iabynOn Wed, Jun 21, 2017 at 05:14:22PM +0200, demerphq wrote:
What you're seeing is an SV that has been freed, and then later -- |
From @demerphqOn 21 June 2017 at 17:14, demerphq <demerphq@gmail.com> wrote:
And If I dump the Regexp stash in sv.c with and without the With: SV = PVHV(0xaf99f8) at 0xb1a2b8 Without: SV = PVHV(0xaf99f8) at 0xb1a2b8 -- |
From @demerphqOn 21 June 2017 at 17:29, Dave Mitchell <davem@iabyn.com> wrote:
Yes, but my point is that somehow the Gv_AMG() changes the original Yves -- |
From @demerphqOn 21 June 2017 at 17:32, demerphq <demerphq@gmail.com> wrote:
With the Gv_AMG() call, PL_curcop ends up as NULL not as a dangling Yves -- |
From @demerphqOn 21 June 2017 at 17:41, demerphq <demerphq@gmail.com> wrote:
So for instance, if I take the code that FC removed from sv_bless() $ git diff Inline Patchdiff --git a/pp_hot.c b/pp_hot.c
index 7c98c90..2394464 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -1923,6 +1923,10 @@ PP(pp_qr)
HV *const stash = gv_stashsv(pkg, GV_ADD);
SvREFCNT_dec_NN(pkg);
(void)sv_bless(rv, stash);
+ if (Gv_AMG(stash))
+ SvAMAGIC_on(rv);
+ else
+ (void)SvAMAGIC_off(rv);
}
if (UNLIKELY(RX_ISTAINTED(rx))) {
-- perl -Mre=debug -e "/just|another|perl|hacker/" |
From @iabynOn Wed, Jun 21, 2017 at 05:49:36PM +0200, demerphq wrote:
I'm afraid you are mistaken. With all 6 combinations of I see an assertion failure, and using a watchpoint under gdb shows the I think everything else you see is just a side affect of I think FC's patch is probably correct. I don't remember the full details, -- |
From @cpansproutOn Thu, 22 Jun 2017 03:55:01 -0700, davem wrote:
This variant fails an assertion all the way back to 5.002: { package F ; The $thing assignment (with a ridiculous repeat count for good measure) makes sure that the freed stash gets reused as an IV. How do we go about fixing this kind of bug? For GVs being freed when there are still CVs, we give the CV a strong pointer, instead of a weak one. I.e., we switch the direction of the weak pointer. That complicates the code, so I wonder whether it is the right thing to do in the case of freed stashes, which are much rarer than freed globs. Would the right thing to do be simply to plug the code paths that can fail an assertion when CopSTASH(&PL_curcop) is something weird (as I did for pp_caller)? What about those cases when a freed stash is reused as another stash (in which case you get subtle bugs hard to track down)? I thought about perhaps nulling out the pointer in PL_stashpad when the stash is freed (and plugging the crashing code paths), but that does not help us with non-threaded builds, which have their stash pointers directly in the COP. (And I don’t like to expand the complexity of PL_stashpad to unthreaded builds, just for the sake of this.) BTW, the test case crashes because it is looking to see whether the class method call could actually be a filehandle method call, which takes precedence over classes.
Yes, that is how it works. That means that for most classes you get exactly one calculation of the overload table, not one for each blessing. And you can change your overload methods and have them take effect immediately on existing objects, instead of waiting for the next blessing to come along. -- Father Chrysostomos |
From @tonycozOn Wed, 21 Jun 2017 07:57:14 -0700, davem wrote:
I agree, now public. Tony |
Migrated from rt.perl.org#131606 (status was 'open')
Searchable as RT131606$
The text was updated successfully, but these errors were encountered: