-
Notifications
You must be signed in to change notification settings - Fork 560
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
Windows fork emulation's child pseudo process cannot restore local scalar values #8641
Comments
From ebhanssen@allverden.noThis is a bug report for perl from ebhanssen@allverden.no, I believe this is a bug in the Windows fork emulation. Basically, a localized scalar value is not restored in a child | C:\>perl -e "$s='outer'; { local $s; exit if fork } print $s, defined($s)" Compare the same code with "real" forks: | ~$ perl -le '$s="outer"; { local $s; exit if fork } print $s, defined($s)' The following longer example reveals more -- giving us both "Attempt | #!perl With real forks, this works just fine, but on Windows, I get the | C:\Share>perl bug.pl Similar testing reveals that the bug does not affect array, hash, Thanks, PS: Sorry if this is a known issue. I searched "fork local" and Flags: Site configuration information for perl v5.8.8: Configured by SYSTEM at Tue Aug 29 12:39:43 2006. Summary of my perl5 (revision 5 version 8 subversion 8) configuration: Locally applied patches: @INC for perl v5.8.8: Environment for perl v5.8.8: |
From @bulk88On Wed Oct 18 19:27:09 2006, ebhanssen wrote:
Using the above script with a DEBUG_LEAKING_SCALARS perl 5.17.6 no
The scalar was actually freeded at
The scalar was attempted to be freeded at
case SAVEt_SV: /* scalar reference */
if (SvSMAGICAL(ARG0_SV)) {
The rogue savestack entry came from Perl_ss_dup during the fork, the -- |
The RT System itself - Status changed from 'new' to 'open' |
From @bulk88On Mon Dec 24 16:55:40 2012, bulk88 wrote:
The bug seems to be, the SV given to ss_dup, for duplicating a SAVEt_SV, In the child is goes like this, The SV head being alloced again quickly happens to be by chance. There is also a 2nd free unreferenced scalar warning that never makes it
In cv_undef, the _dec call came from
} |
From @bulk88On Wed Dec 26 01:12:28 2012, bulk88 wrote:
The problem I think is that This patch stops the unreferenced warnings with Now, I have no idea if this is the correct fix, or side effects of this The other choice is to reverse 4e4c362 . Another choice would be to add a localize stack/array to a GP/GV -- |
From @bulk880001-fix-40565-a-SV-on-save-stack-entry-for-SAVEt_SV-need.patchFrom e07cf8c395d2c4dca2d22fd5b40fc9372b8b712a Mon Sep 17 00:00:00 2001
From: Daniel Dragan <bulk88@hotmail.com>
Date: Wed, 26 Dec 2012 06:51:28 -0500
Subject: [PATCH] fix #40565, a SV on save stack entry for SAVEt_SV needs 2
refcount ++s
I dont know if this is the correct fix. Commit 4e4c362ec2e gave the save
stack entry a refcnt ownership (++) on the SV removed from the GV, yet
the GV still owns a different ownership (++) on the same SV. The SV has
only one reachable link now that it was removed from the GV, through the SS
entry. So during a clone/psudofork, the SV will not be found from the GV,
it will be found only through duping the SS entry, so the SS entry needs 2
++s, not 1 ++.
---
sv.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/sv.c b/sv.c
index 9fc807a..e95d333 100644
--- a/sv.c
+++ b/sv.c
@@ -12699,12 +12699,19 @@ Perl_ss_dup(pTHX_ PerlInterpreter *proto_perl, CLONE_PARAMS* param)
case SAVEt_GVSV: /* scalar slot in GV */
case SAVEt_SV: /* scalar reference */
sv = (const SV *)POPPTR(ss,ix);
- TOPPTR(nss,ix) = sv_dup_inc(sv, param);
+ {
+ SV * sv2 = sv_dup_inc(sv, param);
+ SvREFCNT_inc_simple_void_NN(sv2);
+ TOPPTR(nss,ix) = sv2;
+ }
/* fall through */
case SAVEt_FREESV:
case SAVEt_MORTALIZESV:
sv = (const SV *)POPPTR(ss,ix);
- TOPPTR(nss,ix) = sv_dup_inc(sv, param);
+ {
+ SV * sv2 = sv_dup_inc(sv, param);
+ TOPPTR(nss,ix) = sv2;
+ }
break;
case SAVEt_SHARED_PVREF: /* char* in shared space */
c = (char*)POPPTR(ss,ix);
--
1.7.9.msysgit.0
|
From @iabynOn Wed, Dec 26, 2012 at 04:01:11AM -0800, bulk88 via RT wrote:
I think the issue is not so much Perl_save_scalar() incrementing the ref I suspect (although this is based on a cursory look at the issue) that But given that S_save_scalar_at() isn't just used to save GVSV slots, but But I think the basic principle is sound: i.e. the SV always has a -- |
From @bulk88On Wed Dec 26 04:01:10 2012, bulk88 wrote:
Bump. This bug is fully diagnosed with a but maybe not the best patch for a fix. Can we this ticket rolling to be fixed for 5.20? -- |
From @tonycozOn Tue Jan 21 12:05:17 2014, bulk88 wrote:
I don't think your patch is correct - did you notice Dave's response? I'll look at producing a fix, but I'm not sure it will happen for 5.20. Tony |
From @bulk88On Tue Jan 21 16:05:09 2014, tonyc wrote:
I didn't fully understand davem's response. So under his solution, S_save_scalar_at will sometimes -- the old SV, even though a couple lines earlier in execution, in Perl_save_scalar, the old SV got unconditionally ++ed, AND http://perl5.git.perl.org/perl.git/commitdiff/4e4c362ec2e3f4b5f5c23aa83a26a13b85d0c2c1 is not reverted?
There is a 1% chance I might do a fix, if you beat me to it, I'm fine with it. -- |
From @iabynOn Tue, Jan 21, 2014 at 05:07:53PM -0800, bulk88 via RT wrote:
I'm looking at at the moment. -- |
From @iabynOn Wed, Dec 26, 2012 at 04:01:11AM -0800, bulk88 via RT wrote:
This description is correct, but it turns out that this extra ref count is Its needed for the tied array/hash element localisation, because when So the extra ref needs to stay, and instead the fixup needs doing in
However, the above is wrong in several ways (I'm not sure whether this is Can you rework this so that it does only the sv arg of SAVEt_SV, and -- |
From @dex4er2014/1/23 Dave Mitchell <davem@iabyn.com>:
I've noticed that Strawberry Perl can be run with wine on Linux. I've dexter@pepper:~$ wine cmd Z:\home\dexter>set PATH=C:\strawberry\perl\bin;%PATH% Z:\home\dexter>perl -v This is perl 5, version 18, subversion 2 (v5.18.2) built for Z:\home\dexter>perl test_fork_exit.pl Z:\home\dexter>perl test_rt40565.pl I've put the test scripts also to my repo. This bug causes that my https://github.com/dex4er/Stardust/blob/master/examples/test_fork_exit.pl Seems that it is a way to test Windows implementation on Linux too. -- |
From @iabynOn Thu, Jan 23, 2014 at 04:46:54PM +0100, Piotr Roszatycki wrote:
There's a big difference between installing Strawberry Perl under wine, -- |
From @bulk88Another report of this bug https://rt.perl.org/Ticket/Display.html?id=123277 . 8 years and counting on this ticket. My first try at fixing it was ruled wrong https://rt-archive.perl.org/perl5/Ticket/Display.html?id=40565#txn-1277127 , and I am not confident enough to rework it. -- |
From @cpansproutOn Sat Nov 22 22:00:47 2014, bulk88 wrote:
Does this patch work for you? -- Father Chrysostomos |
From @cpansproutInline Patchdiff --git a/ext/XS-APItest/t/clone-with-stack.t b/ext/XS-APItest/t/clone-with-stack.t
index 3f68c93..179fba0 100644
--- a/ext/XS-APItest/t/clone-with-stack.t
+++ b/ext/XS-APItest/t/clone-with-stack.t
@@ -17,7 +17,7 @@ if (not $Config{'useithreads'}) {
skip_all("clone_with_stack requires threads");
}
-plan(5);
+plan(6);
fresh_perl_is( <<'----', <<'====', undef, "minimal clone_with_stack" );
use XS::APItest;
@@ -78,3 +78,26 @@ f();
====
}
+
+{
+ fresh_perl_is( <<'----', <<'====', undef, "with localised stuff" );
+use XS::APItest;
+$s = "outer";
+$a[0] = "anterior";
+$h{k} = "hale";
+{
+ local $s = "inner";
+ local $a[0] = 'posterior';
+ local $h{k} = "halt";
+ clone_with_stack();
+}
+print "scl: $s\n";
+print "ary: $a[0]\n";
+print "hsh: $h{k}\n";
+----
+scl: outer
+ary: anterior
+hsh: hale
+====
+
+}
diff --git a/sv.c b/sv.c
index c94a529..820cdc4 100644
--- a/sv.c
+++ b/sv.c
@@ -13910,14 +13910,16 @@ Perl_ss_dup(pTHX_ PerlInterpreter *proto_perl, CLONE_PARAMS* param)
case SAVEt_CLEARPADRANGE:
break;
case SAVEt_HELEM: /* hash element */
+ case SAVEt_SV: /* scalar reference */
sv = (const SV *)POPPTR(ss,ix);
- TOPPTR(nss,ix) = sv_dup_inc(sv, param);
+ TOPPTR(nss,ix) = SvREFCNT_inc(sv_dup_inc(sv, param));
/* FALLTHROUGH */
case SAVEt_ITEM: /* normal string */
case SAVEt_GVSV: /* scalar slot in GV */
- case SAVEt_SV: /* scalar reference */
sv = (const SV *)POPPTR(ss,ix);
TOPPTR(nss,ix) = sv_dup_inc(sv, param);
+ if (type == SAVEt_SV)
+ break;
/* FALLTHROUGH */
case SAVEt_FREESV:
case SAVEt_MORTALIZESV:
@@ -13935,6 +13937,8 @@ Perl_ss_dup(pTHX_ PerlInterpreter *proto_perl, CLONE_PARAMS* param)
case SAVEt_SVREF: /* scalar reference */
sv = (const SV *)POPPTR(ss,ix);
TOPPTR(nss,ix) = sv_dup_inc(sv, param);
+ if (type == SAVEt_SVREF)
+ SvREFCNT_inc_simple_void((SV *)TOPPTR(nss,ix));
ptr = POPPTR(ss,ix);
TOPPTR(nss,ix) = svp_dup_inc((SV**)ptr, proto_perl);/* XXXXX */
break;
@@ -14087,7 +14091,7 @@ Perl_ss_dup(pTHX_ PerlInterpreter *proto_perl, CLONE_PARAMS* param)
break;
case SAVEt_AELEM: /* array element */
sv = (const SV *)POPPTR(ss,ix);
- TOPPTR(nss,ix) = sv_dup_inc(sv, param);
+ TOPPTR(nss,ix) = SvREFCNT_inc(sv_dup_inc(sv, param));
i = POPINT(ss,ix);
TOPINT(nss,ix) = i;
av = (const AV *)POPPTR(ss,ix); |
From @bulk88On Tue Nov 25 21:44:46 2014, sprout wrote:
Yes and no. before with patched .t (to prove it fails since Father C doesn't have a Win32 machine) C:\perl521\srcnewb4opt\t>perl -I..\lib harness -v ext\XS-APItest\t\clone-with-st Test Summary Report ../ext/XS-APItest/t/clone-with-stack.t (Wstat: 0 Tests: 6 Failed: 1) C:\perl521\srcnewb4opt\t> After C:\perl521\srcnewb4opt\t>perl -I..\lib harness -v ext\XS-APItest\t\clone-with-st C:\perl521\srcnewb4opt\t> There is another crash in op/fork.t that this patch doesn't fix. before, a op/fork.t crash, the crash isn't in fork.t but a child proc since all the tests use a fresh perl C:\perl521\srcnewb4opt\t>perl -I..\lib harness -v op\fork.t Test Summary Report op/fork.t (Wstat: 0 Tests: 28 Failed: 1) C:\perl521\srcnewb4opt\t> crash code isolated $| = 1; with debugging [Win32's OS poisoning, not perl's poisoning] Heap on Win32 reliable causes a crash with "Unhandled exception at 0x280882b6 (perl521.dll) in perl.exe: 0xC0000005: Access violation reading location 0xfeeefef2." Pointer 0xfeeefef2 is freed memory obviously. Without debugging heap, this very very rarely and randomly crashes for me crash call stack perl521.dll!S_SvREFCNT_dec(interpreter * my_perl=0x0096173c, sv * sv=0x0000001d) Line 162 C IDK whether the patch should be applied, it fixes the local bug, but a similar save stack refcount bug still exists somewhere else. -- |
From @cpansproutOn Sun Nov 30 18:55:18 2014, bulk88 wrote:
Does this fork test fail the same way without my patch? -- Father Chrysostomos |
From @bulk88On Sun Nov 30 21:57:51 2014, sprout wrote:
Your patch only affects SAVEt_SV, my rejected patch in https://rt-archive.perl.org/perl5/Ticket/Display.html?id=40565#txn-1180396 was for types SAVEt_GVSV and SAVEt_SV (and SAVEt_ITEM but IDK what SAVEt_ITEM is) and the fork.t test 6 crash is with type SAVEt_GVSV, not SAVEt_SV. perl521.dll!Perl_save_pushptrptr(interpreter * my_perl=0x00365d74, void * const ptr1=0x00368d14, void * const ptr2=0x0090d7c4, const int type=29) Line 209 C pp_enteriter line is http://perl5.git.perl.org/perl.git/blob/30c67ec60146a2650dd27e865da8a2e7b216b925:/pp_ctl.c#l2118 curcop is at $| = 1; case SAVEt_ITEM: /* normal string */ in the pp_enteriter savestack entry, what goes into sv_dup_inc has refcount 2, newsv (thats my code) comes out with refcnt 1 but that doesn't seem to be the reason for the crash. The GP struct was freed at some point per the attached pic. I will need to do more debugging on why the GP struct was freed. I am hesitant to revert before your patch due to compiling time, so I'll go after the GP struct problem. -- |
From @bulk88On Sun Nov 30 23:17:37 2014, bulk88 wrote:
Forgot pic. -- |
From @bulk88[fork test 6 segv gv struct.PNG](https://rt-archive.perl.org/perl5/Ticket/Attachment/1320853/703638/fork test 6 segv gv struct.PNG) |
From @bulk88On Sun Nov 30 23:17:37 2014, bulk88 wrote:
The GP was freed by a SAVEt_GP_ALIASED_SV. perl521.dll!Perl_save_aliased_sv(interpreter * my_perl=0x00365d74, gv * gv=0x00368d14) Line 729 C pp_enteriter is at http://perl5.git.perl.org/perl.git/blob/30c67ec60146a2650dd27e865da8a2e7b216b925:/pp_ctl.c#l2121 but I noticed something strange. "Skip no-common-vars optimisation for aliases" http://perl5.git.perl.org/perl.git/commitdiff/ff2a62e0c8bedade55bf86a22861a2fe06bb0a16 which is an FC commit @@ -13896,6 +13912,11 @@ Perl_ss_dup(pTHX_ PerlInterpreter *proto_perl, CLONE_PARAMS* param) /* hopefully this is only called on local symbol table entries */ GP* void What is the purpose of GpREFCNT_inc then? (GpREFCNT_inc blamed to http://perl5.git.perl.org/perl.git/commitdiff/1d7c184104c076988718a01b77c8706aae05b092#patch2 comment "/* XXX Remove this so it doesn't have to go thru the macro and return for nothing */" to http://perl5.git.perl.org/perl.git/commitdiff/d4c19fe8d8a6e04364af0548bf783e83ab5987d2 with no background offered) "Make list assignment respect foreach aliasing" http://perl5.git.perl.org/perl.git/commitdiff/c997e36218768fb357b1f3d160131f259311e3a3 -- |
From @cpansproutOn Mon Dec 01 00:08:03 2014, bulk88 wrote:
I was not aware of GpREFCNT_inc (didn’t notice it). It this particular case, it would add a redundant null check. The gp is always non-null for SAVEt_GP_ALIASED_SV. I don‘t know about the cases that use GpREFCNT_inc. Now that your patch from #123339 is applied, does my patch work? -- Father Chrysostomos |
From @bulk88On Mon Dec 01 21:42:11 2014, sprout wrote:
Yes please apply and backport to 5.20 and 5.18. Im not sure what wording you are planning for the perldelta. On Win32 Unwinding [or Restoring?] in a child psudo-process a variable that was C<local()>ed in a parent psudo-process before the C<fork> happened [executed?], caused memory corruption and a crash in the child psudo-process (and therefore OS process). Fixing this means the only 2 major bugs left in psuedo-fork are you can't fork() in a BEGIN (I think) and no IO calls on Win32 from CLONE method. This local() bug is years overdue from being fixed and pretty bad bug, since local() is a basic part of Perl 5 lang. -- |
From @cpansproutOn Tue Dec 02 18:53:41 2014, bulk88 wrote:
Thank you. I have applied it as 83a9455. I do think this should be backported. -- Father Chrysostomos |
@steve-m-hay - Status changed from 'open' to 'pending release' |
From @exodistIs this fix something that can possibly be wrapped in an XS module for even I am not super familiar with XS or patching, so I will accept "That is not -Chad On Wed, Dec 3, 2014 at 12:43 PM, Father Chrysostomos via RT <
|
From @cpansproutOn Thu Dec 11 08:10:45 2014, exodist7@gmail.com wrote:
That is nearly impossible. :-) I think you would have to copy and paste most of the thread-cloning code into the XS module, and then you would have to maintain multiple versions of it. -- Father Chrysostomos |
From @bulk88On Thu Dec 11 09:11:57 2014, sprout wrote:
Yes you will have to copy-paste ss_dup. You will have to maintain different versions of it (the save stack types change over time). Patch it isn't the hardest thing to do on Win32 from XS. Look in export table(GetProcAddress) of perl5**.dll for Perl_ss_dup (Why on earth its exported who knows? its probably a bad decision in the 1st release of embed.fnc, or a very smart decision), make the page "rw-", put in a jump x86 op to the new function in the XS DLL, make page "r-x", release control back to perl from your BEGIN block. How badly do you want it? -- |
From @bulk88On Thu Dec 11 10:58:35 2014, exodist7@gmail.com wrote:
I could try to write such a thing, I just need to know there will be a serious user base for the module since I'll never be writing that "use Win32::FixMyForkCrash;" line for any of my private code since I dont use Win32 psuedofork and never ran into the bug myself. -- |
From @exodisthttps://github.com/Test-More/test-more/blob/master/lib/Test/Stream/Util.pm Mainly I am looking for something I can do with the 3 functions that use On Thu, Dec 11, 2014 at 11:06 AM, bulk88 via RT <perlbug-followup@perl.org>
|
From @exodist
Pretty much anyone who uses Test-Simple on windows would be a user of this. -Chad |
From @exodist
Pretty much anyone who uses Test-Simple on windows would be a user of |
From @exodistThe most ideal solution would be something that uses only core modules I |
From @bulk88On Thu Dec 11 11:16:00 2014, exodist7@gmail.com wrote:
Why doesn't my $dolocal; work ? On Thu Dec 11 11:17:36 2014, exodist7@gmail.com wrote:
My worry is, the rest of CPAN authors put in "skip if Win32" from "5.8.0 2002-Jul-18" to Dec 2014, 12 years [1], if not counting from the bug report filing date which was 8 years ago. Half the code that put in "skip if Win32" is dead/abandoned/minimally maintained, and *nobody* except you will revisit any "skip if Win32" statements or add "use Win32::FixMyForkCrash;" unless I (bulk88) personally submit patches. Chicken and the egg. [1] the real time is longer than 12 years, probably 13 years, since until 5.22.0 and 5.21.2 are released, this bug is "open" and fork is "broken". -- |
From @exodistI could give something like that a try. On Thu, Dec 11, 2014 at 11:40 AM, bulk88 via RT <perlbug-followup@perl.org>
|
From @exodistIs the bug significant enough to release a new 5.8, 5.10, 5.12, 5.14, 5.16, On Thu, Dec 11, 2014 at 11:43 AM, Chad Granum <exodist7@gmail.com> wrote:
|
From @exodistOk, code similar to what bulk88 suggested seems to fix a majority of my On Thu, Dec 11, 2014 at 11:59 AM, Chad Granum <exodist7@gmail.com> wrote:
|
From @cpansproutOn Thu Dec 11 11:59:44 2014, exodist7@gmail.com wrote:
5.18.5, probably yes. The others, probably no. The policy (which is mostly a guideline) is that we only release security fixes for earlier versions. It’s mainly because that’s a lot of work and you’ll have trouble finding enough people to volunteer for it. -- Father Chrysostomos |
From @cpansproutOn Thu Dec 11 12:45:55 2014, exodist7@gmail.com wrote:
BTW, if you need the manual localisation unwound for every type of scope exit (e.g., goto, next), then a destructor-calling-a-closure may also work, as in End.pm and B::Hooks::EndOfScope (which are so trivial you could inline them with just 2-3 lines of code). -- Father Chrysostomos |
From @exodistAh, yeah, I have done that before (in fact there is a similar one in On Thu, Dec 11, 2014 at 2:46 PM, Father Chrysostomos via RT <
|
From @exodistOn that note someone mentioned to me that all it takes is someone with a Even then though, chances of everyone upgrading to a new 5.8 are pretty -Chad On Fri, Dec 12, 2014 at 10:51 AM, Chad Granum <exodist7@gmail.com> wrote:
|
From @rjbs* Chad Granum <exodist7@gmail.com> [2014-12-12T13:54:13]
We can put the a patch in the repository on a maint-5.8 branch, perhaps, but -- |
From editor.buzzfeed@yahoo.comWith real forks, this works just fine, but on Windows, I get the following output: | C:\Share>perl bug.pl Similar testing reveals that the bug does not affect array, hash, nor glob values. (It does of course affect array and hash elements.) Thanks, |
From [Unknown Contact. See original ticket]With real forks, this works just fine, but on Windows, I get the following | C:\Share>perl bug.pl Similar testing reveals that the bug does not affect array, hash, nor glob Thanks, |
From arisadam74@gmail.comI think the issue is not so much Perl_save_scalar incrementing the ref count of the scalar , as of S_save_scalar_at() failing to decrement the refcount of the SV when it removes it from the GvSV slot. I suspect (although this is based on a cursory look at the issue) that Thanks ! |
From johnashley1066@gmail.comOn Wed Apr 08 04:55:58 2015, arisadam74@gmail.com wrote:
|
From [Unknown Contact. See original ticket]On Wed Apr 08 04:55:58 2015, arisadam74@gmail.com wrote:
|
From @khwilliamsonThanks for submitting this ticket The issue should be resolved with the release today of Perl v5.22. If you find that the problem persists, feel free to reopen this ticket -- |
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#40565 (status was 'resolved')
Searchable as RT40565$
The text was updated successfully, but these errors were encountered: