From: Tim Kientzle <kientzle@acm.org>
Date: Thu, 25 Apr 2024 09:39:22 +0000 (-0700)
Subject: bsdtar: Fix error handling around strtol() usages (#2110)
X-Git-Tag: v3.7.4~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d9f44c5b44038c735a78cc1b32fda1ea7b88be25;p=thirdparty%2Flibarchive.git

bsdtar: Fix error handling around strtol() usages (#2110)

The code here had a couple of bad code patterns that seem to have been
copied throughout:
* Checking errno after strtol() -- Standard C doesn't seem to actually
require this, so we shouldn't rely on it
* Casting the result of strtol() directly to `int`. This loses
information prematurely.

Instead, I've added `l` as a temporary of type `long`, use that to hold
the result of `strtol()` until it can be checked. I've also removed the
`errno` tests in favor of checking the end pointer value.

The limit for --strip-components has been raised to 100 000.
---

diff --git a/tar/bsdtar.c b/tar/bsdtar.c
index b070e0fae..42baab286 100644
--- a/tar/bsdtar.c
+++ b/tar/bsdtar.c
@@ -157,6 +157,7 @@ main(int argc, char **argv)
 	char			*tptr, *uptr;
 	char			 possible_help_request;
 	char			 buff[16];
+	long			 l;
 
 	/*
 	 * Use a pointer for consistency, but stack-allocated storage
@@ -301,16 +302,15 @@ main(int argc, char **argv)
 			/* libarchive doesn't need this; just ignore it. */
 			break;
 		case 'b': /* SUSv2 */
-			errno = 0;
 			tptr = NULL;
-			t = (int)strtol(bsdtar->argument, &tptr, 10);
-			if (errno || t <= 0 || t > 8192 ||
+			l = strtol(bsdtar->argument, &tptr, 10);
+			if (l <= 0 || l > 8192L ||
 			    *(bsdtar->argument) == '\0' || tptr == NULL ||
 			    *tptr != '\0') {
 				lafe_errc(1, 0, "Invalid or out of range "
 				    "(1..8192) argument to -b");
 			}
-			bsdtar->bytes_per_block = 512 * t;
+			bsdtar->bytes_per_block = 512 * (int)l;
 			/* Explicit -b forces last block size. */
 			bsdtar->bytes_in_last_block = bsdtar->bytes_per_block;
 			break;
@@ -369,44 +369,42 @@ main(int argc, char **argv)
 			bsdtar->filename = bsdtar->argument;
 			break;
 		case OPTION_GID: /* cpio */
-			errno = 0;
 			tptr = NULL;
-			t = (int)strtol(bsdtar->argument, &tptr, 10);
-			if (errno || t < 0 || *(bsdtar->argument) == '\0' ||
+			l = strtol(bsdtar->argument, &tptr, 10);
+			if (l < 0 || l >= INT_MAX || *(bsdtar->argument) == '\0' ||
 			    tptr == NULL || *tptr != '\0') {
 				lafe_errc(1, 0, "Invalid argument to --gid");
 			}
-			bsdtar->gid = t;
+			bsdtar->gid = (int)l;
 			break;
 		case OPTION_GNAME: /* cpio */
 			bsdtar->gname = bsdtar->argument;
 			break;
 		case OPTION_GROUP: /* GNU tar */
-			errno = 0;
 			tptr = NULL;
 
 			uptr = strchr(bsdtar->argument, ':');
-			if(uptr != NULL) {
-				if(uptr[1] == 0) {
+			if (uptr != NULL) {
+				if (uptr[1] == '\0') {
 					lafe_errc(1, 0, "Invalid argument to --group (missing id after :)");
 				}
 				uptr[0] = 0;
 				uptr++;
-				t = (int)strtol(uptr, &tptr, 10);
-				if (errno || t < 0 || *uptr == '\0' ||
+				l = strtol(uptr, &tptr, 10);
+				if (l < 0 || l >= INT_MAX || *uptr == '\0' ||
 				    tptr == NULL || *tptr != '\0') {
 					lafe_errc(1, 0, "Invalid argument to --group (%s is not a number)", uptr);
 				} else {
-					bsdtar->gid = t;
+					bsdtar->gid = (int)l;
 				}
 				bsdtar->gname = bsdtar->argument;
 			} else {
-				t = (int)strtol(bsdtar->argument, &tptr, 10);
-				if (errno || t < 0 || *(bsdtar->argument) == '\0' ||
+				l = strtol(bsdtar->argument, &tptr, 10);
+				if (l < 0 || l >= INT_MAX || *(bsdtar->argument) == '\0' ||
 				    tptr == NULL || *tptr != '\0') {
 					bsdtar->gname = bsdtar->argument;
 				} else {
-					bsdtar->gid = t;
+					bsdtar->gid = (int)l;
 					bsdtar->gname = "";
 				}
 			}
@@ -662,31 +660,30 @@ main(int argc, char **argv)
 			bsdtar->option_options = bsdtar->argument;
 			break;
 		case OPTION_OWNER: /* GNU tar */
-			errno = 0;
 			tptr = NULL;
 
 			uptr = strchr(bsdtar->argument, ':');
-			if(uptr != NULL) {
-				if(uptr[1] == 0) {
+			if (uptr != NULL) {
+				if (uptr[1] == 0) {
 					lafe_errc(1, 0, "Invalid argument to --owner (missing id after :)");
 				}
 				uptr[0] = 0;
 				uptr++;
-				t = (int)strtol(uptr, &tptr, 10);
-				if (errno || t < 0 || *uptr == '\0' ||
+				l = strtol(uptr, &tptr, 10);
+				if (l < 0 || l >= INT_MAX || *uptr == '\0' ||
 				    tptr == NULL || *tptr != '\0') {
 					lafe_errc(1, 0, "Invalid argument to --owner (%s is not a number)", uptr);
 				} else {
-					bsdtar->uid = t;
+					bsdtar->uid = (int)l;
 				}
 				bsdtar->uname = bsdtar->argument;
 			} else {
-				t = (int)strtol(bsdtar->argument, &tptr, 10);
-				if (errno || t < 0 || *(bsdtar->argument) == '\0' ||
+				l = strtol(bsdtar->argument, &tptr, 10);
+				if (l < 0 || l >= INT_MAX || *(bsdtar->argument) == '\0' ||
 				    tptr == NULL || *tptr != '\0') {
 					bsdtar->uname = bsdtar->argument;
 				} else {
-					bsdtar->uid = t;
+					bsdtar->uid = (int)l;
 					bsdtar->uname = "";
 				}
 			}
@@ -748,15 +745,14 @@ main(int argc, char **argv)
 			bsdtar->extract_flags |= ARCHIVE_EXTRACT_OWNER;
 			break;
 		case OPTION_STRIP_COMPONENTS: /* GNU tar 1.15 */
-			errno = 0;
 			tptr = NULL;
-			t = (int)strtol(bsdtar->argument, &tptr, 10);
-			if (errno || t < 0 || *(bsdtar->argument) == '\0' ||
+			l = strtol(bsdtar->argument, &tptr, 10);
+			if (l < 0 || l > 100000L || *(bsdtar->argument) == '\0' ||
 			    tptr == NULL || *tptr != '\0') {
 				lafe_errc(1, 0, "Invalid argument to "
 				    "--strip-components");
 			}
-			bsdtar->strip_components = t;
+			bsdtar->strip_components = (int)l;
 			break;
 		case 'T': /* GNU tar */
 			bsdtar->names_from_file = bsdtar->argument;
@@ -776,14 +772,13 @@ main(int argc, char **argv)
 			set_mode(bsdtar, opt);
 			break;
 		case OPTION_UID: /* cpio */
-			errno = 0;
 			tptr = NULL;
-			t = (int)strtol(bsdtar->argument, &tptr, 10);
-			if (errno || t < 0 || *(bsdtar->argument) == '\0' ||
+			l = strtol(bsdtar->argument, &tptr, 10);
+			if (l < 0 || l >= INT_MAX || *(bsdtar->argument) == '\0' ||
 			    tptr == NULL || *tptr != '\0') {
 				lafe_errc(1, 0, "Invalid argument to --uid");
 			}
-			bsdtar->uid = t;
+			bsdtar->uid = (int)l;
 			break;
 		case OPTION_UNAME: /* cpio */
 			bsdtar->uname = bsdtar->argument;