Merge lp:~tyhicks/ecryptfs/fix-most-Wall-and-Wextra-warnings into lp:ecryptfs

[Tyler Hicks]

This is a good start towards fixing all warnings emitted when building with
the output of Saucy's `dpkg-buildflags --dump` plus '-Wall -Wextra' on amd64,
i386, and armhf.

I want to have a quiet, warning-free build so that we can use the compiler to
help a little more with catching bugs in new commits. I don't want to go as far
as enabling -Werror by default, but it'd be nice to have the option of using it
locally when developing/testing/reviewing new commits.

What's left is:

[-Wmissing-field-initializers]
[-Wunused-parameter]
[-Wunused-result]

I'll fix those remaining three very soon. I just didn't get to them before
Monday rolled around.

[Tyler Hicks (tyhicks)]

Note that the "Unmerged revisions" section below isn't showing all of the revisions. It is missing the descriptions for r800 through r804. I'm sure you can look at the proposed branch, but here's a quick paste of those revision descriptions:

------------------------------------------------------------
revno: 804
  Fix some of the signed and unsigned int comparisons (-Wsign-compare)
------------------------------------------------------------
revno: 803
  Fix -Wempty-body warning
------------------------------------------------------------
revno: 802
  Remove the unused 'order' member of the supported_key_bytes struct
------------------------------------------------------------
revno: 801
  Fix sign comparison warnings and other issues surrounding the uint32_t type
  used for key_bytes
------------------------------------------------------------
revno: 800
  Fix 'unused variable' warnings
------------------------------------------------------------

[Colin Ian King (colin-king)]

These look good to me. I've run them through Coverity Scan too and it looks fine.

review: Approve

[Tyler Hicks (tyhicks)]

I pushed one more patch, r815, that fixed a bug in r801. I typoed the overflow check conditional. I should have used UINTMAX_MAX instead of UINT_MAX.

[Colin Ian King (colin-king)]

Ah, I didn't spot that, so much for my careful reviewing :-/

[Tyler Hicks (tyhicks)]

The name of those macros are too easy to get switched around. I caught it out of sheer luck during my last second check before pushing.

=== modified file 'src/libecryptfs/ecryptfs-stat.c'
--- src/libecryptfs/ecryptfs-stat.c	2013-03-13 14:52:43 +0000
+++ src/libecryptfs/ecryptfs-stat.c	2013-10-28 17:50:31 +0000
@@ -80,7 +80,7 @@
 				  char *buf, int *bytes_read)
 {
 	int rc = 0;
-	int i;
+	size_t i;
 	uint32_t flags;
 	int big_endian;
 
 
=== modified file 'src/libecryptfs/key_management.c'
--- src/libecryptfs/key_management.c	2013-10-25 19:45:09 +0000
+++ src/libecryptfs/key_management.c	2013-10-28 17:50:31 +0000
@@ -247,7 +247,6 @@
 	int rc = 0;
 	ssize_t size;
 	int fd;
-	int i;
 	char *p = NULL;
 	char decrypted_passphrase[ECRYPTFS_MAX_PASSPHRASE_BYTES + 1];
 
 
=== modified file 'src/libecryptfs/miscdev.c'
--- src/libecryptfs/miscdev.c	2013-10-25 19:45:09 +0000
+++ src/libecryptfs/miscdev.c	2013-10-28 17:50:31 +0000
@@ -153,7 +153,7 @@
 		packet_len = 0;
 	}
 	miscdev_msg_data_size = (1 + 4 + packet_len_size + packet_len);
-	if (miscdev_msg_data_size != read_bytes) {
+	if (miscdev_msg_data_size != (size_t)read_bytes) {
 		rc = -EINVAL;
 		syslog(LOG_ERR, "%s: Invalid packet. (1 + 4 + "
 		       "packet_len_size=[%zu] + packet_len=[%zu])=[%zu] != "
 
=== modified file 'src/libecryptfs/module_mgr.c'
--- src/libecryptfs/module_mgr.c	2013-10-25 19:45:09 +0000
+++ src/libecryptfs/module_mgr.c	2013-10-28 17:50:31 +0000
@@ -23,6 +23,8 @@
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <stdint.h>
+#include <inttypes.h>
 #include "../include/ecryptfs.h"
 #include "../include/decision_graph.h"
 
@@ -265,8 +267,9 @@
 	} else if (node->val) {
 		if (yn < 0)
 			rc = WRONG_VALUE;
-	} else
+	} else {
 		/* default: no */;
+	}
 out_free:
 	if (node->val) {
 		free(node->val);
@@ -389,33 +392,32 @@
 static struct supported_key_bytes {
 	char *cipher_name;
 	uint32_t key_bytes;
-	uint32_t order;
 } supported_key_bytes[] = {
-	{"aes", 16, 1},
-	{"aes", 32, 2},
-	{"aes", 24, 3},
-	{"anubis", 16, 1},
-	{"anubis", 32, 2},
-	{"des3_ede", 24, 1},
-	{"serpent", 16, 1},
-	{"serpent", 32, 2},
-	{"tnepres", 16, 1},
-	{"tnepres", 32, 2},
-	{"tea", 16, 1},
-	{"xeta", 16, 1},
-	{"xtea", 16, 1},
-	{"cast5", 16, 1},
-	{"cast6", 16, 1},
-	{"cast6", 32, 2},
-	{"twofish", 16, 1},
-	{"twofish", 32, 2},
-	{"blowfish", 16, 1},
-	{"blowfish", 32, 2},
-	{"blowfish", 56, 2},
-	{"khazad", 16, 1},
-	{"arc4", 16, 1},
-	{"arc4", 32, 2},
-	{NULL, 0, 0}
+	{"aes", 16},
+	{"aes", 32},
+	{"aes", 24},
+	{"anubis", 16},
+	{"anubis", 32},
+	{"des3_ede", 24},
+	{"serpent", 16},
+	{"serpent", 32},
+	{"tnepres", 16},
+	{"tnepres", 32},
+	{"tea", 16},
+	{"xeta", 16},
+	{"xtea", 16},
+	{"cast5", 16},
+	{"cast6", 16},
+	{"cast6", 32},
+	{"twofish", 16},
+	{"twofish", 32},
+	{"blowfish", 16},
+	{"blowfish", 32},
+	{"blowfish", 56},
+	{"khazad", 16},
+	{"arc4", 16},
+	{"arc4", 32},
+	{NULL, 0}
 };
 
 static int tf_ecryptfs_key_bytes(struct ecryptfs_ctx *ctx,
@@ -438,7 +440,7 @@
 }
 
 static int init_ecryptfs_key_bytes_param_node(char *cipher_name, 
-					      int min, int max)
+					      uint32_t min, uint32_t max)
 {
 	int i;
 	int rc = 0;
@@ -453,7 +455,7 @@
 			
 			tn = &ecryptfs_key_bytes_param_node.tl[
 				ecryptfs_key_bytes_param_node.num_transitions];
-			rc = asprintf(&tn->val, "%d",
+			rc = asprintf(&tn->val, "%"PRIu32,
 				      supported_key_bytes[i].key_bytes);
 			if (rc == -1) {
 				rc = -ENOMEM;
@@ -462,7 +464,7 @@
 			rc = 0;
 			if (!ecryptfs_key_bytes_param_node.suggested_val) {
 				rc = asprintf(&ecryptfs_key_bytes_param_node.suggested_val,
-					      "%d",
+					      "%"PRIu32,
 					      supported_key_bytes[i].key_bytes);
 				if (rc == -1) {
 					rc = -ENOMEM;
@@ -485,12 +487,33 @@
 	return rc;
 }
 
+static int parse_key_bytes(uint32_t *bytes, const char *str)
+{
+	uintmax_t tmp;
+	char *eptr;
+
+	if (str[0] == '-')
+		return -ERANGE;
+
+	errno = 0;
+	tmp = strtoumax(str, &eptr, 10);
+	if (eptr == str)
+		return -EINVAL;
+	else if (tmp == UINT_MAX && errno == ERANGE)
+		return -ERANGE;
+	else if (tmp > UINT32_MAX)
+		return -ERANGE;
+
+	*bytes = (uint32_t)tmp;
+	return 0;
+}
+
 static int tf_ecryptfs_cipher(struct ecryptfs_ctx *ctx, struct param_node *node,
 			      struct val_node **head, void **foo)
 {
 	char *opt;
 	int rc;
-	int min = 0, max = 999999;
+	uint32_t min = 0, max = 999999;
 	struct val_node *tmp = *head, *tmpprev = NULL;
 
 	while (tmp) {
@@ -498,18 +521,16 @@
 		int popval = 0;
 		if (tmp->val && (strstr(tmp->val,"max_key_bytes=") != NULL) && 
 		    ((ptr=strchr(tmp->val,'=')) != NULL)) {
-			char *eptr;
-			max = strtol(++ptr, &eptr, 10);
-			if (eptr == ptr)
-				return -EINVAL;
+			rc = parse_key_bytes(&max, ++ptr);
+			if (rc)
+				return rc;
 			popval = 1;
 		}
 		if (tmp->val && (strstr(tmp->val,"min_key_bytes=") != NULL) && 
 		    ((ptr=strchr(tmp->val,'=')) != NULL)) {
-			char *eptr;
-			min = strtol(++ptr, &eptr, 10);
-			if (eptr == ptr)
-				return -EINVAL;
+			rc = parse_key_bytes(&min, ++ptr);
+			if (rc)
+				return rc;
 			popval = 1;
 		}
 		if (popval) {
 
=== modified file 'src/pam_ecryptfs/pam_ecryptfs.c'
--- src/pam_ecryptfs/pam_ecryptfs.c	2013-10-25 19:45:09 +0000
+++ src/pam_ecryptfs/pam_ecryptfs.c	2013-10-28 17:50:31 +0000
@@ -46,31 +46,6 @@
 
 #define PRIVATE_DIR "Private"
 
-static void error(const char *msg)
-{
-	syslog(LOG_ERR, "pam_ecryptfs: errno = [%i]; strerror = [%m]\n", errno);
-	switch (errno) {
-	case ENOKEY:
-		syslog(LOG_ERR, "pam_ecryptfs: %s: Requested key not available\n", msg);
-		return;
-
-	case EKEYEXPIRED:
-		syslog(LOG_ERR, "pam_ecryptfs: %s: Key has expired\n", msg);
-		return;
-
-	case EKEYREVOKED:
-		syslog(LOG_ERR, "pam_ecryptfs: %s: Key has been revoked\n", msg);
-		return;
-
-	case EKEYREJECTED:
-		syslog(LOG_ERR, "pam_ecryptfs: %s: Key was rejected by service\n", msg);
-		return;
-	default:
-		syslog(LOG_ERR, "pam_ecryptfs: %s: Unknown key error\n", msg);
-		return;
-	}
-}
-
 /* returns: 0 if file does not exist, 1 if it exists, <0 for error */
 static int file_exists_dotecryptfs(const char *homedir, char *filename)
 {
@@ -211,8 +186,6 @@
 		if ((argc == 1)
 		    && (memcmp(argv[0], "unwrap\0", 7) == 0)) {
 			char *wrapped_pw_filename;
-			char *unwrapped_pw_filename;
-			struct stat s;
 
 			rc = asprintf(
 				&wrapped_pw_filename, "%s/.ecryptfs/%s",
@@ -306,8 +279,6 @@
 	char *autoumount = "auto-umount";
 	struct stat s;
 	pid_t pid;
-	struct utmp *u;
-	int count = 0;
 
 	if ((pwd = fetch_pwd(pamh)) == NULL) {
 		/* fetch_pwd() logged a message */
@@ -433,7 +404,6 @@
 	char *old_passphrase = NULL;
 	char *new_passphrase = NULL;
 	char *wrapped_pw_filename;
-	char *name = NULL;
 	char salt[ECRYPTFS_SALT_SIZE];
 	char salt_hex[ECRYPTFS_SALT_SIZE_HEX];
 	pid_t child_pid, tmp_pid;
@@ -448,7 +418,6 @@
 			uid = pwd->pw_uid;
 			gid = pwd->pw_gid;
 			homedir = pwd->pw_dir;
-			name = pwd->pw_name;
 		}
 	} else {
 		syslog(LOG_ERR, "pam_ecryptfs: Error getting passwd info for user [%s]; rc = [%d]\n", username, rc);
 
=== modified file 'src/utils/mount.ecryptfs.c'
--- src/utils/mount.ecryptfs.c	2013-10-25 19:45:09 +0000
+++ src/utils/mount.ecryptfs.c	2013-10-28 17:50:31 +0000
@@ -33,6 +33,7 @@
 #include <sys/mount.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include "ecryptfs.h"
 #include "decision_graph.h"
 #include "io.h"
 
=== modified file 'src/utils/mount.ecryptfs_private.c'
--- src/utils/mount.ecryptfs_private.c	2013-10-25 21:33:30 +0000
+++ src/utils/mount.ecryptfs_private.c	2013-10-28 17:50:31 +0000
@@ -30,6 +30,7 @@
 #include <sys/param.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <ctype.h>
 #include <errno.h>
 #include <keyutils.h>
 #include <mntent.h>
@@ -51,7 +52,7 @@
 #define FSTYPE "ecryptfs"
 #define TMP "/dev/shm"
 
-int read_config(char *pw_dir, int uid, char *alias, char **s, char **d, char **o) {
+int read_config(char *pw_dir, uid_t uid, char *alias, char **s, char **d, char **o) {
 /* Read an fstab(5) style config file */
 	char *fnam;
 	struct stat mstat;
@@ -91,7 +92,7 @@
 	*s = strdup(e->mnt_fsname);
 	if (!*s)
 		return -2;
-out:
+
 	return 0;
 }
 
@@ -138,7 +139,7 @@
 	char *sig_file;
 	FILE *fh = NULL;
 	char **sig = NULL;
-	int i, j;
+	unsigned int i, j;
 	/* Construct sig file name */
 	if (asprintf(&sig_file, "%s/.ecryptfs/%s.sig", pw_dir, alias) < 0) {
 		perror("asprintf");
@@ -219,7 +220,7 @@
 	return NULL;
 }
 
-int check_ownership_mnt(int uid, char **mnt) {
+int check_ownership_mnt(uid_t uid, char **mnt) {
 /* Check ownership of mount point, chdir into it, and
  * canonicalize the path for use in mtab updating.
  * Return 0 if everything is in order, 1 on error.
@@ -260,7 +261,7 @@
 }
 
 
-int check_ownerships(int uid, char *path) {
+int check_ownerships(uid_t uid, char *path) {
 /* Check ownership of device and mount point.
  * Return 0 if everything is in order, 1 on error.
  */
@@ -372,7 +373,7 @@
 	return 1;
 }
 
-FILE *lock_counter(char *u, int uid, char *alias) {
+FILE *lock_counter(char *u, uid_t uid, char *alias) {
 	char *f;
 	int fd;
 	FILE *fh;
@@ -506,7 +507,9 @@
  *  c) updating /etc/mtab
  */
 int main(int argc, char *argv[]) {
-	int uid, gid, mounting;
+	uid_t uid;
+	gid_t gid;
+	int mounting;
 	int force = 0;
 	struct passwd *pwd;
 	char *alias, *src, *dest, *opt, *opts2;
 
=== modified file 'src/utils/test.c'
--- src/utils/test.c	2013-10-25 19:45:09 +0000
+++ src/utils/test.c	2013-10-28 17:50:31 +0000
@@ -103,7 +103,7 @@
 	unsigned long lower_page_idx;
 	int byte_offset;
 	int rc = 0;
-	int i;
+	size_t i;
 
 	printf("Testing ecryptfs_extent_to_lwr_pg_idx_and_offset()... ");
 	crypt_stat.extent_size = ECRYPTFS_EXTENT_SIZE;
@@ -122,14 +122,14 @@
 		if (lower_page_idx
 		    != translation_test_vector[i].lower_page_idx) {
 			rc = -1;
-			printf("\nError on test vector entry [%d]; "
+			printf("\nError on test vector entry [%zu]; "
 			       "lower_page_idx = [%lu]\n", i, lower_page_idx);
 			goto out;
 		}
 		if (byte_offset
 		    != translation_test_vector[i].byte_offset) {
 			rc = -1;
-			printf("\nError on test vector entry [%d]; "
+			printf("\nError on test vector entry [%zd]; "
 			       "byte offset = [%d]\n", i, byte_offset);
 			goto out;
 		}
@@ -276,13 +276,13 @@
 	unsigned long extent_offset = 0;
 	unsigned long lower_page_idx = 0;
 	unsigned long prior_lower_page_idx = 0;
+	unsigned int num_extents_per_page;
 	struct page *lower_page;
 	struct inode *lower_inode;
 	struct ecryptfs_crypt_stat *crypt_stat;
 	int rc = 0;
-	int lower_byte_offset;
+	int lower_byte_offset = 0;
 	int orig_byte_offset = 0;
-	int num_extents_per_page;
 #define ECRYPTFS_PAGE_STATE_UNREAD    0
 #define ECRYPTFS_PAGE_STATE_READ      1
 #define ECRYPTFS_PAGE_STATE_MODIFIED  2
@@ -410,7 +410,7 @@
 	int rc = 0;
 	unsigned long lower_size;
 	struct ecryptfs_crypt_stat crypt_stat;
-	int i;
+	size_t i;
 
 	for (i = 0;
 	     i < (sizeof(upper_lower_test_vector)
 
=== modified file 'tests/kernel/directory-concurrent/test.c'
--- tests/kernel/directory-concurrent/test.c	2012-02-02 15:04:32 +0000
+++ tests/kernel/directory-concurrent/test.c	2013-10-28 17:50:31 +0000
@@ -149,7 +149,7 @@
 
 int test_dirs(const char *path, const int max_dirs)
 {
-	int i, j;
+	int i;
 	char *filename;
 	size_t len = strlen(path) + 32;
 	int ret = TEST_PASSED;
 
=== modified file 'tests/kernel/enospc/test.c'
--- tests/kernel/enospc/test.c	2013-10-25 22:51:40 +0000
+++ tests/kernel/enospc/test.c	2013-10-28 17:50:31 +0000
@@ -38,7 +38,6 @@
 int test_exercise(char *filename, off_t size)
 {
 	int fd;
-	struct stat statbuf;
 	int ret = TEST_FAILED;
 	unsigned char buff[BUFF_SZ];
 
 
=== modified file 'tests/kernel/extend-file-random/test.c'
--- tests/kernel/extend-file-random/test.c	2013-10-25 23:14:23 +0000
+++ tests/kernel/extend-file-random/test.c	2013-10-28 17:50:31 +0000
@@ -42,32 +42,46 @@
 
 int test_write(int fd, char *buffer, size_t len, off_t offset)
 {
+	ssize_t rc;
+
 	if (lseek(fd, offset, SEEK_SET) < 0) {
 		fprintf(stderr, "Failed to seek to position %jd: %s\n", 
 			(intmax_t)offset, strerror(errno));
 		return TEST_FAILED;
 	}
 
-	if (write(fd, buffer, len) != len) {
+	rc = write(fd, buffer, len);
+	if (rc < 0) {
 		fprintf(stderr, "Failed to write %zu bytes, position %jd: %s\n",
 			len, (intmax_t)offset, strerror(errno));
 		return TEST_FAILED;
+	} else if (((size_t)rc) < len) {
+		fprintf(stderr, "Failed to write %zu bytes, position %jd: short write of %zd bytes\n",
+			len, (intmax_t)offset, rc);
+		return TEST_FAILED;
 	}
 	return TEST_PASSED;
 }
 
 int test_read(int fd, char *buffer, size_t len, off_t offset)
 {
+	ssize_t rc;
+
 	if (lseek(fd, offset, SEEK_SET) < 0) {
 		fprintf(stderr, "Failed to seek to position %jd: %s\n", 
 			(intmax_t)offset, strerror(errno));
 		return TEST_FAILED;
 	}
 
-	if (read(fd, buffer, len) != len) {
+	rc = read(fd, buffer, len);
+	if (rc < 0) {
 		fprintf(stderr, "Failed to read %zu bytes, position %jd: %s\n", 
 			len, (intmax_t)offset, strerror(errno));
 		return TEST_FAILED;
+	} else if (((size_t)rc) < len) {
+		fprintf(stderr, "Failed to read %zu bytes, position %jd: short read of %zd bytes\n",
+			len, (intmax_t)offset, rc);
+		return TEST_FAILED;
 	}
 	return TEST_PASSED;
 }
 
=== modified file 'tests/kernel/file-concurrent/test.c'
--- tests/kernel/file-concurrent/test.c	2012-03-08 17:31:44 +0000
+++ tests/kernel/file-concurrent/test.c	2013-10-28 17:50:31 +0000
@@ -177,7 +177,7 @@
 
 int test_files(const char *path, const int max_files)
 {
-	int i, j;
+	int i;
 	char *filename;
 	size_t len = strlen(path) + 32;
 	int ret = TEST_PASSED;
 
=== modified file 'tests/kernel/inode-race-stat/test.c'
--- tests/kernel/inode-race-stat/test.c	2013-03-13 14:55:11 +0000
+++ tests/kernel/inode-race-stat/test.c	2013-10-28 17:50:31 +0000
@@ -107,7 +107,6 @@
 {
 	for (;;) {
 		int n;
-		int ret;
 		char cmd[32];
 
 		if ((n = read(fdin, cmd, sizeof(cmd))) < 1) {
@@ -362,9 +361,9 @@
 	cmd[0] = CMD_QUIT;
 	for (i = 0; i < threads; i++) {
 		int status;
-		int ret;
 
-		ret = write(pipe_to[i][1], cmd, 1);
+		if (write(pipe_to[i][1], cmd, 1) != 1)
+			continue;
 		(void)waitpid(pids[i], &status, 0);
 
 		(void)close(pipe_to[i][1]);
 
=== modified file 'tests/kernel/inotify/test.c'
--- tests/kernel/inotify/test.c	2013-06-06 07:58:30 +0000
+++ tests/kernel/inotify/test.c	2013-10-28 17:50:31 +0000
@@ -195,7 +195,7 @@
  *  mk_filename()
  *	simple helper to create a filename
  */
-void inline mk_filename(char *filename, size_t len, const char *path, const char *name)
+inline void mk_filename(char *filename, size_t len, const char *path, const char *name)
 {
 	snprintf(filename, len, "%s/%s", path, name);
 }
 
=== modified file 'tests/kernel/lp-509180/test.c'
--- tests/kernel/lp-509180/test.c	2012-03-01 15:55:22 +0000
+++ tests/kernel/lp-509180/test.c	2013-10-28 17:50:31 +0000
@@ -48,7 +48,6 @@
 	int fd;
 	int opt, flags = 0;
 	int rc = 0;
-	unsigned int *ptr;
 	char *file;
 	unsigned char buffer[1];
 
 
=== modified file 'tests/kernel/trunc-file/test.c'
--- tests/kernel/trunc-file/test.c	2013-07-05 14:56:30 +0000
+++ tests/kernel/trunc-file/test.c	2013-10-28 17:50:31 +0000
@@ -39,7 +39,7 @@
 
 int write_buff(int fd, unsigned char *data, ssize_t size)
 {
-	char *ptr = data;
+	unsigned char *ptr = data;
 	ssize_t n;
 	ssize_t sz = size;
 
@@ -55,7 +55,7 @@
 
 int read_buff(int fd, unsigned char *data, ssize_t size)
 {
-	char *ptr = data;
+	unsigned char *ptr = data;
 	ssize_t n;
 	ssize_t sz = size;
 
@@ -159,9 +159,6 @@
 int test_exercise(char *filename, ssize_t size)
 {
 	int fd;
-	ssize_t i;
-	ssize_t n;
-	ssize_t buflen;
 	int ret = TEST_FAILED;
 	ssize_t trunc_size = size / 2;
 	struct stat statbuf;
@@ -257,8 +254,6 @@
 {
 	ssize_t len = DEFAULT_SIZE;
 	const ssize_t max_len = SSIZE_MAX / 1024;
-	int i;
-	int ret;
 
 	if (argc < 2) {
 		fprintf(stderr, "Syntax: %s filename [size_in_K]\n", argv[0]);
 
=== modified file 'tests/userspace/wrap-unwrap/test.c'
--- tests/userspace/wrap-unwrap/test.c	2013-06-05 18:29:54 +0000
+++ tests/userspace/wrap-unwrap/test.c	2013-10-28 17:50:31 +0000
@@ -19,7 +19,7 @@
 	int decrypted_passphrase_size;
 	char salt[ECRYPTFS_SALT_SIZE + 1];
 	char *path;
-	int i, j;
+	int i;
 	int rc = 0;
 
 	if (argc != 2) {