commit 509d815465eb6e0ce6f357d92c59025290798021
Author:     Jacob Welsh <jfw>
AuthorDate: Tue Apr 23 18:35:33 2024 +0000
Commit:     Jacob Welsh <jfw>
CommitDate: Tue Apr 23 18:53:19 2024 +0000

    busybox/archival(tar,cpio): don't die on first extraction error
    
    This more closely follows the behavior of GNU tar, and uses the same deferred failure message on exit, as this is all helpful to the user and costs little.
    
    In the other relevant applets (ar,rpm,dpkg) the calling code is more complex and it's not clear to me if deferred exit is desirable, so preserving their prior behavior for now.

diff --git a/base/busybox/archival/cpio.c b/base/busybox/archival/cpio.c
index cdc16c1..0615227 100644
--- a/base/busybox/archival/cpio.c
+++ b/base/busybox/archival/cpio.c
@@ -486,5 +486,10 @@ int cpio_main(int argc UNUSED_PARAM, char **argv)
 		fprintf(stderr, "%"OFF_FMT"u blocks\n", archive_handle->cpio__blocks);
 	}
 
+	if (archive_handle->status != EXIT_SUCCESS) {
+		bb_error_msg("Exiting with failure status due to previous errors");
+		return archive_handle->status;
+	}
+
 	return EXIT_SUCCESS;
 }
diff --git a/base/busybox/archival/libarchive/data_extract_all.c b/base/busybox/archival/libarchive/data_extract_all.c
index 065c35c..0dde95f 100644
--- a/base/busybox/archival/libarchive/data_extract_all.c
+++ b/base/busybox/archival/libarchive/data_extract_all.c
@@ -9,8 +9,6 @@
 void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 {
 	file_header_t *file_header = archive_handle->file_header;
-	int dst_fd;
-	int res;
 	int restore_parent_times = 0;
 	struct timespec parent_times[2];
 	char *slash = 0;
@@ -35,15 +33,17 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 		*slash = '\0';
 		if (lstat(file_header->name, &st) == -1) {
 			if (errno != ENOENT) {
-				bb_perror_msg_and_die("can't stat prefix %s", file_header->name);
+				bb_perror_msg("can't stat prefix %s", file_header->name);
+				goto abort;
 			}
 			if (!(archive_handle->ah_flags & ARCHIVE_CREATE_LEADING_DIRS)) {
 				/* Prefix doesn't exist but we weren't asked to create. (Could probably ignore this and let whatever subsequent syscall catch the problem.) */
-				bb_simple_perror_msg_and_die(file_header->name);
+				bb_simple_perror_msg(file_header->name);
+				goto abort;
 			}
 			if (mkdir(file_header->name, 0777) == -1) {
-				bb_perror_msg_and_die("can't create prefix %s",
-					file_header->name);
+				bb_perror_msg("can't create prefix %s", file_header->name);
+				goto abort;
 			}
 			if (restore_parent_times) {
 				*lastslash = '\0';
@@ -56,7 +56,8 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 			/* Prefix exists */
 			if (!S_ISDIR(st.st_mode)) {
 				/* Don't defer catching this to a later syscall, as it could be a directory symlink, which would lead us to successfully write outside the designated path (symlink attack)! */
-				bb_error_msg_and_die("prefix not a directory: %s", file_header->name);
+				bb_error_msg("prefix not a directory: %s", file_header->name);
+				goto abort;
 			}
 			if (archive_handle->ah_flags & ARCHIVE_RESTORE_DATE) {
 				/* We were asked to preserve archive timestamps; but directory mtimes are bumped by creating/renaming/unlinking anything inside, so we need to restore them afterward. It might be preferred to do this only for directories explicitly created by the archive; but without maintaining a space-consuming list of those, there's no way to distinguish. */
@@ -95,8 +96,8 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 		 && errno != ENOENT
 		 && !(errno == EISDIR && S_ISDIR(file_header->mode))
 		) {
-			bb_perror_msg_and_die("can't remove old file %s",
-					file_header->name);
+			bb_perror_msg("can't remove old file %s", file_header->name);
+			goto abort;
 		}
 	}
 	else if (archive_handle->ah_flags & (ARCHIVE_EXTRACT_NEWER | ARCHIVE_O_TRUNC)) {
@@ -104,8 +105,8 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 		struct stat existing_sb;
 		if (lstat(file_header->name, &existing_sb) == -1) {
 			if (errno != ENOENT) {
-				bb_perror_msg_and_die("can't stat old file %s",
-						file_header->name);
+				bb_perror_msg("can't stat old file %s", file_header->name);
+				goto abort;
 			}
 			/* Doesn't exist, no problem */
 		} else if (archive_handle->ah_flags & ARCHIVE_EXTRACT_NEWER) {
@@ -127,16 +128,17 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 			 && errno != ENOENT
 			 && !(errno == EISDIR && S_ISDIR(file_header->mode))
 			) {
-				bb_perror_msg_and_die("can't remove old file %s",
-						file_header->name);
+				bb_perror_msg("can't remove old file %s", file_header->name);
+				goto abort;
 			}
 		} else { /* ARCHIVE_O_TRUNC mode */
 			/* A directory may naturally block the overwrite, but not all special file types will; especially do NOT try writing into a symlink, fifo or device node! */
 			if (!S_ISREG(existing_sb.st_mode)
 			 && !(S_ISDIR(existing_sb.st_mode) && S_ISDIR(file_header->mode))
 			) {
-				bb_error_msg_and_die("can't overwrite non-regular file %s",
+				bb_error_msg("can't overwrite non-regular file %s",
 						file_header->name);
+				goto abort;
 			}
 			/* Regular file, no problem */
 		}
@@ -151,12 +153,11 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 
 		/* We encode hard links as regular files of size 0 with a symlink */
 		if (file_header->link_target && file_header->size == 0) {
-			res = link(file_header->link_target, file_header->name);
-			if ((res == -1) && !(archive_handle->ah_flags & ARCHIVE_EXTRACT_QUIET)) {
-				bb_perror_msg("can't create %slink "
-						"from %s to %s", "hard",
+			if (link(file_header->link_target, file_header->name) == -1) {
+				bb_perror_msg("can't create hardlink from %s to %s",
 						file_header->name,
 						file_header->link_target);
+				goto abort;
 			}
 			/* Hardlinks have no separate mode/ownership, skip chown/chmod */
 			goto ret;
@@ -170,64 +171,88 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 #ifdef ARCHIVE_REPLACE_VIA_RENAME
 		if (archive_handle->ah_flags & ARCHIVE_REPLACE_VIA_RENAME)
 			/* rpm-style temp file name */
+			/* XXX not guaranteed unique... maybe force O_EXCL or use mkstemp */
 			dst_name = xasprintf("%s;%x", dst_name, (int)getpid());
 #endif
-		dst_fd = xopen3(dst_name,
-			flags,
-			file_header->mode
-			);
-		bb_copyfd_exact_size(archive_handle->src_fd, dst_fd, file_header->size);
-		close(dst_fd);
+		{
+			int dst_fd;
+			off_t sz;
+
+			dst_fd = open(dst_name, flags, file_header->mode);
+			if (dst_fd < 0) {
+				bb_perror_msg("can't open '%s'", dst_name);
+				goto abort;
+			}
+
+			/* As in bb_copyfd_exact_size but without the xfunc_die(). */
+			/* XXX are we sure size > 0? */
+			sz = bb_copyfd_size(archive_handle->src_fd, dst_fd,
+					file_header->size);
+			if (sz != file_header->size) {
+				if (sz != -1) {
+					bb_error_msg("short read");
+				}
+				/* if sz == -1, bb_copyfd_XX already complained */
+				close(dst_fd);
+				goto abort;
+			}
+			close(dst_fd);
+		}
 #ifdef ARCHIVE_REPLACE_VIA_RENAME
 		if (archive_handle->ah_flags & ARCHIVE_REPLACE_VIA_RENAME) {
-			xrename(dst_name, file_header->name);
+			if (rename(dst_name, file_header->name)) {
+				bb_perror_msg("can't move '%s' to '%s'", dst_name,
+						file_header->name);
+				free(dst_name);
+				/* Don't goto abort: we already modified the parent directory so may need to restore its times. */
+				archive_handle->status = EXIT_FAILURE;
+				goto ret;
+			}
 			free(dst_name);
 		}
 #endif
 		break;
 	}
 	case S_IFDIR:
-		res = mkdir(file_header->name, file_header->mode);
-		if ((res == -1)
+		if ((mkdir(file_header->name, file_header->mode) == -1)
 		 && (errno != EISDIR) /* btw, Linux doesn't return this */
 		 && (errno != EEXIST)
-		 && !(archive_handle->ah_flags & ARCHIVE_EXTRACT_QUIET)
 		) {
 			bb_perror_msg("can't make dir %s", file_header->name);
+			goto abort;
 		}
 		/* TODO maybe warn also if it exists and ISN'T a dir. Due to the initial path check, this doesn't by itself open a symlink attack, but it does mean we failed to reproduce the archive structure. (This can happen when neither ARCHIVE_UNLINK_OLD nor ARCHIVE_O_TRUNC is set, for instance with tar -k.) */
 		break;
 	case S_IFLNK:
 		/* Symlink */
 		if (file_header->link_target) {
-			res = symlink(file_header->link_target, file_header->name);
-			if ((res == -1)
-			 && !(archive_handle->ah_flags & ARCHIVE_EXTRACT_QUIET)
-			) {
+			if (symlink(file_header->link_target, file_header->name) == -1) {
 				bb_perror_msg("can't create %slink "
 						"from %s to %s", "sym",
 						file_header->name,
 						file_header->link_target);
+				goto abort;
 			}
 		} else {
 			bb_perror_msg("inconsistent file header: "
 					"symlink without target %s",
 					file_header->name);
+			goto abort;
 		}
 		break;
 	case S_IFSOCK:
 	case S_IFBLK:
 	case S_IFCHR:
 	case S_IFIFO:
-		res = mknod(file_header->name, file_header->mode, file_header->device);
-		if ((res == -1)
-		 && !(archive_handle->ah_flags & ARCHIVE_EXTRACT_QUIET)
-		) {
+		if (mknod(file_header->name, file_header->mode,
+					file_header->device) == -1) {
 			bb_perror_msg("can't create node %s", file_header->name);
+			goto abort;
 		}
 		break;
 	default:
-		bb_error_msg_and_die("unrecognized file type");
+		bb_error_msg("unrecognized file type for %s", file_header->name);
+		goto abort;
 	}
 
 	if (!S_ISLNK(file_header->mode)) {
@@ -281,4 +306,13 @@ void FAST_FUNC data_extract_all(archive_handle_t *archive_handle)
 		setfscreatecon(NULL);
 	}
 #endif
+	return;
+
+abort:
+	if (slash) *slash = '/';
+	if (lastslash) *lastslash = '/';
+#if ENABLE_FEATURE_TAR_SELINUX
+	if (sctx) setfscreatecon(NULL);
+#endif
+	archive_handle->status = EXIT_FAILURE;
 }
diff --git a/base/busybox/archival/libarchive/get_header_ar.c b/base/busybox/archival/libarchive/get_header_ar.c
index aed31d4..a800f9e 100644
--- a/base/busybox/archival/libarchive/get_header_ar.c
+++ b/base/busybox/archival/libarchive/get_header_ar.c
@@ -130,8 +130,11 @@ char FAST_FUNC get_header_ar(archive_handle_t *archive_handle)
 		archive_handle->action_header(typed);
 #if ENABLE_DPKG || ENABLE_DPKG_DEB
 		if (archive_handle->dpkg__sub_archive) {
-			while (archive_handle->dpkg__action_data_subarchive(archive_handle->dpkg__sub_archive) == EXIT_SUCCESS)
-				continue;
+			while (archive_handle->dpkg__action_data_subarchive(archive_handle->dpkg__sub_archive) == EXIT_SUCCESS) {
+				if (archive_handle->dpkg__sub_archive->status != EXIT_SUCCESS) {
+					xfunc_die();
+				}
+			}
 		} else
 #endif
 			archive_handle->action_data(archive_handle);
diff --git a/base/busybox/archival/libarchive/init_handle.c b/base/busybox/archival/libarchive/init_handle.c
index cbae06a..3d97676 100644
--- a/base/busybox/archival/libarchive/init_handle.c
+++ b/base/busybox/archival/libarchive/init_handle.c
@@ -17,6 +17,7 @@ archive_handle_t* FAST_FUNC init_handle(void)
 	archive_handle->action_data = data_skip;
 	archive_handle->filter = filter_accept_all;
 	archive_handle->seek = seek_by_jump;
+	archive_handle->status = EXIT_SUCCESS;
 
 	return archive_handle;
 }
diff --git a/base/busybox/archival/libarchive/unpack_ar_archive.c b/base/busybox/archival/libarchive/unpack_ar_archive.c
index 0bc0303..c226b82 100644
--- a/base/busybox/archival/libarchive/unpack_ar_archive.c
+++ b/base/busybox/archival/libarchive/unpack_ar_archive.c
@@ -17,6 +17,9 @@ void FAST_FUNC unpack_ar_archive(archive_handle_t *ar_archive)
 	}
 	ar_archive->offset += AR_MAGIC_LEN;
 
-	while (get_header_ar(ar_archive) == EXIT_SUCCESS)
-		continue;
+	while (get_header_ar(ar_archive) == EXIT_SUCCESS) {
+		if (ar_archive->status != EXIT_SUCCESS) {
+			xfunc_die();
+		}
+	}
 }
diff --git a/base/busybox/archival/rpm.c b/base/busybox/archival/rpm.c
index 1053944..504dde4 100644
--- a/base/busybox/archival/rpm.c
+++ b/base/busybox/archival/rpm.c
@@ -123,8 +123,11 @@ static void extract_cpio(int fd, const char *source_rpm)
 	/*archive_handle->offset = 0; - init_handle() did it */
 
 	setup_unzip_on_fd(archive_handle->src_fd, /*fail_if_not_compressed:*/ 1);
-	while (get_header_cpio(archive_handle) == EXIT_SUCCESS)
-		continue;
+	while (get_header_cpio(archive_handle) == EXIT_SUCCESS) {
+		if (archive_handle->status != EXIT_SUCCESS) {
+			xfunc_die();
+		}
+	}
 }
 
 static rpm_index **rpm_gettags(int fd, int *num_tags)
diff --git a/base/busybox/archival/tar.c b/base/busybox/archival/tar.c
index 9243ae7..f1cdbc5 100644
--- a/base/busybox/archival/tar.c
+++ b/base/busybox/archival/tar.c
@@ -1202,5 +1202,10 @@ int tar_main(int argc UNUSED_PARAM, char **argv)
 		check_errors_in_children(0);
 	}
 
+	if (tar_handle->status != EXIT_SUCCESS) {
+		bb_error_msg("Exiting with failure status due to previous errors");
+		return tar_handle->status;
+	}
+
 	return bb_got_signal;
 }
diff --git a/base/busybox/include/bb_archive.h b/base/busybox/include/bb_archive.h
index f82e2fc..e59ea8a 100644
--- a/base/busybox/include/bb_archive.h
+++ b/base/busybox/include/bb_archive.h
@@ -77,6 +77,9 @@ typedef struct archive_handle_t {
 	/* Count processed bytes */
 	off_t offset;
 
+	/* Error/exit status */
+	int status;
+
 	/* Archiver specific. Can make it a union if it ever gets big */
 #define PAX_NEXT_FILE 0
 #define PAX_GLOBAL    1