From matthew.l.dailey@dartmouth.edu Thu Nov 20 18:16:30 2014 Received: (at submit) by bugs.x2go.org; 20 Nov 2014 17:16:32 +0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on ymir.das-netzwerkteam.de X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=BAYES_50 autolearn=ham version=3.3.2 X-Greylist: delayed 418 seconds by postgrey-1.34 at ymir.das-netzwerkteam.de; Thu, 20 Nov 2014 18:16:30 CET Received: from mailhub2.dartmouth.edu (mailhost.dartmouth.edu [129.170.17.107]) by ymir.das-netzwerkteam.de (Postfix) with ESMTPS id 50D5B5E0A2 for ; Thu, 20 Nov 2014 18:16:30 +0100 (CET) Received: from schultz.kiewit.dartmouth.edu (schultz.kiewit.dartmouth.edu [129.170.71.46]) (authenticated bits=0) by mailhub2.dartmouth.edu (8.13.5/DND2.0/8.13.5) with ESMTP id sAKH9Chw026071 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Thu, 20 Nov 2014 12:09:12 -0500 From: "Matthew L. Dailey" Content-Type: multipart/mixed; boundary="Apple-Mail=_8619CB05-7F8D-4116-8587-1DF613BFF9E0" Subject: File handles not closed properly in x2gocleansessions Message-Id: <161ADA96-03D0-40FF-BF9B-F25F4ED36B99@dartmouth.edu> Date: Thu, 20 Nov 2014 12:09:12 -0500 To: submit@bugs.x2go.org Mime-Version: 1.0 (Mac OS X Mail 8.1 \(1993\)) X-Mailer: Apple Mail (2.1993) X-Dartmouth-MailScanner-ID: sAKH9Chw026071 X-MailScanner: Found to be clean by mailhub2.dartmouth.edu X-MailScanner-From: matthew.l.dailey@dartmouth.edu --Apple-Mail=_8619CB05-7F8D-4116-8587-1DF613BFF9E0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Package: x2goserver Version: 4.0.1.18 Greetings, When x2gocleansessions starts up and forks, it is not properly closing = stdin and stdout. This can lead to problems with upstream = programs/scripts. For example, if a script calls apt-get upgrade to = upgrade the x2goserver package, the postinst script loads = x2gocleansessions and since stdin/stdout are not closed, the call will = not finish properly. Looking through bug reports and commits, it looks like there was a bug = #441 to fix a similar problem, but this fix looks like it wasn=E2=80=99t = correct. Then, commit 336917af0dc087e4540fb3d55eb501d2fa4349be in = 4.0.1.15 implemented a more comprehensive fix, but specifically excluded = closing stdin and stout. Attached is a patch to fix a couple of things in this bit of the code: * Redirect stdin, stdout and stderr to /dev/null - as far as I know, = this is best practice for perl daemons and what I have used for years in = my own projects * Test for the existence of the file descriptor before issuing the close = - this fixes a race condition where glob opens its own file descriptor = to get the list, so this descriptor is gone when the script attempts to = close it * Only capture the file descriptor backreference in the regex * Send any close failures to syslog I hope this all makes sense and that this can be incorporated into the = code. Let me know if you have any questions or need any other = information. Thanks for all your great work on X2Go!! --=20 Matthew L. Dailey Senior Systems Engineer Thayer School of Engineering Dartmouth College --- x2gocleansessions.orig 2014-10-07 00:05:56.000000000 -0400 +++ x2gocleansessions 2014-11-20 10:21:56.312590068 -0500 @@ -130,23 +130,21 @@ # close any open file descriptor left open by our parent before = the fork my $fd; for (glob "/proc/$$/fd/*") { - if ($_ =3D~ m/\/proc\/(\d+)\/fd\/(\d+)/) { - $fd =3D $2; - if ( $fd =3D=3D 0 ) { next; } - if ( $fd =3D=3D 1 ) { next; } - if (POSIX::close($fd)) { - print ""; - #print "Closed:II$_\n"; - } else { - print ""; - #print "Error Closing:I$_\n"; + if ( ! -e $_ ) { next; } + if ($_ =3D~ m/\/proc\/\d+\/fd\/(\d+)/) { + $fd =3D $1; + if ( $fd < 3 ) { next; } + if (! POSIX::close($fd)) { + syslog('warning', "Error Closing $_: = $!"); } - } else { - print ""; - #print "ERROR: $_\n"; } } + # redirect stdin, stdout and stderr + open *STDIN, q{<}, '/dev/null'; + open *STDOUT, q{>>}, '/dev/null'; + open *STDERR, q{>>}, '/dev/null'; + $SIG{TERM}=3D\&catch_term; $SIG{CHLD} =3D sub { wait }; --Apple-Mail=_8619CB05-7F8D-4116-8587-1DF613BFF9E0 Content-Disposition: attachment; filename=x2gocleansessions.patch Content-Type: application/octet-stream; name="x2gocleansessions.patch" Content-Transfer-Encoding: 7bit --- x2gocleansessions.orig 2014-10-07 00:05:56.000000000 -0400 +++ x2gocleansessions 2014-11-20 10:21:56.312590068 -0500 @@ -130,23 +130,21 @@ # close any open file descriptor left open by our parent before the fork my $fd; for (glob "/proc/$$/fd/*") { - if ($_ =~ m/\/proc\/(\d+)\/fd\/(\d+)/) { - $fd = $2; - if ( $fd == 0 ) { next; } - if ( $fd == 1 ) { next; } - if (POSIX::close($fd)) { - print ""; - #print "Closed:II$_\n"; - } else { - print ""; - #print "Error Closing:I$_\n"; + if ( ! -e $_ ) { next; } + if ($_ =~ m/\/proc\/\d+\/fd\/(\d+)/) { + $fd = $1; + if ( $fd < 3 ) { next; } + if (! POSIX::close($fd)) { + syslog('warning', "Error Closing $_: $!"); } - } else { - print ""; - #print "ERROR: $_\n"; } } + # redirect stdin, stdout and stderr + open *STDIN, q{<}, '/dev/null'; + open *STDOUT, q{>>}, '/dev/null'; + open *STDERR, q{>>}, '/dev/null'; + $SIG{TERM}=\&catch_term; $SIG{CHLD} = sub { wait }; --Apple-Mail=_8619CB05-7F8D-4116-8587-1DF613BFF9E0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=us-ascii --Apple-Mail=_8619CB05-7F8D-4116-8587-1DF613BFF9E0--