If the attempt to open the archive fails (either the client open

routine fails or the first read fails), invoke the client close
routine immediately so the client can clean up.  Also, don't store the
client pointers in this case, so that the client close routine can't
accidentally get called more than once.

A minor style fix to archive_read_open_fd.c while I'm here.

PR: 86453
Thanks to: Andrew Turner for reporting this and suggesting a fix.

2 files changed, 38 insertions, 14 deletions

diff --git a/lib/libarchive/archive_read.c b/lib/libarchive/archive_read.c
index a7ed733..c6e47e1 100644
--- a/lib/libarchive/archive_read.c
+++ b/lib/libarchive/archive_read.c
@@ -107,8 +107,8 @@ archive_read_set_bytes_per_block(struct archive *a, int bytes_per_block)
  */
 int
 archive_read_open(struct archive *a, void *client_data,
-    archive_open_callback *opener, archive_read_callback *reader,
-    archive_close_callback *closer)
+    archive_open_callback *client_opener, archive_read_callback *client_reader,
+    archive_close_callback *client_closer)
 {
 	const void *buffer;
 	ssize_t bytes_read;
@@ -117,36 +117,59 @@ archive_read_open(struct archive *a, void *client_data,
 
 	__archive_check_magic(a, ARCHIVE_READ_MAGIC, ARCHIVE_STATE_NEW, "archive_read_open");
 
-	if (reader == NULL)
+	if (client_reader == NULL)
 		__archive_errx(1,
 		    "No reader function provided to archive_read_open");
 
-	a->client_reader = reader;
-	a->client_opener = opener;
-	a->client_closer = closer;
-	a->client_data = client_data;
+	/*
+	 * Set these NULL initially.  If the open or initial read fails,
+	 * we'll leave them NULL to indicate that the file is invalid.
+	 * (In particular, this helps ensure that the closer doesn't
+	 * get called more than once.)
+	 */
+	a->client_opener = NULL;
+	a->client_reader = NULL;
+	a->client_closer = NULL;
+	a->client_data = NULL;
 
 	/* Open data source. */
-	if (a->client_opener != NULL) {
-		e =(a->client_opener)(a, a->client_data);
-		if (e != 0)
+	if (client_opener != NULL) {
+		e =(client_opener)(a, client_data);
+		if (e != 0) {
+			/* If the open failed, call the closer to clean up. */
+			if (client_closer)
+				(client_closer)(a, client_data);
 			return (e);
+		}
 	}
 
 	/* Read first block now for format detection. */
-	bytes_read = (a->client_reader)(a, a->client_data, &buffer);
+	bytes_read = (client_reader)(a, client_data, &buffer);
 
-	/* client_reader should have already set error information. */
-	if (bytes_read < 0)
+	if (bytes_read < 0) {
+		/* If the first read fails, close before returning error. */
+		if (client_closer)
+			(client_closer)(a, client_data);
+		/* client_reader should have already set error information. */
 		return (ARCHIVE_FATAL);
+	}
 
 	/* An empty archive is a serious error. */
 	if (bytes_read == 0) {
 		archive_set_error(a, ARCHIVE_ERRNO_FILE_FORMAT,
 		    "Empty input file");
+		/* Close the empty file. */
+		if (client_closer)
+			(client_closer)(a, client_data);
 		return (ARCHIVE_FATAL);
 	}
 
+	/* Now that the client callbacks have worked, remember them. */
+	a->client_opener = client_opener; /* Do we need to remember this? */
+	a->client_reader = client_reader;
+	a->client_closer = client_closer;
+	a->client_data = client_data;
+
 	/* Select a decompression routine. */
 	high_bidder = choose_decompressor(a, buffer, bytes_read);
 	if (high_bidder < 0)
diff --git a/lib/libarchive/archive_read_open_fd.c b/lib/libarchive/archive_read_open_fd.c
index c5d1195..c5716e7 100644
--- a/lib/libarchive/archive_read_open_fd.c
+++ b/lib/libarchive/archive_read_open_fd.c
@@ -99,7 +99,8 @@ file_close(struct archive *a, void *client_data)
 	struct read_fd_data *mine = client_data;
 
 	(void)a; /* UNUSED */
-	free(mine->buffer);
+	if (mine->buffer != NULL)
+		free(mine->buffer);
 	free(mine);
 	return (ARCHIVE_OK);
 }