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

IPC::Open3 directs output incorrectly when STDOUT not connected to fd#1 #9759

Open
p5pRT opened this issue Jun 3, 2009 · 21 comments
Open

IPC::Open3 directs output incorrectly when STDOUT not connected to fd#1 #9759

p5pRT opened this issue Jun 3, 2009 · 21 comments

Comments

@p5pRT
Copy link

p5pRT commented Jun 3, 2009

Migrated from rt.perl.org#66224 (status was 'open')

Searchable as RT66224$

@p5pRT
Copy link
Author

p5pRT commented Jun 3, 2009

From @scottgifford

This is a bug report for perl from sgifford@​suspectclass.com,
generated with the help of perlbug 1.35 running under perl v5.8.8.

IPC​::Open3​::open3 directs output incorrectly if STDOUT is set to a
filehandle with a descriptor number other than 1. This appears to
happen under mod_perl 2 (at least in my configuration).

Here is a script that demonstrates the problem​:

===8<===8<===8<===8<===
#!/usr/bin/perl

use warnings;
use strict;

use IPC​::Open3;

# Basic setup stuff
open(SAVE_STDOUT,">&",STDOUT) or die "save stdout failed";
open(DEVNULL,"< /dev/null") or die "open /dev/null failed";

# Now set STDOUT to a filehandle on a new descriptor
open(FH1,"> /dev/null") or die "open /dev/null failed";
*STDOUT = *FH1;

# Run the command
open3(*DEVNULL, *PIPE, undef, "/bin/sh","-c",<<EOF) or die "open3 failed";
printf "out 1\\nout 2\\nout 3\\n"
printf "err 1\\nerr 2\\nerr 3\\n" >&2
EOF
;

# And copy the output
open(STDOUT,">&",*SAVE_STDOUT) or die "restore stout failed";
while (<PIPE>) {
  print "PIPE​: ",$_;
}
close(PIPE) or die "close pipe failed";

exit(0);
===>8===>8===>8===>8===

The expected output from this is​:

  PIPE​: out 1
  PIPE​: out 2
  PIPE​: out 3
  PIPE​: err 1
  PIPE​: err 2
  PIPE​: err 3

which indicates that the child process had both standard output and
standard error sent to the pipe created by open3.

Instead, with both IPC​::Open3 1.02 and 1.03, I get this output​:

  out 1
  out 2
  out 3
  PIPE​: err 1
  PIPE​: err 2
  PIPE​: err 3

This indicates that the standard output from the script didn't go
through the pipe. Instead it went directly to my terminal, where
file descriptor #1 is connected to in the parent process.

If you look around line 246 of Open3.pm, you'll see this code​:

  if ($dup_rdr) {
  xopen \*STDOUT, ">&$dad_rdr" if fileno(STDOUT) != xfileno($dad_rdr);
  } else {
  xclose $dad_rdr;
  xopen \*STDOUT, ">&=" . fileno $kid_wtr;
  }

That will ensure that Perl's STDOUT filehandle is connected to the
right filehandle, but what the child script cares about is what's
connected to file descriptor #1, and that's not necessarily correct.

This patch to IPC/Open3.pm seems to fix the problem​:

===8<===8<===8<===8<===

Inline Patch
--- /usr/share/perl/5.8.8/IPC/Open3.pm	2009-01-14 17:58:13.000000000 -0500
+++ fixed/IPC/Open3.pm	2009-06-03 01:10:14.000000000 -0400
@@ -3,6 +3,7 @@
 use strict;
 no strict 'refs'; # because users pass me bareword filehandles
 our ($VERSION, @ISA, @EXPORT);
+use POSIX;
 
 require Exporter;
 
@@ -134,30 +135,33 @@
 	}
 
 	if ($dup_wtr) {
-	    xopen \*STDIN,  "<&$dad_wtr" if fileno(STDIN) != xfileno($dad_wtr);
+  	    POSIX::dup2(xfileno($dad_wtr), 0) if 0 != xfileno($dad_wtr);
 	} else {
 	    xclose $dad_wtr;
-	    xopen \*STDIN,  "<&=" . fileno $kid_rdr;
+	    POSIX::dup2(fileno($kid_rdr), 0);
 	}
+
 	if ($dup_rdr) {
-	    xopen \*STDOUT, ">&$dad_rdr" if fileno(STDOUT) != xfileno($dad_rdr);
+  	    POSIX::dup2(xfileno($dad_rdr), 1) if 1 != xfileno($dad_rdr);
 	} else {
 	    xclose $dad_rdr;
-	    xopen \*STDOUT, ">&=" . fileno $kid_wtr;
+	    POSIX::dup2(fileno($kid_wtr), 1);
 	}
 	if ($dad_rdr ne $dad_err) {
 	    if ($dup_err) {
 		# I have to use a fileno here because in this one case
 		# I'm doing a dup but the filehandle might be a reference
 		# (from the special case above).
-		xopen \*STDERR, ">&" . xfileno($dad_err)
-		    if fileno(STDERR) != xfileno($dad_err);
+		POSIX::dup2(xfileno($dad_err), 2)
+		    if 2 != xfileno($dad_err);
 	    } else {
 		xclose $dad_err;
-		xopen \*STDERR, ">&=" . fileno $kid_err;
+		POSIX::dup2(fileno($kid_err), 2);
 	    }
 	} else {
-	    xopen \*STDERR, ">&STDOUT" if fileno(STDERR) != fileno(STDOUT);
+            if (fileno(STDERR) != fileno(STDOUT)) {
+		POSIX::dup2(1, 2);
+            }
 	}
 	if ($cmd[0] eq '-') {
 	    croak "Arguments don't make sense when the command is '-'"
===>8===>8===>8===>8===

Since POSIX​::dup2 works explicitly with file descriptors, this will
ensure the right file handles end up connected to the right file
descriptors in the child process.

With this patch, the above test program works properly for me.


Flags​:
  category=core
  severity=medium


Site configuration information for perl v5.8.8​:

Configured by Debian Project at Wed Jan 14 22​:46​:08 UTC 2009.

Summary of my perl5 (revision 5 version 8 subversion 8) configuration​:
  Platform​:
  osname=linux, osvers=2.6.24-19-server, archname=i486-linux-gnu-thread-multi
  uname='linux palmer 2.6.24-19-server #1 smp sat jul 12 00​:40​:01 utc 2008 i686 gnulinux '
  config_args='-Dusethreads -Duselargefiles -Dccflags=-DDEBIAN -Dcccdlflags=-fPIC -Darchname=i486-linux-gnu -Dprefix=/usr -Dprivlib=/usr/share/perl/5.8 -Darchlib=/usr/lib/perl/5.8 -Dvendorprefix=/usr -Dvendorlib=/usr/share/perl5 -Dvendorarch=/usr/lib/perl5 -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl/5.8.8 -Dsitearch=/usr/local/lib/perl/5.8.8 -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Dman1ext=1 -Dman3ext=3perl -Dpager=/usr/bin/sensible-pager -Uafs -Ud_csh -Ud_ualarm -Uusesfio -Uusenm -Duseshrplib -Dlibperl=libperl.so.5.8.8 -Dd_dosuid -des'
  hint=recommended, useposix=true, d_sigaction=define
  usethreads=define use5005threads=undef useithreads=define usemultiplicity=define
  useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
  use64bitint=undef use64bitall=undef uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBIAN -fno-strict-aliasing -pipe -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
  optimize='-O2',
  cppflags='-D_REENTRANT -D_GNU_SOURCE -DTHREADS_HAVE_PIDS -DDEBIAN -fno-strict-aliasing -pipe -I/usr/local/include'
  ccversion='', gccversion='4.2.3 (Ubuntu 4.2.3-2ubuntu7)', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
  ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
  alignbytes=4, prototype=define
  Linker and Libraries​:
  ld='cc', ldflags =' -L/usr/local/lib'
  libpth=/usr/local/lib /lib /usr/lib
  libs=-lgdbm -lgdbm_compat -ldb -ldl -lm -lpthread -lc -lcrypt
  perllibs=-ldl -lm -lpthread -lc -lcrypt
  libc=/lib/libc-2.7.so, so=so, useshrplib=true, libperl=libperl.so.5.8.8
  gnulibc_version='2.7'
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
  cccdlflags='-fPIC', lddlflags='-shared -L/usr/local/lib'

Locally applied patches​:
 


@​INC for perl v5.8.8​:
  /etc/perl
  /usr/local/lib/perl/5.8.8
  /usr/local/share/perl/5.8.8
  /usr/lib/perl5
  /usr/share/perl5
  /usr/lib/perl/5.8
  /usr/share/perl/5.8
  /usr/local/lib/site_perl
  .


Environment for perl v5.8.8​:
  HOME=/home/sgifford
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/usr/local/sbin​:/usr/local/bin​:/usr/sbin​:/usr/bin​:/sbin​:/bin​:/usr/games
  PERL_BADLANG (unset)
  SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Jun 3, 2009

@scottgifford - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Jun 5, 2009

From @ikegami

Has this been tested on Windows? fd 0,1,2 as stdin/out/err and dup2 are
unixy concepts, and I don't know how well they are emulated.

And there's the nit that xopen performs error checking but your
replacement doesn't.

@p5pRT
Copy link
Author

p5pRT commented Jun 5, 2009

From @scottgifford

On Fri Jun 05 10​:11​:48 2009, ikegami@​adaelis.com wrote​:

Has this been tested on Windows? fd 0,1,2 as stdin/out/err and dup2 are
unixy concepts, and I don't know how well they are emulated.

No. If the general thrust of my solution seems right, I'll try it out on
Windows and figure out what to do there. I don't have much expertise in
Perl on Windows, but I'll see what I can do.

And there's the nit that xopen performs error checking but your
replacement doesn't.

Yeah, good point, if everything else looks OK I can fix that.

-----Scott.

@p5pRT
Copy link
Author

p5pRT commented Jun 5, 2009

From [Unknown Contact. See original ticket]

On Fri Jun 05 10​:11​:48 2009, ikegami@​adaelis.com wrote​:

Has this been tested on Windows? fd 0,1,2 as stdin/out/err and dup2 are
unixy concepts, and I don't know how well they are emulated.

No. If the general thrust of my solution seems right, I'll try it out on
Windows and figure out what to do there. I don't have much expertise in
Perl on Windows, but I'll see what I can do.

And there's the nit that xopen performs error checking but your
replacement doesn't.

Yeah, good point, if everything else looks OK I can fix that.

-----Scott.

@p5pRT
Copy link
Author

p5pRT commented Jun 6, 2009

From @Corion

Hello,

I can confirm the problem for bleadperl on Windows. Unfortunately, if I
modify lib/IPC/Open3.pm as outlined in the ticket, the error stays, even
though Windows has the same concept of fd0,1,2 as Unix.

Please find attached the modified test file, which should work across
more platforms. I've exorcised the shell code and replaced it with Perl,
and also removed the hardcoded '/dev/null' in favor or File​::Spec->devnull.

-max

PS​: On a purely stylistic note, please do

use POSIX ()

to avoid importing all the symbols that POSIX.pm exports, especially
when it's only for a single function that you don't even want imported
into your namespace.

@p5pRT
Copy link
Author

p5pRT commented Jun 6, 2009

From @Corion

rt66224.pl

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2010

From roger.j.meier@gmx.ch

Had the same problem when using open3() to start a process from a
handler in HTTP​::Server​::Simple​::CGI.

The attached Open3.pm patch worked for me​: It uses dup2() implicitly by
opening ">&=1" and ">&=2" for STDOUT and STDERR inside the forked/cloned
process, in fact just after the xfork()​:

  open(STDOUT, ">&=1");
  open(STDERR, ">&=2");

Since it's used only within the forked process, resetting the original
file descriptor numbers seems not to be necessary. Error checking could
be an issue, but descriptors 1 and 2 should be always available, at
least on UNIX.

@p5pRT
Copy link
Author

p5pRT commented Apr 29, 2010

From roger.j.meier@gmx.ch

open3.pm.diff

@p5pRT
Copy link
Author

p5pRT commented Dec 18, 2010

From ambs@zbr.pt

Having this same problem on Mac OS X, perl 5.12.2, IPC​::Open3 v1.05,
when running open3 within Dancer.

As this ticked was opened about 1 year ago, I wonder what other
requester are using instead of IPC​::Open3.

Cheers
ambs

@p5pRT
Copy link
Author

p5pRT commented Dec 18, 2010

From ambs@zbr.pt

On Sat Dec 18 10​:53​:01 2010, ambs wrote​:

Having this same problem on Mac OS X, perl 5.12.2, IPC​::Open3 v1.05,
when running open3 within Dancer.

As this ticked was opened about 1 year ago, I wonder what other
requester are using instead of IPC​::Open3.

Patch by rogerjmeier (adding the two open >&=1 and >&=2) seems not to
work as expected here...

@p5pRT
Copy link
Author

p5pRT commented Dec 19, 2010

From ambs@zbr.pt

Probably this doesn't help much, but in attach a summarized version of
my strace on my Dancer + open3 app.

@p5pRT
Copy link
Author

p5pRT commented Dec 19, 2010

From ambs@zbr.pt

Open3 Starts​:

22220 close(4) = 0
22220 ioctl(8, SNDCTL_TMR_TIMEBASE or TCGETS, 0x7fffb4f6fc70) = -1 EINVAL (Invalid argument)
22220 lseek(8, 0, SEEK_CUR) = -1 ESPIPE (Illegal seek)
22220 fstat(8, {st_mode=S_IFIFO|0600, st_size=0, ...}) = 0
22219 read(10, <unfinished ...>
22220 fcntl(8, F_SETFD, FD_CLOEXEC) = 0
22220 close(10) = 0
22220 ioctl(11, SNDCTL_TMR_TIMEBASE or TCGETS, 0x7fffb4f6fc70) = -1 EINVAL (Invalid argument)
22220 lseek(11, 0, SEEK_CUR) = -1 ESPIPE (Illegal seek)
22220 fstat(11, {st_mode=S_IFIFO|0600, st_size=0, ...}) = 0
22220 fcntl(11, F_SETFD, FD_CLOEXEC) = 0
22220 close(12) = 0
22220 ioctl(13, SNDCTL_TMR_TIMEBASE or TCGETS, 0x7fffb4f6fc70) = -1 EINVAL (Invalid argument)
22220 lseek(13, 0, SEEK_CUR) = -1 ESPIPE (Illegal seek)
22220 fstat(13, {st_mode=S_IFIFO|0600, st_size=0, ...}) = 0
22220 dup2(13, 2) = 2
22220 dup(13) = 4
22220 fcntl(13, F_GETFD) = 0x1 (flags FD_CLOEXEC)
22220 dup2(4, 13) = 13
22220 fcntl(13, F_SETFD, FD_CLOEXEC) = 0
22220 close(4) = 0
22220 fcntl(2, F_SETFD, 0) = 0
22220 rt_sigaction(SIGFPE, {SIG_DFL, [], SA_RESTORER, 0x3699a0f440}, {SIG_IGN, [FPE], SA_RESTORER|SA_RESTART, 0x3699632a20}, 8) = 0
22220 execve("/usr/local/bin/cqp", ["/usr/local/bin/cqp", "-c"], [/* 54 vars */] <unfinished ...>
22219 <... read resumed> "", 4096) = 0

Here, FD 13 is STDERR
Probably given my lack of experience on these things, Can't find any redirection on STDOUT or STDIN

Meanwhile, the process (cqp) loads a set of libraries, and accesses a set of local files.

And it continues​:

22220 rt_sigaction(SIGPIPE, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART, 0x3699632a20}, {SIG_IGN, [], 0}, 8) = 0
22220 rt_sigaction(SIGPIPE, {SIG_DFL, [PIPE], SA_RESTORER|SA_RESTART, 0x3699632a20}, {SIG_IGN, [PIPE], SA_RESTORER|SA_RESTART, 0x3699632a20}, 8) = 0
22220 rt_sigaction(SIGINT, {0x4075b0, [INT], SA_RESTORER|SA_RESTART, 0x3699632a20}, {SIG_DFL, [], 0}, 8) = 0
22220 write(1, "CQP version 3.0.0", 17) = 17
22220 write(1, "\n", 1) = 1

That is, cqp wrote to stdout (1)

22220 ioctl(0, SNDCTL_TMR_TIMEBASE or TCGETS, {B9600 opost isig icanon echo ...}) = 0
22220 ioctl(0, SNDCTL_TMR_TIMEBASE or TCGETS, {B9600 opost isig icanon echo ...}) = 0
22220 fstat(0, {st_mode=S_IFCHR|0600, st_rdev=makedev(136, 2), ...}) = 022220 mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fad3d116000
22220 read(0, <unfinished ...>

And starts reading on 0... and ends when I kill the cqp process

22220 <... read resumed> 0x7fad3d116000, 1024) = ? ERESTARTSYS (To be restarted)
22220 --- SIGINT (Interrupt) @​ 0 (0) ---

@p5pRT
Copy link
Author

p5pRT commented Dec 19, 2010

From ambs@zbr.pt

Applying the patch from sgifford (first from the thread) things work.
Any change on giving some love to this bug?

@p5pRT
Copy link
Author

p5pRT commented Dec 19, 2010

From ambs@zbr.pt

Added a diff to add a test to this into HEAD@​git.

I'm happy to help solving this bug, accordingly with my own limitations.

@p5pRT
Copy link
Author

p5pRT commented Dec 19, 2010

From ambs@zbr.pt

ipc.diff

@p5pRT
Copy link
Author

p5pRT commented Dec 19, 2010

From ambs@zbr.pt

On Sun Dec 19 12​:54​:59 2010, ambs wrote​:

Added a diff to add a test to this into HEAD@​git.

I'm happy to help solving this bug, accordingly with my own limitations.

Tested the two line patch above​:
  open(STDOUT, ">&=1");
  open(STDERR, ">&=2");

and the added test works. But there is some issue with STDIN as well.

The other solution (POSIX based) solves that STDIN problem as well.

@p5pRT
Copy link
Author

p5pRT commented Feb 16, 2013

From @jkeenan

On Sun Dec 19 13​:53​:42 2010, ambs wrote​:

On Sun Dec 19 12​:54​:59 2010, ambs wrote​:

Added a diff to add a test to this into HEAD@​git.

I'm happy to help solving this bug, accordingly with my own limitations.

Tested the two line patch above​:
open(STDOUT, ">&=1");
open(STDERR, ">&=2");

and the added test works. But there is some issue with STDIN as well.

The other solution (POSIX based) solves that STDIN problem as well.

Are there still issues to be resolved in this ticket?

(I looked for the code which the OP cited in
ext/IPC-Open3/lib/IPC/Open3.pm and could not locate it.)

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Feb 16, 2013

From @scottgifford

The code at​:
  http​://cpansearch.perl.org/src/DOM/perl-5.12.5/ext/IPC-Open3/lib/IPC/Open3.pm

doesn't look much different than the code I reported in the original bug, and nobody has
reported here that the bug is fixed, so I would suspect it is still around. I'd be happy to retest if
anybody has reason to believe the bug might be fixed though.

------Scott.

@p5pRT
Copy link
Author

p5pRT commented Feb 17, 2013

From @jkeenan

On Fri Feb 15 20​:29​:11 2013, sgifford wrote​:

The code at​:
http​://cpansearch.perl.org/src/DOM/perl-5.12.5/ext/IPC-
Open3/lib/IPC/Open3.pm

doesn't look much different than the code I reported in the original
bug, and nobody has
reported here that the bug is fixed, so I would suspect it is still
around. I'd be happy to retest if
anybody has reason to believe the bug might be fixed though.

------Scott.

Scott, the link you cite displays version 1.05 of IPC-Open3. However,
Perl 5.16.0 was released with version 1.12, and we are now up to version
1.13 in blead.

Would it be possible for you to re-test with either 1.12 or 1.13?

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Feb 17, 2013

From @arc

James E Keenan via RT <perlbug-followup@​perl.org> wrote​:

Scott, the link you cite displays version 1.05 of IPC-Open3. However,
Perl 5.16.0 was released with version 1.12, and we are now up to version
1.13 in blead.

Would it be possible for you to re-test with either 1.12 or 1.13?

I've run Scott's original test script on newer Perl and IPC​::Open3,
including a recent blead, and I get the same results he reports.

--
Aaron Crane ** http​://aaroncrane.co.uk/

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

No branches or pull requests

2 participants