X2Go Bug report logs - #678
File handles not closed properly in x2gocleansessions

Full log

🔗 View this message in rfc822 format

X-Loop: owner@bugs.x2go.org
Subject: Bug#678: File handles not closed properly in x2gocleansessions
Reply-To: "Matthew L. Dailey" <matthew.l.dailey@dartmouth.edu>, 678@bugs.x2go.org
Resent-From: "Matthew L. Dailey" <matthew.l.dailey@dartmouth.edu>
Resent-To: x2go-dev@lists.x2go.org
Resent-CC: X2Go Developers <x2go-dev@lists.x2go.org>
X-Loop: owner@bugs.x2go.org
Resent-Date: Thu, 20 Nov 2014 17:20:02 +0000
Resent-Message-ID: <handler.678.B.141650379219199@bugs.x2go.org>
Resent-Sender: owner@bugs.x2go.org
X-X2Go-PR-Message: report 678
X-X2Go-PR-Package: x2goserver
X-X2Go-PR-Keywords: 
Received: via spool by submit@bugs.x2go.org id=B.141650379219199
          (code B); Thu, 20 Nov 2014 17:20:02 +0000
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 <submit@bugs.x2go.org>; 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 <submit@bugs.x2go.org>; Thu, 20 Nov 2014 12:09:12 -0500
From: "Matthew L. Dailey" <matthew.l.dailey@dartmouth.edu>
Content-Type: multipart/mixed; boundary="Apple-Mail=_8619CB05-7F8D-4116-8587-1DF613BFF9E0"
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

[Message part 1 (text/plain, inline)]

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’t 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!!

-- 
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 ($_ =~ 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 };

[x2gocleansessions.patch (application/octet-stream, attachment)]

[Message part 3 (text/plain, inline)]

Send a report that this bug log contains spam.