Subject: File handles not closed properly in x2gocleansessions

From: "Matthew L. Dailey" <[email-masked]>
Date: Thu, 20 Nov 2014 12:09:12 -0500

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 };