xz: Use tuklib_mbstr_nonprint

Call tuklib_mask_nonprint() on filenames and also on a few other
strings from the command line too.

The filename printed by "xz --robot --list" (in list.c) is also masked.
It's good to get rid of tabs and newlines which would desync the output
but masking other chars wouldn't be strictly necessary. It might matter
with sensible filenames if LC_CTYPE is "C" (when iswprint() might reject
non-ASCII chars) and a script wants to read a filename from xz's output.
Hopefully it's an unusual enough corner case to not be a real problem.

CMakeLists.txt

@@ -1989,6 +1989,8 @@ if(XZ_TOOL_XZ)
         src/common/sysdefs.h
         src/common/tuklib_common.h
         src/common/tuklib_config.h
+        src/common/tuklib_mbstr_nonprint.c
+        src/common/tuklib_mbstr_nonprint.h
         src/common/tuklib_exit.c
         src/common/tuklib_exit.h
         src/common/tuklib_gettext.h

src/xz/Makefile.am

@@ -33,6 +33,7 @@ xz_SOURCES = \
 	../common/tuklib_progname.c \
 	../common/tuklib_exit.c \
 	../common/tuklib_mbstr_fw.c \
+	../common/tuklib_mbstr_nonprint.c \
 	../common/tuklib_mbstr_width.c \
 	../common/tuklib_mbstr_wrap.c
 

src/xz/coder.c

@@ -1003,7 +1003,8 @@ coder_init(file_pair *pair)
 			strm.avail_out = 0;
 			while ((ret = lzma_code(&strm, LZMA_RUN))
 					== LZMA_UNSUPPORTED_CHECK)
-				message_warning(_("%s: %s"), pair->src_name,
+				message_warning(_("%s: %s"),
+					tuklib_mask_nonprint(pair->src_name),
 					message_strm(ret));
 
 			// With --single-stream lzma_code won't wait for
@@ -1019,7 +1020,9 @@ coder_init(file_pair *pair)
 	}
 
 	if (ret != LZMA_OK) {
-		message_error(_("%s: %s"), pair->src_name, message_strm(ret));
+		message_error(_("%s: %s"),
+				tuklib_mask_nonprint(pair->src_name),
+				message_strm(ret));
 		if (ret == LZMA_MEMLIMIT_ERROR)
 			message_mem_needed(V_ERROR, lzma_memusage(&strm));
 
@@ -1320,10 +1323,12 @@ coder_normal(file_pair *pair)
 			// wrong and we print an error. Otherwise it's just
 			// a warning and coding can continue.
 			if (stop) {
-				message_error(_("%s: %s"), pair->src_name,
+				message_error(_("%s: %s"),
+					tuklib_mask_nonprint(pair->src_name),
 					message_strm(ret));
 			} else {
-				message_warning(_("%s: %s"), pair->src_name,
+				message_warning(_("%s: %s"),
+					tuklib_mask_nonprint(pair->src_name),
 					message_strm(ret));
 
 				// When compressing, all possible errors set

src/xz/file_io.c

@@ -205,8 +205,9 @@ io_wait(file_pair *pair, int timeout, bool is_reading)
 				continue;
 
 			message_error(_("%s: poll() failed: %s"),
-					is_reading ? pair->src_name
+					tuklib_mask_nonprint(is_reading
-						: pair->dest_name,
+						? pair->src_name
+						: pair->dest_name),
 					strerror(errno));
 			return IO_WAIT_ERROR;
 		}
@@ -272,14 +273,15 @@ io_unlink(const char *name, const struct stat *known_st)
 		// of the original file, and in that case it obviously
 		// shouldn't be removed.
 		message_warning(_("%s: File seems to have been moved, "
-				"not removing"), name);
+				"not removing"), tuklib_mask_nonprint(name));
 	else
 #endif
 		// There's a race condition between lstat() and unlink()
 		// but at least we have tried to avoid removing wrong file.
 		if (unlink(name))
 			message_warning(_("%s: Cannot remove: %s"),
-					name, strerror(errno));
+					tuklib_mask_nonprint(name),
+					strerror(errno));
 
 	return;
 }
@@ -305,7 +307,8 @@ io_copy_attrs(const file_pair *pair)
 	if (fchown(pair->dest_fd, pair->src_st.st_uid, (gid_t)(-1))
 			&& warn_fchown)
 		message_warning(_("%s: Cannot set the file owner: %s"),
-				pair->dest_name, strerror(errno));
+				tuklib_mask_nonprint(pair->dest_name),
+				strerror(errno));
 
 	mode_t mode;
 
@@ -318,7 +321,8 @@ io_copy_attrs(const file_pair *pair)
 			&& fchown(pair->dest_fd, (uid_t)(-1),
 				pair->src_st.st_gid)) {
 		message_warning(_("%s: Cannot set the file group: %s"),
-				pair->dest_name, strerror(errno));
+				tuklib_mask_nonprint(pair->dest_name),
+				strerror(errno));
 		// We can still safely copy some additional permissions:
 		// 'group' must be at least as strict as 'other' and
 		// also vice versa.
@@ -337,7 +341,8 @@ io_copy_attrs(const file_pair *pair)
 
 	if (fchmod(pair->dest_fd, mode))
 		message_warning(_("%s: Cannot set the file permissions: %s"),
-				pair->dest_name, strerror(errno));
+				tuklib_mask_nonprint(pair->dest_name),
+				strerror(errno));
 #endif
 
 	// Copy the timestamps. We have several possible ways to do this, of
@@ -515,13 +520,15 @@ io_open_src_real(file_pair *pair)
 	if (!follow_symlinks) {
 		struct stat st;
 		if (lstat(pair->src_name, &st)) {
-			message_error(_("%s: %s"), pair->src_name,
+			message_error(_("%s: %s"),
+					tuklib_mask_nonprint(pair->src_name),
 					strerror(errno));
 			return true;
 
 		} else if (S_ISLNK(st.st_mode)) {
 			message_warning(_("%s: Is a symbolic link, "
-					"skipping"), pair->src_name);
+					"skipping"),
+					tuklib_mask_nonprint(pair->src_name));
 			return true;
 		}
 	}
@@ -583,13 +590,15 @@ io_open_src_real(file_pair *pair)
 
 		if (was_symlink)
 			message_warning(_("%s: Is a symbolic link, "
-					"skipping"), pair->src_name);
+					"skipping"),
+					tuklib_mask_nonprint(pair->src_name));
 		else
 #endif
 			// Something else than O_NOFOLLOW failing
 			// (assuming that the race conditions didn't
 			// confuse us).
-			message_error(_("%s: %s"), pair->src_name,
+			message_error(_("%s: %s"),
+					tuklib_mask_nonprint(pair->src_name),
 					strerror(errno));
 
 		return true;
@@ -612,13 +621,13 @@ io_open_src_real(file_pair *pair)
 
 	if (S_ISDIR(pair->src_st.st_mode)) {
 		message_warning(_("%s: Is a directory, skipping"),
-				pair->src_name);
+				tuklib_mask_nonprint(pair->src_name));
 		goto error;
 	}
 
 	if (reg_files_only && !S_ISREG(pair->src_st.st_mode)) {
 		message_warning(_("%s: Not a regular file, skipping"),
-				pair->src_name);
+				tuklib_mask_nonprint(pair->src_name));
 		goto error;
 	}
 
@@ -636,21 +645,21 @@ io_open_src_real(file_pair *pair)
 			// explicitly in io_copy_attr().
 			message_warning(_("%s: File has setuid or "
 					"setgid bit set, skipping"),
-					pair->src_name);
+					tuklib_mask_nonprint(pair->src_name));
 			goto error;
 		}
 
 		if (pair->src_st.st_mode & S_ISVTX) {
 			message_warning(_("%s: File has sticky bit "
 					"set, skipping"),
-					pair->src_name);
+					tuklib_mask_nonprint(pair->src_name));
 			goto error;
 		}
 
 		if (pair->src_st.st_nlink > 1) {
 			message_warning(_("%s: Input file has more "
-					"than one hard link, "
+					"than one hard link, skipping"),
-					"skipping"), pair->src_name);
+					tuklib_mask_nonprint(pair->src_name));
 			goto error;
 		}
 	}
@@ -679,7 +688,8 @@ io_open_src_real(file_pair *pair)
 	return false;
 
 error_msg:
-	message_error(_("%s: %s"), pair->src_name, strerror(errno));
+	message_error(_("%s: %s"), tuklib_mask_nonprint(pair->src_name),
+			strerror(errno));
 error:
 	(void)close(pair->src_fd);
 	return true;
@@ -816,7 +826,8 @@ io_open_dest_real(file_pair *pair)
 			if (st.st_dev == -1) {
 				message_error("%s: Refusing to write to "
 						"a DOS special file",
-						pair->dest_name);
+						tuklib_mask_nonprint(
+							pair->dest_name));
 				free(pair->dest_name);
 				return true;
 			}
@@ -826,7 +837,8 @@ io_open_dest_real(file_pair *pair)
 					&& st.st_ino == pair->src_st.st_ino) {
 				message_error("%s: Output file is the same "
 						"as the input file",
-						pair->dest_name);
+						tuklib_mask_nonprint(
+							pair->dest_name));
 				free(pair->dest_name);
 				return true;
 			}
@@ -836,7 +848,8 @@ io_open_dest_real(file_pair *pair)
 		// If --force was used, unlink the target file first.
 		if (opt_force && unlink(pair->dest_name) && errno != ENOENT) {
 			message_error(_("%s: Cannot remove: %s"),
-					pair->dest_name, strerror(errno));
+					tuklib_mask_nonprint(pair->dest_name),
+					strerror(errno));
 			free(pair->dest_name);
 			return true;
 		}
@@ -851,7 +864,8 @@ io_open_dest_real(file_pair *pair)
 		pair->dest_fd = open(pair->dest_name, flags, mode);
 
 		if (pair->dest_fd == -1) {
-			message_error(_("%s: %s"), pair->dest_name,
+			message_error(_("%s: %s"),
+					tuklib_mask_nonprint(pair->dest_name),
 					strerror(errno));
 			free(pair->dest_name);
 			return true;
@@ -882,7 +896,7 @@ io_open_dest_real(file_pair *pair)
 	else if (pair->dest_fd != STDOUT_FILENO
 			&& !S_ISREG(pair->dest_st.st_mode)) {
 		message_error("%s: Destination is not a regular file",
-				pair->dest_name);
+				tuklib_mask_nonprint(pair->dest_name));
 
 		// dest_fd needs to be reset to -1 to keep io_close() working.
 		(void)close(pair->dest_fd);
@@ -1005,7 +1019,8 @@ io_close_dest(file_pair *pair, bool success)
 
 	if (close(pair->dest_fd)) {
 		message_error(_("%s: Closing the file failed: %s"),
-				pair->dest_name, strerror(errno));
+				tuklib_mask_nonprint(pair->dest_name),
+				strerror(errno));
 
 		// Closing destination file failed, so we cannot trust its
 		// contents. Get rid of junk:
@@ -1042,7 +1057,8 @@ io_close(file_pair *pair, bool success)
 				SEEK_CUR) == -1) {
 			message_error(_("%s: Seeking failed when trying "
 					"to create a sparse file: %s"),
-					pair->dest_name, strerror(errno));
+					tuklib_mask_nonprint(pair->dest_name),
+					strerror(errno));
 			success = false;
 		} else {
 			const uint8_t zero[1] = { '\0' };
@@ -1141,7 +1157,8 @@ io_read(file_pair *pair, io_buf *buf, size_t size)
 #endif
 
 			message_error(_("%s: Read error: %s"),
-					pair->src_name, strerror(errno));
+					tuklib_mask_nonprint(pair->src_name),
+					strerror(errno));
 
 			return SIZE_MAX;
 		}
@@ -1171,7 +1188,8 @@ io_seek_src(file_pair *pair, uint64_t pos)
 
 	if (lseek(pair->src_fd, (off_t)(pos), SEEK_SET) == -1) {
 		message_error(_("%s: Error seeking the file: %s"),
-				pair->src_name, strerror(errno));
+				tuklib_mask_nonprint(pair->src_name),
+				strerror(errno));
 		return true;
 	}
 
@@ -1195,7 +1213,7 @@ io_pread(file_pair *pair, io_buf *buf, size_t size, uint64_t pos)
 
 	if (amount != size) {
 		message_error(_("%s: Unexpected end of file"),
-				pair->src_name);
+				tuklib_mask_nonprint(pair->src_name));
 		return true;
 	}
 
@@ -1254,7 +1272,8 @@ io_write_buf(file_pair *pair, const uint8_t *buf, size_t size)
 			// user_abort, and get EPIPE here.
 			if (errno != EPIPE)
 				message_error(_("%s: Write error: %s"),
-					pair->dest_name, strerror(errno));
+					tuklib_mask_nonprint(pair->dest_name),
+					strerror(errno));
 
 			return true;
 		}
@@ -1304,7 +1323,9 @@ io_write(file_pair *pair, const io_buf *buf, size_t size)
 					SEEK_CUR) == -1) {
 				message_error(_("%s: Seeking failed when "
 						"trying to create a sparse "
-						"file: %s"), pair->dest_name,
+						"file: %s"),
+						tuklib_mask_nonprint(
+							pair->dest_name),
 						strerror(errno));
 				return true;
 			}

src/xz/list.c

@@ -347,13 +347,14 @@ static bool
 parse_indexes(xz_file_info *xfi, file_pair *pair)
 {
 	if (pair->src_st.st_size <= 0) {
-		message_error(_("%s: File is empty"), pair->src_name);
+		message_error(_("%s: File is empty"),
+				tuklib_mask_nonprint(pair->src_name));
 		return true;
 	}
 
 	if (pair->src_st.st_size < 2 * LZMA_STREAM_HEADER_SIZE) {
 		message_error(_("%s: Too small to be a valid .xz file"),
-				pair->src_name);
+				tuklib_mask_nonprint(pair->src_name));
 		return true;
 	}
 
@@ -365,7 +366,9 @@ parse_indexes(xz_file_info *xfi, file_pair *pair)
 			hardware_memlimit_get(MODE_LIST),
 			(uint64_t)(pair->src_st.st_size));
 	if (ret != LZMA_OK) {
-		message_error(_("%s: %s"), pair->src_name, message_strm(ret));
+		message_error(_("%s: %s"),
+				tuklib_mask_nonprint(pair->src_name),
+				message_strm(ret));
 		return true;
 	}
 
@@ -411,7 +414,8 @@ parse_indexes(xz_file_info *xfi, file_pair *pair)
 		}
 
 		default:
-			message_error(_("%s: %s"), pair->src_name,
+			message_error(_("%s: %s"),
+					tuklib_mask_nonprint(pair->src_name),
 					message_strm(ret));
 
 			// If the error was too low memory usage limit,
@@ -473,7 +477,8 @@ parse_block_header(file_pair *pair, const lzma_index_iter *iter,
 		break;
 
 	case LZMA_OPTIONS_ERROR:
-		message_error(_("%s: %s"), pair->src_name,
+		message_error(_("%s: %s"),
+				tuklib_mask_nonprint(pair->src_name),
 				message_strm(LZMA_OPTIONS_ERROR));
 		return true;
 
@@ -587,7 +592,8 @@ parse_block_header(file_pair *pair, const lzma_index_iter *iter,
 
 	// Check if the stringification succeeded.
 	if (str_ret != LZMA_OK) {
-		message_error(_("%s: %s"), pair->src_name,
+		message_error(_("%s: %s"),
+				tuklib_mask_nonprint(pair->src_name),
 				message_strm(str_ret));
 		return true;
 	}
@@ -596,7 +602,8 @@ parse_block_header(file_pair *pair, const lzma_index_iter *iter,
 
 data_error:
 	// Show the error message.
-	message_error(_("%s: %s"), pair->src_name,
+	message_error(_("%s: %s"),
+			tuklib_mask_nonprint(pair->src_name),
 			message_strm(LZMA_DATA_ERROR));
 	return true;
 }
@@ -744,7 +751,7 @@ print_info_basic(const xz_file_info *xfi, file_pair *pair)
 	char checks[CHECKS_STR_SIZE];
 	get_check_names(checks, lzma_index_checks(xfi->idx), false);
 
-	const char *cols[7] = {
+	const char *cols[6] = {
 		uint64_to_str(lzma_index_stream_count(xfi->idx), 0),
 		uint64_to_str(lzma_index_block_count(xfi->idx), 1),
 		uint64_to_nicestr(lzma_index_file_size(xfi->idx),
@@ -754,7 +761,6 @@ print_info_basic(const xz_file_info *xfi, file_pair *pair)
 		get_ratio(lzma_index_file_size(xfi->idx),
 			lzma_index_uncompressed_size(xfi->idx)),
 		checks,
-		pair->src_name,
 	};
 	printf("%*s %*s  %*s  %*s  %*s  %-*s %s\n",
 			tuklib_mbstr_fw(cols[0], 5), cols[0],
@@ -763,7 +769,7 @@ print_info_basic(const xz_file_info *xfi, file_pair *pair)
 			tuklib_mbstr_fw(cols[3], 11), cols[3],
 			tuklib_mbstr_fw(cols[4], 5), cols[4],
 			tuklib_mbstr_fw(cols[5], 7), cols[5],
-			cols[6]);
+			tuklib_mask_nonprint(pair->src_name));
 
 	return false;
 }
@@ -1048,7 +1054,11 @@ print_info_robot(xz_file_info *xfi, file_pair *pair)
 	char checks[CHECKS_STR_SIZE];
 	get_check_names(checks, lzma_index_checks(xfi->idx), false);
 
-	printf("name\t%s\n", pair->src_name);
+	// Robot mode has to mask at least some control chars to prevent
+	// the output from getting out of sync if filename is malicious.
+	// Masking all non-printable chars is more than we need but
+	// perhaps this is good enough in practice.
+	printf("name\t%s\n", tuklib_mask_nonprint(pair->src_name));
 
 	printf("file\t%" PRIu64 "\t%" PRIu64 "\t%" PRIu64 "\t%" PRIu64
 			"\t%s\t%s\t%" PRIu64 "\n",

src/xz/main.c

@@ -87,7 +87,8 @@ read_name(const args_info *args)
 				continue;
 
 			message_error(_("%s: Error reading filenames: %s"),
-					args->files_name, strerror(errno));
+				tuklib_mask_nonprint(args->files_name),
+				strerror(errno));
 			return NULL;
 		}
 
@@ -95,7 +96,8 @@ read_name(const args_info *args)
 			if (pos != 0)
 				message_error(_("%s: Unexpected end of input "
 						"when reading filenames"),
-						args->files_name);
+						tuklib_mask_nonprint(
+							args->files_name));
 
 			return NULL;
 		}
@@ -120,7 +122,9 @@ read_name(const args_info *args)
 			message_error(_("%s: Null character found when "
 					"reading filenames; maybe you meant "
 					"to use '--files0' instead "
-					"of '--files'?"), args->files_name);
+					"of '--files'?"),
+					tuklib_mask_nonprint(
+						args->files_name));
 			return NULL;
 		}
 

src/xz/message.c

@@ -196,10 +196,12 @@ print_filename(void)
 		// If we don't know how many files there will be due
 		// to usage of --files or --files0.
 		if (files_total == 0)
-			fprintf(file, "%s (%u)\n", filename,
+			fprintf(file, "%s (%u)\n",
+					tuklib_mask_nonprint(filename),
 					files_pos);
 		else
-			fprintf(file, "%s (%u/%u)\n", filename,
+			fprintf(file, "%s (%u/%u)\n",
+					tuklib_mask_nonprint(filename),
 					files_pos, files_total);
 
 		signals_unblock();
@@ -648,7 +650,7 @@ progress_flush(bool finished)
 				cols[4]);
 	} else {
 		// The filename is always printed.
-		fprintf(stderr, _("%s: "), filename);
+		fprintf(stderr, _("%s: "), tuklib_mask_nonprint(filename));
 
 		// Percentage is printed only if we didn't finish yet.
 		if (!finished) {

src/xz/options.c

@@ -83,14 +83,15 @@ parse_options(const char *str, const option_map *opts,
 
 		if (value == NULL || value[0] == '\0')
 			message_fatal(_("%s: Options must be 'name=value' "
-					"pairs separated with commas"), str);
+					"pairs separated with commas"),
+					tuklib_mask_nonprint(str));
 
 		// Look for the option name from the option map.
 		unsigned i = 0;
 		while (true) {
 			if (opts[i].name == NULL)
 				message_fatal(_("%s: Invalid option name"),
-						name);
+						tuklib_mask_nonprint(name));
 
 			if (strcmp(name, opts[i].name) == 0)
 				break;
@@ -110,7 +111,7 @@ parse_options(const char *str, const option_map *opts,
 
 			if (opts[i].map[j].name == NULL)
 				message_fatal(_("%s: Invalid option value"),
-						value);
+						tuklib_mask_nonprint(value));
 
 			set(filter_options, i, opts[i].map[j].id, value);
 
@@ -244,7 +245,8 @@ tuklib_attr_noreturn
 static void
 error_lzma_preset(const char *valuestr)
 {
-	message_fatal(_("Unsupported LZMA1/LZMA2 preset: %s"), valuestr);
+	message_fatal(_("Unsupported LZMA1/LZMA2 preset: %s"),
+			tuklib_mask_nonprint(valuestr));
 }
 
 

src/xz/private.h

@@ -28,6 +28,7 @@
 #include "tuklib_gettext.h"
 #include "tuklib_progname.h"
 #include "tuklib_exit.h"
+#include "tuklib_mbstr_nonprint.h"
 #include "tuklib_mbstr.h"
 
 #if defined(_WIN32) && !defined(__CYGWIN__)

src/xz/suffix.c

@@ -163,7 +163,7 @@ uncompressed_name(const char *src_name, const size_t src_len)
 
 	if (new_len == 0) {
 		message_warning(_("%s: Filename has an unknown suffix, "
-				"skipping"), src_name);
+				"skipping"), tuklib_mask_nonprint(src_name));
 		return NULL;
 	}
 
@@ -178,13 +178,14 @@ uncompressed_name(const char *src_name, const size_t src_len)
 }
 
 
-/// This message is needed in multiple places in compressed_name(),
-/// so the message has been put into its own function.
 static void
 msg_suffix(const char *src_name, const char *suffix)
 {
+	char *mem = NULL;
 	message_warning(_("%s: File already has '%s' suffix, skipping"),
-			src_name, suffix);
+			tuklib_mask_nonprint(src_name),
+			tuklib_mask_nonprint_r(suffix, &mem));
+	free(mem);
 	return;
 }
 
@@ -390,7 +391,8 @@ suffix_set(const char *suffix)
 	// Empty suffix and suffixes having a directory separator are
 	// rejected. Such suffixes would break things later.
 	if (suffix[0] == '\0' || has_dir_sep(suffix))
-		message_fatal(_("%s: Invalid filename suffix"), suffix);
+		message_fatal(_("%s: Invalid filename suffix"),
+				tuklib_mask_nonprint(suffix));
 
 	// Replace the old custom_suffix (if any) with the new suffix.
 	free(custom_suffix);