Args to 'system' not evaluated before forking, making $$ wrong #14489
Comments
From @jimavThis is a bug report for perl from jim.avera@gmail.com, The original problem which led me to this was: system "ps","-p",$$,"-o","pid,etime,bsdtime,c,rss"; # always shows wrong values The problem appears to be that $$ is not correct when passed to system(). Can 'system' (and any other forking operators) be made to #!/usr/bin/perl my system "/bin/echo", 'echo: $$ =', $$; Flags: Site configuration information for perl 5.20.1: Configured by Debian Project at Fri Oct 10 14:16:26 UTC 2014. Summary of my perl5 (revision 5 version 20 subversion 1) configuration: Locally applied patches: @INC for perl 5.20.1: Environment for perl 5.20.1: |
From @iabynOn Mon, Feb 09, 2015 at 01:56:59PM -0800, via RT wrote:
That will also affect things like tied vars, which are currently also sub TIESCALAR { bless [] } which outputs: parent: pid=4773 I suspect that evaluating in the parent is The Right Thing to Do, but -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @karenetheridgeOn Wed Feb 11 03:36:20 2015, davem wrote:
Agreed on both points. If the status quo is kept (for now or forever), a documentation note should be added for any variable whose value will differ between processes (I can't think of any besides $$, but maybe some whose lexical scope is significant?) |
From @ikegamiThe workaround is to replace system(..., $$, ...) with system(..., "$$", ...) |
From @jimav
|
From @rjbs* Dave Mitchell <davem@iabyn.com> [2015-02-11T06:28:48]
Same here. We could try making this change in 5.23, but my further worry is -- |
From chwagner@gmti.gannett.comThis is a confirmed regression introduced with making $$ magic in 5.16. This could also affect any other command that interprets its arguments post fork(). The cheapest fix is probably to make all fork()'ing statements copy the argument list before the fork(). $ perl -le 'print "$^V |
From @LeontOn Mon, Feb 9, 2015 at 10:56 PM, via RT <perlbug-followup@perl.org> wrote:
Fix attached. Leon |
From @Leont0001-Evaluate-get-magic-in-system-and-openpipe-before-the.patchFrom 14e44794d6597318abed388a8614f5f28eb91389 Mon Sep 17 00:00:00 2001
From: Leon Timmermans <fawaka@gmail.com>
Date: Sat, 14 Feb 2015 00:37:18 +0100
Subject: [PATCH] Evaluate get magic in system and openpipe before the fork
---
doio.c | 2 +-
mathoms.c | 4 ++++
pp_sys.c | 6 ++++++
t/op/exec.t | 20 +++++++++++++++++++-
util.c | 7 +++++++
5 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/doio.c b/doio.c
index a63f2a2..dc4de86 100644
--- a/doio.c
+++ b/doio.c
@@ -1544,7 +1544,7 @@ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
while (++mark <= sp) {
if (*mark)
- *a++ = SvPV_nolen_const(*mark);
+ *a++ = SvPV_nomg_nolen(*mark);
else
*a++ = "";
}
diff --git a/mathoms.c b/mathoms.c
index 2a65fb4..6ad8eb3 100644
--- a/mathoms.c
+++ b/mathoms.c
@@ -713,8 +713,12 @@ Perl_do_binmode(pTHX_ PerlIO *fp, int iotype, int mode)
bool
Perl_do_aexec(pTHX_ SV *really, SV **mark, SV **sp)
{
+ SV** current;
PERL_ARGS_ASSERT_DO_AEXEC;
+ for (current = mark + 1; current <= SP; current++) {
+ SvGETMAGIC(*current);
+ }
return do_aexec5(really, mark, sp, 0, 0);
}
#endif
diff --git a/pp_sys.c b/pp_sys.c
index e2f8edf..7e4ff61 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -4259,6 +4259,12 @@ PP(pp_system)
MARK = ORIGMARK;
TAINT_PROPER("system");
}
+ else {
+ while (++MARK <= SP) {
+ (void)SvGETMAGIC(*MARK); /* For get magic */
+ }
+ MARK = ORIGMARK;
+ }
PERL_FLUSHALL_FOR_CHILD;
#if (defined(HAS_FORK) || defined(AMIGAOS)) && !defined(VMS) && !defined(OS2) || defined(PERL_MICRO)
{
diff --git a/t/op/exec.t b/t/op/exec.t
index 6ec3646..e7ef923 100644
--- a/t/op/exec.t
+++ b/t/op/exec.t
@@ -36,7 +36,7 @@ $ENV{LANGUAGE} = 'C'; # Ditto in GNU.
my $Is_VMS = $^O eq 'VMS';
my $Is_Win32 = $^O eq 'MSWin32';
-plan(tests => 24);
+plan(tests => 26);
my $Perl = which_perl();
@@ -153,6 +153,24 @@ TODO: {
"exec failure doesn't terminate process");
}
+SKIP: {
+ skip 'Doesn\'t work on Windows/VMS', 2 if $Is_VMS || $Is_Win32;
+ open my $fh, '-|', 'echo', $$;
+ my $pid = <$fh>;
+ chomp $pid;
+ is($pid, $$, 'Pid is as expected in openpipe');
+
+ skip 'Can\'t load POSIX' if not eval { require POSIX };
+ if (my $child = open my $fh, '-|') {
+ my $pid = <$fh>;
+ chomp $pid;
+ is($pid, $child, 'Pid is as expected in system');
+ } else {
+ system 'echo', $$;
+ POSIX::_exit(0);
+ }
+}
+
my $test = curr_test();
exec $Perl, '-le', qq{${quote}print 'ok $test - exec PROG, LIST'${quote}};
fail("This should never be reached if the exec() worked");
diff --git a/util.c b/util.c
index 9ffdbde..ebd3ab4 100644
--- a/util.c
+++ b/util.c
@@ -2349,6 +2349,13 @@ Perl_my_popen_list(pTHX_ const char *mode, int n, SV **args)
/* Try for another pipe pair for error return */
if (PerlProc_pipe(pp) >= 0)
did_pipes = 1;
+
+ {
+ SV** current;
+ for (current = args; current <= args-1+n; current++) {
+ SvGETMAGIC(*current);
+ }
+ }
while ((pid = PerlProc_fork()) < 0) {
if (errno != EAGAIN) {
PerlLIO_close(p[This]);
--
2.2.0-369-g3b010e3
|
From @jkeenanOn Fri Feb 13 15:40:38 2015, LeonT wrote:
In the parts of the patch where you are adding lines to source code or tests, could you use space characters rather than hard-tabs for leading whitespace, so as to make the appearance tidier? Thank you very much. -- |
From @LeontOn Wed, Feb 11, 2015 at 12:28 PM, Dave Mitchell <davem@iabyn.com> wrote:
The issue didn't exist until 5.16 Leon |
From @LeontOn Sat, Feb 14, 2015 at 1:02 AM, James E Keenan via RT <
Perl's source consistently uses a convention of tabstop=8 and Leon |
From @khwilliamsonOn 02/13/2015 05:15 PM, Leon Timmermans wrote:
That is out-of-date. All new code should use spaces exclusively. |
From @LeontOn Sat, Feb 14, 2015 at 1:32 AM, Karl Williamson <public@khwilliamson.com>
In that case, updated. Leon |
From @Leont0001-Evaluate-get-magic-in-system-and-openpipe-before-the.patchFrom 14e44794d6597318abed388a8614f5f28eb91389 Mon Sep 17 00:00:00 2001
From: Leon Timmermans <fawaka@gmail.com>
Date: Sat, 14 Feb 2015 00:37:18 +0100
Subject: [PATCH] Evaluate get magic in system and openpipe before the fork
---
doio.c | 2 +-
mathoms.c | 4 ++++
pp_sys.c | 6 ++++++
t/op/exec.t | 20 +++++++++++++++++++-
util.c | 7 +++++++
5 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/doio.c b/doio.c
index a63f2a2..dc4de86 100644
--- a/doio.c
+++ b/doio.c
@@ -1544,7 +1544,7 @@ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
while (++mark <= sp) {
if (*mark)
- *a++ = SvPV_nolen_const(*mark);
+ *a++ = SvPV_nomg_nolen(*mark);
else
*a++ = "";
}
diff --git a/mathoms.c b/mathoms.c
index 2a65fb4..6ad8eb3 100644
--- a/mathoms.c
+++ b/mathoms.c
@@ -713,8 +713,12 @@ Perl_do_binmode(pTHX_ PerlIO *fp, int iotype, int mode)
bool
Perl_do_aexec(pTHX_ SV *really, SV **mark, SV **sp)
{
+ SV** current;
PERL_ARGS_ASSERT_DO_AEXEC;
+ for (current = mark + 1; current <= SP; current++) {
+ SvGETMAGIC(*current);
+ }
return do_aexec5(really, mark, sp, 0, 0);
}
#endif
diff --git a/pp_sys.c b/pp_sys.c
index e2f8edf..7e4ff61 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -4259,6 +4259,12 @@ PP(pp_system)
MARK = ORIGMARK;
TAINT_PROPER("system");
}
+ else {
+ while (++MARK <= SP) {
+ (void)SvGETMAGIC(*MARK); /* For get magic */
+ }
+ MARK = ORIGMARK;
+ }
PERL_FLUSHALL_FOR_CHILD;
#if (defined(HAS_FORK) || defined(AMIGAOS)) && !defined(VMS) && !defined(OS2) || defined(PERL_MICRO)
{
diff --git a/t/op/exec.t b/t/op/exec.t
index 6ec3646..e7ef923 100644
--- a/t/op/exec.t
+++ b/t/op/exec.t
@@ -36,7 +36,7 @@ $ENV{LANGUAGE} = 'C'; # Ditto in GNU.
my $Is_VMS = $^O eq 'VMS';
my $Is_Win32 = $^O eq 'MSWin32';
-plan(tests => 24);
+plan(tests => 26);
my $Perl = which_perl();
@@ -153,6 +153,24 @@ TODO: {
"exec failure doesn't terminate process");
}
+SKIP: {
+ skip 'Doesn\'t work on Windows/VMS', 2 if $Is_VMS || $Is_Win32;
+ open my $fh, '-|', 'echo', $$;
+ my $pid = <$fh>;
+ chomp $pid;
+ is($pid, $$, 'Pid is as expected in openpipe');
+
+ skip 'Can\'t load POSIX' if not eval { require POSIX };
+ if (my $child = open my $fh, '-|') {
+ my $pid = <$fh>;
+ chomp $pid;
+ is($pid, $child, 'Pid is as expected in system');
+ } else {
+ system 'echo', $$;
+ POSIX::_exit(0);
+ }
+}
+
my $test = curr_test();
exec $Perl, '-le', qq{${quote}print 'ok $test - exec PROG, LIST'${quote}};
fail("This should never be reached if the exec() worked");
diff --git a/util.c b/util.c
index 9ffdbde..ebd3ab4 100644
--- a/util.c
+++ b/util.c
@@ -2349,6 +2349,13 @@ Perl_my_popen_list(pTHX_ const char *mode, int n, SV **args)
/* Try for another pipe pair for error return */
if (PerlProc_pipe(pp) >= 0)
did_pipes = 1;
+
+ {
+ SV** current;
+ for (current = args; current <= args-1+n; current++) {
+ SvGETMAGIC(*current);
+ }
+ }
while ((pid = PerlProc_fork()) < 0) {
if (errno != EAGAIN) {
PerlLIO_close(p[This]);
--
2.2.0-369-g3b010e3
|
From @jkeenanOn Fri Feb 13 16:46:53 2015, LeonT wrote:
Thank you very much. Now, can we get comments on the *content* of Leon's patch (which is beyond my skill set to evaluate)? -- |
From wagnerc@plebeian.comI would like to lobby for evaluating all of the arguments that are not literal strings before the fork. True, $PID is the scenario at hand but there could be any number of cases down the road. The other obvious example is anything tie()'d. Spooky action at a distance is bad and I think the expected behavior of anything passed to system(), etc, is to be the value of the expression at that line of the program. I can think of one secenario where a tie variable returns memory information about the current process. Definately do not want that in the forked copy. Forgive me if I didn't see that in the proposed patch. For the test case, I have a better suggestion. It will work on any system. <code> |
From @demerphqOn 14 February 2015 at 08:55, James E Keenan via RT
He is just magic sure we call GETMAGIC on the arguments, which is Yves -- |
From @leonerdOn Fri, 13 Feb 2015 16:55:04 -0800
LGTM -- leonerd@leonerd.org.uk |
From @LeontOn Sat, Feb 14, 2015 at 5:47 AM, Chris Wagner via RT <
That is already what the patch does.
That test was about testing the list form of openpipe, it has the same I was probably too careful, I suspect my version also works on Windows when
I would still expect that to run into pseudothreads on Windows, but I Leon |
From @Leont0002-Simplify-system-tests.patchFrom c2d16f4e6465f8bfd392400a80c816ac90dc1dfe Mon Sep 17 00:00:00 2001
From: Leon Timmermans <fawaka@gmail.com>
Date: Sat, 14 Feb 2015 15:43:07 +0100
Subject: [PATCH 2/2] Simplify system $$ tests
---
t/op/exec.t | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/t/op/exec.t b/t/op/exec.t
index d5ec32e..f26b8fd 100644
--- a/t/op/exec.t
+++ b/t/op/exec.t
@@ -153,22 +153,14 @@ TODO: {
"exec failure doesn't terminate process");
}
-SKIP: {
- skip 'Doesn\'t work on Windows/VMS', 2 if $Is_VMS || $Is_Win32;
- open my $fh, '-|', 'echo', $$;
+{
+ my @echo = $Is_VMS ? ($Perl, '-le', 'print $ARGV[0]') : 'echo';
+ open my $fh, '-|', @echo, $$;
my $pid = <$fh>;
chomp $pid;
is($pid, $$, 'Pid is as expected in openpipe');
- skip 'Can\'t load POSIX' if not eval { require POSIX };
- if (my $child = open my $fh, '-|') {
- my $pid = <$fh>;
- chomp $pid;
- is($pid, $child, 'Pid is as expected in system');
- } else {
- system 'echo', $$;
- POSIX::_exit(0);
- }
+ ok(!system($Perl, '-e', 'exit ($ARGV[0] eq $$)', $$), 'Pid is as expected in system');
}
my $test = curr_test();
--
2.2.0-369-g3b010e3
|
From chwagner@gmti.gannett.comOn Sat Feb 14 17:10:11 2015, LeonT wrote:
The problem on Windows is likely masked by the pseudo-fork. It prints the "right pid". I verified that two Perl processes get made with the right pids in TaskInfo. C:\>perl -le "print qq/$^V\nparent pid $$/; system qw/perl -le/, q/print qq{passed pid This can be elucidated with the following fork example. This pid is negative and there is only ever one Perl process in TaskInfo. So I would say that any improper symbol evaluation is taking place in the 2nd thread. This could still be a problem depending on what is behind a tie()'d variable. I've come up with this command line that could serve as a universal test case for system() atleast. Change the quotation marks for Unix of course. Does GETMAGIC cause FETCH to be executed on tie()'d variables? Thanks. |
From @tonycozOn Mon, Feb 16, 2015 at 09:23:51AM -0800, Chris Wagner via RT wrote:
system() on Win32 doesn't use fork() of any sort. Tony |
From @LeontOn Mon, Feb 16, 2015 at 6:23 PM, Chris Wagner via RT <
That is exactly what I'd expect.
I'm not completely sure I understand. Are you saying my second patch isn't
Yes. Leon |
From chwagner@gmti.gannett.comOn Tue Feb 17 08:28:33 2015, LeonT wrote:
What I was mainly talking about was the Windows command prompt. Apostrophe quoting doesn't work. It must be quotation marks. Nothing to do with the t file test case. Tony indicated that system() does not use the fork() mechanism on Windows. So that might need a completely seperate patch. Or it could be unaffected. |
From @avarOn Sat, Feb 14, 2015 at 1:03 AM, Leon Timmermans <fawaka@gmail.com> wrote:
That commit was followed-up by my 985213f, which in theory has the |
From @tonycozOn Fri Feb 13 16:46:53 2015, LeonT wrote:
Inline Patchdiff --git a/doio.c b/doio.c
index a63f2a2..dc4de86 100644
--- a/doio.c
+++ b/doio.c
@@ -1544,7 +1544,7 @@ Perl_do_aexec5(pTHX_ SV *really, SV **mark, SV **sp,
- *a++ = SvPV_nolen_const(*mark);
+ *a++ = SvPV_nomg_nolen(*mark);
diff --git a/mathoms.c b/mathoms.c
index 2a65fb4..6ad8eb3 100644
--- a/mathoms.c
+++ b/mathoms.c
@@ -713,8 +713,12 @@ Perl_do_binmode(pTHX_ PerlIO *fp, int iotype, int mode)
+ for (current = mark + 1; current <= SP; current++) {
+ SvGETMAGIC(*current);
+ }
diff --git a/pp_sys.c b/pp_sys.c
index e2f8edf..7e4ff61 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -4259,6 +4259,12 @@ PP(pp_system)
+ else {
+ while (++MARK <= SP) {
+ (void)SvGETMAGIC(*MARK); /* For get magic */
+ }
+ MARK = ORIGMARK;
+ }
Around Tue Feb 17 10:57:01 2015 Chris Wagner said:
system() on Win32 evaluates all of the arguments in the parent process and The same is true of the newer list form pipe open implementation. If you're seeing this problem on Windows, there's something unrelated going on. Around Sat Feb 14 17:10:11 2015 Leon Timmermans said:
Windows doesn't handle "-|" with no arguments as a fork (it attempts to |
From @LeontOn Wed, Feb 18, 2015 at 4:46 AM, Tony Cook via RT <perlbug-followup@perl.org
Good point. I suspect the clean way out of this is to rethink do_aexec5 to Leon |
From chrjae@gmail.comThis issue is also being discussed on the london.pm mailing list: https://groups.perlists.pm/sympa/arc/london.pm/2015-10/msg00003.html |
From chrjae@gmail.comThis issue is also being discussed on the london.pm mailing list: https://groups.perlists.pm/sympa/arc/london.pm/2015-10/msg00003.html |
From @wolfsageThis looks like a duplicate of Thanks, -- Matthew Horsfall (alh) |
|
This appears to have been fixed in recent perl versions, though not intentionally. |
How would I know that it has been fixed? |
|
I wrote this tester:
print "PARENT PID: $$\n";
system('echo', "CHILD GOT:", $$);Simcop on #perl ran it across multiple perl versions: As can be seen in that paste, it broke around 5.16, then was fixed around 5.28.2 |
jkeenan
added
the
Closable?
Bisection points to this commit: Which suggests perhaps it was the same problem as in #13561. Is this ticket now closable? Thank you very much. |
|
It does appear that commit thoroughly fixed the issue, it even fixes stringification overloads in the argument list: https://perl.bot/p/fxsfj4 |
jkeenan
removed
the
Closable?
|
This should be added to perldeltas -- both the pod/perlXXXXdelta.pod that corresponds to the version the fix was in, and also pod/perldelta.pod (for the next blead-point release) in the "Errata From Previous Releases" section. |
|
This is briefly mentioned in https://perldoc.pl/perl5280delta#Selected-Bug-Fixes:
|
Migrated from rt.perl.org#123775 (status was 'open')
Searchable as RT123775$
The text was updated successfully, but these errors were encountered: