Merge lp:~iahmad/libpam-freerdp/improved-test-coverage into lp:libpam-freerdp

[I Ahmad]

Improving the test coverage.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:39
http://jenkins.qa.ubuntu.com/job/libpam-freerdp-ci/17/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libpam-freerdp-ci/./label=precise/17/console

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:39
http://s-jenkins:8080/job/libpam-freerdp2-ci/1/
Executed test runs:
    FAILURE: http://s-jenkins:8080/job/libpam-freerdp2-ci/./build=pbuilder,distribution=quantal,flavor=amd64/1/console
    FAILURE: http://s-jenkins:8080/job/libpam-freerdp2-ci/./build=pbuilder,distribution=quantal,flavor=i386/1/console

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:40
http://s-jenkins:8080/job/libpam-freerdp2-ci/2/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/libpam-freerdp2-ci/./build=pbuilder,distribution=quantal,flavor=amd64/2/console
    SUCCESS: http://s-jenkins:8080/job/libpam-freerdp2-ci/./build=pbuilder,distribution=quantal,flavor=i386/2/console

review: Approve (continuous-integration)

[David Barth (dbarth)]

The code changes look fine to me.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
No commit message was specified.
http://s-jenkins:8080/job/libpam-freerdp-autolanding/16/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/libpam-freerdp-autolanding/./label=precise/16/console

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
No commit message was specified.
http://s-jenkins:8080/job/libpam-freerdp2-autolanding/1/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/libpam-freerdp2-autolanding/./build=pbuilder,distribution=quantal,flavor=amd64/1/console

review: Needs Fixing (continuous-integration)

=== modified file 'src/Makefile.am'
--- src/Makefile.am	2012-09-25 15:59:04 +0000
+++ src/Makefile.am	2012-10-16 11:18:22 +0000
@@ -23,7 +23,8 @@
 	libfreerdpcore.la
 
 libfreerdpcore_la_SOURCES = \
-	pam-freerdp.c
+	pam-freerdp.c \
+	pam-freerdp-children.c
 libfreerdpcore_la_CFLAGS = \
 	-Wall -Werror \
 	$(COVERAGE_CFLAGS)

=== added file 'src/pam-freerdp-children.c'
--- src/pam-freerdp-children.c	1970-01-01 00:00:00 +0000
+++ src/pam-freerdp-children.c	2012-10-16 11:18:22 +0000
@@ -0,0 +1,231 @@
+/*
+ * Copyright © 2012 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Ted Gould <[e-mail]>
+ */
+
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/wait.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <sys/un.h>
+#include <pwd.h>
+#include <grp.h>
+#include <errno.h>
+
+#include <security/pam_modules.h>
+#include <security/pam_modutil.h>
+#include <security/pam_appl.h>
+
+#include "pam-freerdp-children.h"
+#include "auth-check-path.h"
+
+void
+pam_sm_authenticate_helper (int *stdinpipe, const char* username, const char* rhost, const char* ruser, const char* rdomain)
+{
+
+	dup2(stdinpipe[0], 0);
+
+	char * args[5];
+
+	args[0] = (char *)auth_check_path;
+	args[1] = (char *)rhost;
+	args[2] = (char *)ruser;
+	args[3] = (char *)rdomain;
+	args[4] = NULL;
+
+	struct passwd * pwdent = getpwnam(username);
+	if (pwdent == NULL) {
+		_exit(EXIT_FAILURE);
+	}
+
+	/* Setting groups, but allowing EPERM as if we're not 100% root
+	   we might not be able to do this */
+	if (setgroups(1, &pwdent->pw_gid) != 0 && errno != EPERM) {
+		_exit(EXIT_FAILURE);
+	}
+
+	if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
+			setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
+		_exit(EXIT_FAILURE);
+	}
+
+	if (clearenv() != 0) {
+		_exit(EXIT_FAILURE);
+	}
+
+	if (chdir(pwdent->pw_dir) != 0) {
+		_exit(EXIT_FAILURE);
+	}
+
+	setenv("HOME", pwdent->pw_dir, 1);
+
+	execvp(args[0], args);
+	_exit(0);
+}
+
+int
+session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruser, const char * rhost, const char * rdomain, const char * password)
+{
+	/* Socket stuff */
+	int socketfd = 0;
+	struct sockaddr_un socket_addr;
+
+	/* Connected user */
+	socklen_t connected_addr_size;
+	int connectfd = 0;
+	struct sockaddr_un connected_addr;
+
+	/* Our buffer */
+	char * buffer = NULL;
+	int buffer_len = 0;
+	int buffer_fill = 0;
+
+	/* Track write out */
+	int writedata = 0;
+
+	/* Track ready writing */
+	int readywrite = 0;
+
+	/* Setting groups, but allowing EPERM as if we're not 100% root
+	   we might not be able to do this */
+	if (setgroups(1, &pwdent->pw_gid) != 0 && errno != EPERM) {
+		_exit(EXIT_FAILURE);
+	}
+
+	if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
+			setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
+	if (clearenv() != 0) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
+	if (chdir(pwdent->pw_dir) != 0) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
+	if (rdomain[0] == '\0') {
+		rdomain = ".";
+	}
+
+	/* Build this up as a buffer so we can just write it and see that
+	   very, very clearly */
+	buffer_len += strlen(ruser) + 1;    /* Add one for the space */
+	buffer_len += strlen(rhost) + 1;    /* Add one for the space */
+	buffer_len += strlen(rdomain) + 1;  /* Add one for the space */
+	buffer_len += strlen(password) + 1; /* Add one for the NULL */
+
+	if (buffer_len < 5) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
+	buffer = malloc(buffer_len);
+
+	if (buffer == NULL) {
+		/* Don't need to clean up yet */
+		return EXIT_FAILURE;
+	}
+
+	/* Lock the buffer before writing */
+	if (mlock(buffer, buffer_len) != 0) {
+		/* We can't lock, we go home */
+		goto cleanup;
+	}
+
+	buffer_fill = snprintf(buffer, buffer_len, "%s %s %s %s", ruser, password, rdomain, rhost);
+	if (buffer_fill > buffer_len) {
+		/* This really shouldn't happen, but if for some reason we have an
+		   difference between they way that the lengths are calculated we want
+		   to catch that. */
+		goto cleanup;
+	}
+
+	/* Make our socket and bind it */
+	socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
+	if (socketfd < 0) {
+		goto cleanup;
+	}
+
+	memset(&socket_addr, 0, sizeof(struct sockaddr_un));
+	socket_addr.sun_family = AF_UNIX;
+	strncpy(socket_addr.sun_path, pwdent->pw_dir, sizeof(socket_addr.sun_path) - 1);
+	strncpy(socket_addr.sun_path + strlen(pwdent->pw_dir), "/.freerdp-socket", (sizeof(socket_addr.sun_path) - strlen(pwdent->pw_dir)) - 1);
+
+	/* We bind the socket before forking so that we ensure that
+	   there isn't a race condition to get to it.  Things will block
+	   otherwise. */
+	if (bind(socketfd, (struct sockaddr *)&socket_addr, sizeof(struct sockaddr_un)) < 0) {
+		goto cleanup;
+	}
+
+	/* Set the socket file permissions to be 600 and the user and group
+	   to be the guest user.  NOTE: This won't protect on BSD */
+	if (chmod(socket_addr.sun_path, S_IRUSR | S_IWUSR) != 0 ||
+			chown(socket_addr.sun_path, pwdent->pw_uid, pwdent->pw_gid) != 0) {
+		goto cleanup;
+	}
+
+	if (listen(socketfd, 1) < 0) {
+		goto cleanup;
+	}
+
+	readywrite = write(readypipe, ALL_GOOD_SIGNAL, strlen(ALL_GOOD_SIGNAL) + 1);
+	if (readywrite != strlen(ALL_GOOD_SIGNAL) + 1) {
+		goto cleanup;
+	}
+
+	connected_addr_size = sizeof(struct sockaddr_un);
+	connectfd = accept(socketfd, (struct sockaddr *)&connected_addr, &connected_addr_size);
+	if (connectfd < 0) {
+		goto cleanup;
+	}
+
+	writedata = write(connectfd, buffer, buffer_len);
+
+cleanup:
+	if (socketfd != 0) {
+		close(socketfd);
+	}
+	if (connectfd != 0) {
+		close(connectfd);
+	}
+
+	if (buffer != NULL) {
+		memset(buffer, 0, buffer_len);
+		munlock(buffer, buffer_len);
+		free(buffer);
+		buffer = NULL;
+	}
+
+	/* This should be only true on the write, so we can use this to check
+	   out as writedata is init to 0 */
+	if (writedata == buffer_len) {
+		_exit (0);
+	}
+
+	_exit(EXIT_FAILURE);
+}
+

=== added file 'src/pam-freerdp-children.h'
--- src/pam-freerdp-children.h	1970-01-01 00:00:00 +0000
+++ src/pam-freerdp-children.h	2012-10-16 11:18:22 +0000
@@ -0,0 +1,30 @@
+/*
+ * Copyright © 2012 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Ted Gould <[e-mail]>
+ */
+
+#ifndef _PAM_FREERDP_CHILDREN_H_
+#define _PAM_FREERDP_CHILDREN_H_
+
+#define PAM_TYPE_DOMAIN  1234
+#define ALL_GOOD_SIGNAL  "Ar, ready to authenticate cap'n"
+
+void
+pam_sm_authenticate_helper (int *stdinpipe, const char* username, const char* rhost, const char* ruser, const char* rdomain);
+
+int
+session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruser, const char * rhost, const char * rdomain, const char * password);
+#endif //_PAM_FREERDP_CHILDREN_H_

=== modified file 'src/pam-freerdp.c'
--- src/pam-freerdp.c	2012-09-20 08:39:53 +0000
+++ src/pam-freerdp.c	2012-10-16 11:18:22 +0000
@@ -34,11 +34,9 @@
 #include <security/pam_modutil.h>
 #include <security/pam_appl.h>
 
+#include "pam-freerdp-children.h"
 #include "auth-check-path.h"
 
-#define PAM_TYPE_DOMAIN  1234
-#define ALL_GOOD_SIGNAL  "Ar, ready to authenticate cap'n"
-
 static int unpriveleged_kill (struct passwd * pwdent);
 
 static char * global_domain = NULL;
@@ -236,44 +234,7 @@
 	pid_t pid;
 	switch (pid = fork()) {
 	case 0: { /* child */
-		dup2(stdinpipe[0], 0);
-
-		char * args[5];
-
-		args[0] = (char *)auth_check_path;
-		args[1] = rhost;
-		args[2] = ruser;
-		args[3] = rdomain;
-		args[4] = NULL;
-
-		struct passwd * pwdent = getpwnam(username);
-		if (pwdent == NULL) {
-			_exit(EXIT_FAILURE);
-		}
-
-		/* Setting groups, but allowing EPERM as if we're not 100% root
-		   we might not be able to do this */
-		if (setgroups(1, &pwdent->pw_gid) != 0 && errno != EPERM) {
-			_exit(EXIT_FAILURE);
-		}
-
-		if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
-				setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
-			_exit(EXIT_FAILURE);
-		}
-
-		if (clearenv() != 0) {
-			_exit(EXIT_FAILURE);
-		}
-
-		if (chdir(pwdent->pw_dir) != 0) {
-			_exit(EXIT_FAILURE);
-		}
-
-		setenv("HOME", pwdent->pw_dir, 1);
-
-		execvp(args[0], args);
-		_exit(EXIT_FAILURE);
+		pam_sm_authenticate_helper (stdinpipe, username, rhost, ruser, rdomain);
 		break;
 	}
 	case -1: { /* fork'n error! */
@@ -304,153 +265,7 @@
 	return retval;
 }
 
-static int
-session_socket_handler (struct passwd * pwdent, int readypipe, const char * ruser, const char * rhost, const char * rdomain, const char * password)
-{
-	/* Socket stuff */
-	int socketfd = 0;
-	struct sockaddr_un socket_addr;
-
-	/* Connected user */
-	socklen_t connected_addr_size;
-	int connectfd = 0;
-	struct sockaddr_un connected_addr;
-
-	/* Our buffer */
-	char * buffer = NULL;
-	int buffer_len = 0;
-	int buffer_fill = 0;
-
-	/* Track write out */
-	int writedata = 0;
-
-	/* Track ready writing */
-	int readywrite = 0;
-
-	/* Setting groups, but allowing EPERM as if we're not 100% root
-	   we might not be able to do this */
-	if (setgroups(1, &pwdent->pw_gid) != 0 && errno != EPERM) {
-		_exit(EXIT_FAILURE);
-	}
-
-	if (setgid(pwdent->pw_gid) < 0 || setuid(pwdent->pw_uid) < 0 ||
-			setegid(pwdent->pw_gid) < 0 || seteuid(pwdent->pw_uid) < 0) {
-		/* Don't need to clean up yet */
-		return EXIT_FAILURE;
-	}
-
-	if (clearenv() != 0) {
-		/* Don't need to clean up yet */
-		return EXIT_FAILURE;
-	}
-
-	if (chdir(pwdent->pw_dir) != 0) {
-		/* Don't need to clean up yet */
-		return EXIT_FAILURE;
-	}
-
-	if (rdomain[0] == '\0') {
-		rdomain = ".";
-	}
-
-	/* Build this up as a buffer so we can just write it and see that
-	   very, very clearly */
-	buffer_len += strlen(ruser) + 1;    /* Add one for the space */
-	buffer_len += strlen(rhost) + 1;    /* Add one for the space */
-	buffer_len += strlen(rdomain) + 1;  /* Add one for the space */
-	buffer_len += strlen(password) + 1; /* Add one for the NULL */
-
-	if (buffer_len < 5) {
-		/* Don't need to clean up yet */
-		return EXIT_FAILURE;
-	}
-
-	buffer = malloc(buffer_len);
-
-	if (buffer == NULL) {
-		/* Don't need to clean up yet */
-		return EXIT_FAILURE;
-	}
-
-	/* Lock the buffer before writing */
-	if (mlock(buffer, buffer_len) != 0) {
-		/* We can't lock, we go home */
-		goto cleanup;
-	}
-
-	buffer_fill = snprintf(buffer, buffer_len, "%s %s %s %s", ruser, password, rdomain, rhost);
-	if (buffer_fill > buffer_len) {
-		/* This really shouldn't happen, but if for some reason we have an
-		   difference between they way that the lengths are calculated we want
-		   to catch that. */
-		goto cleanup;
-	}
-
-	/* Make our socket and bind it */
-	socketfd = socket(AF_UNIX, SOCK_STREAM, 0);
-	if (socketfd < 0) {
-		goto cleanup;
-	}
-
-	memset(&socket_addr, 0, sizeof(struct sockaddr_un));
-	socket_addr.sun_family = AF_UNIX;
-	strncpy(socket_addr.sun_path, pwdent->pw_dir, sizeof(socket_addr.sun_path) - 1);
-	strncpy(socket_addr.sun_path + strlen(pwdent->pw_dir), "/.freerdp-socket", (sizeof(socket_addr.sun_path) - strlen(pwdent->pw_dir)) - 1);
-
-	/* We bind the socket before forking so that we ensure that
-	   there isn't a race condition to get to it.  Things will block
-	   otherwise. */
-	if (bind(socketfd, (struct sockaddr *)&socket_addr, sizeof(struct sockaddr_un)) < 0) {
-		goto cleanup;
-	}
-
-	/* Set the socket file permissions to be 600 and the user and group
-	   to be the guest user.  NOTE: This won't protect on BSD */
-	if (chmod(socket_addr.sun_path, S_IRUSR | S_IWUSR) != 0 ||
-			chown(socket_addr.sun_path, pwdent->pw_uid, pwdent->pw_gid) != 0) {
-		goto cleanup;
-	}
-
-	if (listen(socketfd, 1) < 0) {
-		goto cleanup;
-	}
-
-	readywrite = write(readypipe, ALL_GOOD_SIGNAL, strlen(ALL_GOOD_SIGNAL) + 1);
-	if (readywrite != strlen(ALL_GOOD_SIGNAL) + 1) {
-		goto cleanup;
-	}
-
-	connected_addr_size = sizeof(struct sockaddr_un);
-	connectfd = accept(socketfd, (struct sockaddr *)&connected_addr, &connected_addr_size);
-	if (connectfd < 0) {
-		goto cleanup;
-	}
-
-	writedata = write(connectfd, buffer, buffer_len);
-
-cleanup:
-	if (socketfd != 0) {
-		close(socketfd);
-	}
-	if (connectfd != 0) {
-		close(connectfd);
-	}
-
-	if (buffer != NULL) {
-		memset(buffer, 0, buffer_len);
-		munlock(buffer, buffer_len);
-		free(buffer);
-		buffer = NULL;
-	}
-
-	/* This should be only true on the write, so we can use this to check
-	   out as writedata is init to 0 */
-	if (writedata == buffer_len) {
-		return 0;
-	}
-
-	return EXIT_FAILURE;
-}
+
 
 pid_t session_pid = 0;
 /* Open Session.  Here we need to fork a little process so that we can
@@ -492,12 +307,11 @@
 
 	pid_t pid = fork();
 	if (pid == 0) {
-		int retval = 0;
-
-		retval = session_socket_handler(pwdent, sessionready[1], ruser, rhost, rdomain, password);
+		
+		int ret = session_socket_handler(pwdent, sessionready[1], ruser, rhost, rdomain, password);
 
 		close(sessionready[1]);
-		_exit(retval);
+		_exit(ret);
 	} else if (pid < 0) {
 		close(sessionready[0]);
 		close(sessionready[1]);

=== modified file 'tests/mock_guest.c'
--- tests/mock_guest.c	2012-09-19 19:35:36 +0000
+++ tests/mock_guest.c	2012-10-16 11:18:22 +0000
@@ -9,6 +9,11 @@
 #include <errno.h>
 #include <unistd.h>
 #include <sys/stat.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <stdio.h>
 
 static struct passwd guest = { "guest",
 							   "password",
@@ -49,3 +54,69 @@
 int chown(const char *path, uid_t owner, gid_t group)
 { return 0; }
 
+int execvp(const char *file, char *const argv[])
+{
+	return 0;
+}
+/* wrap _exit, to make sure the gcov_exit function installed with atexit()
+   is really called to collect coverage statistics */
+void _exit (int exitcode)
+{
+	exit (exitcode);
+}
+
+
+#define BUFFER_SIZE  512
+
+/*Borrowed this code form socket-sucker.c in lightdm-remote-session-freerdp*/
+int
+socket_sucker ()
+{
+	int socket_fd = 0;
+	int servlen = 0;
+	struct sockaddr_un serv_addr;
+
+	bzero((char *)&serv_addr, sizeof(serv_addr));
+
+	const char * home = getenv("HOME");
+	if (home == NULL) {
+		return -1;
+	}
+
+	serv_addr.sun_family = AF_UNIX;
+	
+	int printsize = snprintf(serv_addr.sun_path, sizeof(serv_addr.sun_path) - 1, "%s/%s", home, ".freerdp-socket");
+	if (printsize > sizeof(serv_addr.sun_path) - 1 || printsize < 0) {
+		return -1;
+	}
+
+	servlen = strlen(serv_addr.sun_path) + sizeof(serv_addr.sun_family);
+
+	if ((socket_fd = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {
+		return -1;
+	}
+
+	if (connect(socket_fd, (struct sockaddr *)&serv_addr, servlen) < 0) {
+		return -1;
+	}
+
+	char buffer[BUFFER_SIZE + 2];
+	int in = 0;
+	int out = 0;
+
+	in = read(socket_fd, buffer, BUFFER_SIZE);
+
+	if (in > 0) {
+		out = write(1, buffer, in);
+	}
+
+	close(socket_fd);
+
+	if (in > 0 && out > 0 && in == out) {
+		return 0;
+	} else {
+		return -1;
+	}
+}
+
+

=== modified file 'tests/mock_guest.h'
--- tests/mock_guest.h	2012-09-19 19:35:36 +0000
+++ tests/mock_guest.h	2012-10-16 11:18:22 +0000
@@ -20,5 +20,7 @@
 int seteuid(uid_t uid);
 int chmod(const char *path, mode_t mode);
 int chown(const char *path, uid_t owner, gid_t group);
+int execvp(const char *file, char *const argv[]);
+int socket_sucker();
 
 #endif

=== modified file 'tests/test-freerdp-wrapper.cc'
--- tests/test-freerdp-wrapper.cc	2012-09-20 04:32:08 +0000
+++ tests/test-freerdp-wrapper.cc	2012-10-16 11:18:22 +0000
@@ -67,8 +67,10 @@
 				 pam_sm_authenticate (pamh, 0, 0, argv));
 	  EXPECT_EQ (PAM_SUCCESS, 
 				 pam_sm_setcred (pamh, 0, 0, argv));
+	  
 	  EXPECT_EQ (PAM_SUCCESS, 
 				 pam_sm_open_session (pamh, 0, 0, argv));
+	  EXPECT_EQ(0, socket_sucker()); 
 	  EXPECT_EQ (PAM_SUCCESS, 
 				 pam_sm_close_session (pamh, 0, 0, argv));
   }