A bunch of style and security fixes (error checking return values, etc),

mostly from: Tim J Robbins

4 files changed, 70 insertions, 35 deletions

diff --git a/usr.bin/tar/bsdtar.c b/usr.bin/tar/bsdtar.c
index c35fa78..89dc5ad 100644
--- a/usr.bin/tar/bsdtar.c
+++ b/usr.bin/tar/bsdtar.c
@@ -225,7 +225,9 @@ main(int argc, char **argv)
 			mode = opt;
 			break;
 		case OPTION_EXCLUDE: /* GNU tar */
-			exclude(bsdtar, optarg);
+			if (exclude(bsdtar, optarg))
+				bsdtar_errc(bsdtar, 1, 0,
+				    "Couldn't exclude %s\n", optarg);
 			break;
 		case 'F':
 			bsdtar->create_format = optarg;
@@ -251,7 +253,10 @@ main(int argc, char **argv)
 			exit(0);
 			break;
 		case OPTION_INCLUDE:
-			include(bsdtar, optarg);
+			if (include(bsdtar, optarg))
+				bsdtar_errc(bsdtar, 1, 0,
+				    "Failed to add %s to inclusion list",
+				    optarg);
 			break;
 		case 'j': /* GNU tar */
 			if (bsdtar->create_compression != '\0')
@@ -354,7 +359,10 @@ main(int argc, char **argv)
 			bsdtar->option_interactive = 1;
 			break;
 		case 'X': /* GNU tar */
-			exclude_from_file(bsdtar, optarg);
+			if (exclude_from_file(bsdtar, optarg))
+				bsdtar_errc(bsdtar, 1, 0,
+				    "failed to process exclusions from file %s",
+				    optarg);
 			break;
 		case 'x': /* SUSv2 */
 			if (mode != '\0')
@@ -392,7 +400,7 @@ main(int argc, char **argv)
 	/*
 	 * Sanity-check options.
 	 */
-	if (mode == '\0' && possible_help_request) {
+	if ((mode == '\0') && possible_help_request) {
 		long_help(bsdtar);
 		exit(0);
 	}
@@ -658,7 +666,7 @@ bsdtar_getopt(struct bsdtar *bsdtar, const char *optstring,
 		p = optarg;
 		q = strchr(optarg, '=');
 		if (q != NULL) {
-			option_length = q - p;
+			option_length = (size_t)(q - p);
 			optarg = q + 1;
 		} else {
 			option_length = strlen(p);
diff --git a/usr.bin/tar/read.c b/usr.bin/tar/read.c
index 2c71a76..03dd4df 100644
--- a/usr.bin/tar/read.c
+++ b/usr.bin/tar/read.c
@@ -233,15 +233,14 @@ list_item_verbose(struct bsdtar *bsdtar, struct archive_entry *entry)
 
 	/* Use uname if it's present, else uid. */
 	p = archive_entry_uname(entry);
-	if (p && *p) {
-		sprintf(tmp, "%s ", p);
-	} else {
+	if ((p == NULL) || (*p == '\0')) {
 		sprintf(tmp, "%d ", st->st_uid);
+		p = tmp;
 	}
-	w = strlen(tmp);
+	w = strlen(p);
 	if (w > bsdtar->u_width)
 		bsdtar->u_width = w;
-	fprintf(out, "%-*s", (int)bsdtar->u_width, tmp);
+	fprintf(out, "%-*s", (int)bsdtar->u_width, p);
 
 	/* Use gname if it's present, else gid. */
 	p = archive_entry_gname(entry);
@@ -260,7 +259,9 @@ list_item_verbose(struct bsdtar *bsdtar, struct archive_entry *entry)
 	 * If gs_width is too small, grow it.
 	 */
 	if (S_ISCHR(st->st_mode) || S_ISBLK(st->st_mode)) {
-		sprintf(tmp, "%u,%u", major(st->st_rdev), minor(st->st_rdev));
+		sprintf(tmp, "%d,%u",
+		    major(st->st_rdev),
+		    (unsigned)minor(st->st_rdev)); /* ls(1) also casts here. */
 	} else {
 		/*
 		 * Note the use of platform-dependent macros to format
@@ -365,6 +366,8 @@ security_problem(struct bsdtar *bsdtar, struct archive_entry *entry)
 		while (strlen(name) >= bsdtar->security->path_size)
 			bsdtar->security->path_size *= 2;
 		bsdtar->security->path = malloc(bsdtar->security->path_size);
+		if (bsdtar->security->path == NULL)
+			bsdtar_errc(bsdtar, 1, errno, "No Memory");
 	}
 	p = bsdtar->security->path;
 	while (pn != NULL && pn[0] != '\0') {
diff --git a/usr.bin/tar/util.c b/usr.bin/tar/util.c
index d0ea39e..48e30c2 100644
--- a/usr.bin/tar/util.c
+++ b/usr.bin/tar/util.c
@@ -169,10 +169,12 @@ yes(const char *fmt, ...)
 	fflush(stderr);
 
 	l = read(2, buff, sizeof(buff));
+	if (l <= 0)
+		return (0);
 	buff[l] = 0;
 
 	for (p = buff; *p != '\0'; p++) {
-		if (isspace(*p))
+		if (isspace(0xff & (int)*p))
 			continue;
 		switch(*p) {
 		case 'y': case 'Y':
@@ -301,7 +303,7 @@ process_lines(struct bsdtar *bsdtar, const char *pathname,
 		} else {
 			/* Line is too big; enlarge the buffer. */
 			p = realloc(buff, buff_length *= 2);
-			if (buff == NULL)
+			if (p == NULL)
 				bsdtar_errc(bsdtar, 1, ENOMEM,
 				    "Line too long in %s", pathname);
 			buff_end = p + (buff_end - buff);
diff --git a/usr.bin/tar/write.c b/usr.bin/tar/write.c
index 787b734..6495e3d 100644
--- a/usr.bin/tar/write.c
+++ b/usr.bin/tar/write.c
@@ -141,16 +141,17 @@ tar_mode_c(struct bsdtar *bsdtar)
 	a = archive_write_new();
 
 	/* Support any format that the library supports. */
-	if (bsdtar->create_format == NULL)
-		archive_write_set_format_pax_restricted(a);
-	else {
+	if (bsdtar->create_format == NULL) {
+		r = archive_write_set_format_pax_restricted(a);
+		bsdtar->create_format = "pax restricted";
+	} else {
 		r = archive_write_set_format_by_name(a, bsdtar->create_format);
-		if (r != ARCHIVE_OK) {
-			fprintf(stderr, "Can't use format %s: %s\n",
-			    bsdtar->create_format,
-			    archive_error_string(a));
-			usage(bsdtar);
-		}
+	}
+	if (r != ARCHIVE_OK) {
+		fprintf(stderr, "Can't use format %s: %s\n",
+		    bsdtar->create_format,
+		    archive_error_string(a));
+		usage(bsdtar);
 	}
 
 	/*
@@ -242,10 +243,10 @@ tar_mode_r(struct bsdtar *bsdtar)
 	if (format == ARCHIVE_FORMAT_TAR_GNUTAR)
 		format = ARCHIVE_FORMAT_TAR_USTAR;
 	archive_write_set_format(a, format);
-	lseek(bsdtar->fd, end_offset, SEEK_SET);
-	archive_write_open_fd(a, bsdtar->fd);
+	lseek(bsdtar->fd, end_offset, SEEK_SET); /* XXX check return val XXX */
+	archive_write_open_fd(a, bsdtar->fd); /* XXX check return val XXX */
 
-	write_archive(a, bsdtar);
+	write_archive(a, bsdtar); /* XXX check return val XXX */
 
 	archive_write_finish(a);
 	close(bsdtar->fd);
@@ -400,7 +401,7 @@ write_archive(struct archive *a, struct bsdtar *bsdtar)
 			 * directories; such requests will only fail
 			 * if the directory must be accessed.
 			 */
-			if (pending_dir && *arg == '/') {
+			if (pending_dir != NULL && *arg == '/') {
 				/* The -C /foo -C /bar case; dump first one. */
 				free(pending_dir);
 				pending_dir = NULL;
@@ -408,7 +409,7 @@ write_archive(struct archive *a, struct bsdtar *bsdtar)
 			if (pending_dir) {
 				/* The -C /foo -C bar case; concatenate */
 				char *old_pending = pending_dir;
-				int old_len = strlen(old_pending);
+				size_t old_len = strlen(old_pending);
 
 				pending_dir =
 				    malloc(old_len + 1 + strlen(arg));
@@ -433,7 +434,7 @@ write_archive(struct archive *a, struct bsdtar *bsdtar)
 			if (pending_dir != NULL &&
 			    (*arg != '/' || (*arg == '@' && arg[1] != '/'))) {
 				/* Handle a deferred -C */
-				if (chdir(pending_dir)) {
+				if (chdir(pending_dir) != 0) {
 					bsdtar_warnc(bsdtar, 0,
 					    "could not chdir to '%s'\n",
 					    pending_dir);
@@ -445,7 +446,7 @@ write_archive(struct archive *a, struct bsdtar *bsdtar)
 			}
 
 			if (*arg == '@') {
-				if (append_archive(bsdtar, a, arg+1))
+				if (append_archive(bsdtar, a, arg + 1) != 0)
 					break;
 			} else
 				write_heirarchy(bsdtar, a, arg);
@@ -582,6 +583,8 @@ write_heirarchy(struct bsdtar *bsdtar, struct archive *a, const char *path)
 	 * copy 'path' to mutable storage.
 	 */
 	fts_argv[0] = strdup(path);
+	if (fts_argv[0] == NULL)
+		bsdtar_errc(bsdtar, 1, ENOMEM, "Can't open %s", path);
 	fts_argv[1] = NULL;
 	ftsoptions = FTS_PHYSICAL;
 	switch (bsdtar->symlink_mode) {
@@ -794,9 +797,9 @@ write_entry(struct bsdtar *bsdtar, struct archive *a, struct stat *st,
 		st->st_size = 0;
 
 	/* Strip redundant "./" from start of filename. */
-	if (pathname && pathname[0] == '.' && pathname[1] == '/') {
+	if (pathname != NULL && pathname[0] == '.' && pathname[1] == '/') {
 		pathname += 2;
-		if (*pathname == 0)	/* This is the "./" directory. */
+		if (*pathname == '\0')	/* This is the "./" directory. */
 			goto cleanup;	/* Don't archive it ever. */
 	}
 
@@ -1080,12 +1083,18 @@ lookup_hardlink(struct bsdtar *bsdtar, struct archive_entry *entry,
 
 	/* Add this entry to the links cache. */
 	le = malloc(sizeof(struct links_entry));
-	if (le == NULL) {
+	if (le != NULL)
+		le->name = strdup(archive_entry_pathname(entry));
+	if ((le == NULL) || (le->name == NULL)) {
+		/* TODO: Just flush the entire links cache when we
+		 * run out of memory; don't hold onto anything. */
 		links_cache->stop_allocating = 1;
 		bsdtar_warnc(bsdtar, ENOMEM,
 		    "No more memory for recording hard links");
 		bsdtar_warnc(bsdtar, 0,
 		    "Remaining hard links will be dumped as full files");
+		if (le != NULL)
+			free(le);
 		return;
 	}
 	if (links_cache->buckets[hash] != NULL)
@@ -1097,7 +1106,6 @@ lookup_hardlink(struct bsdtar *bsdtar, struct archive_entry *entry,
 	le->dev = st->st_dev;
 	le->ino = st->st_ino;
 	le->links = st->st_nlink - 1;
-	le->name = strdup(archive_entry_pathname(entry));
 }
 
 #ifdef HAVE_POSIX_ACL
@@ -1218,6 +1226,8 @@ lookup_name(struct bsdtar *bsdtar, struct name_cache **name_cache_variable,
 
 	if (*name_cache_variable == NULL) {
 		*name_cache_variable = malloc(sizeof(struct name_cache));
+		if (*name_cache_variable == NULL)
+			bsdtar_errc(bsdtar, 1, ENOMEM, "No more memory");
 		memset(*name_cache_variable, 0, sizeof(struct name_cache));
 		(*name_cache_variable)->size = name_cache_size;
 	}
@@ -1245,8 +1255,15 @@ lookup_name(struct bsdtar *bsdtar, struct name_cache **name_cache_variable,
 			cache->cache[slot].id = id;
 		} else {
 			cache->cache[slot].name = strdup(name);
-			cache->cache[slot].id = id;
-			return (cache->cache[slot].name);
+			if (cache->cache[slot].name != NULL) {
+				cache->cache[slot].id = id;
+				return (cache->cache[slot].name);
+			}
+			/*
+			 * Conveniently, NULL marks an empty slot, so
+			 * if the strdup() fails, we've just failed to
+			 * cache it.  No recovery necessary.
+			 */
 		}
 	}
 	return (NULL);
@@ -1361,7 +1378,12 @@ add_dir_list(struct bsdtar *bsdtar, const char *path,
 	}
 
 	p = malloc(sizeof(*p));
+	if (p == NULL)
+		bsdtar_errc(bsdtar, 1, ENOMEM, "Can't read archive directory");
+
 	p->name = strdup(path);
+	if (p->name == NULL)
+		bsdtar_errc(bsdtar, 1, ENOMEM, "Can't read archive directory");
 	p->mtime_sec = mtime_sec;
 	p->mtime_nsec = mtime_nsec;
 	p->next = NULL;