lib/file: Fix errors found by Coverity scan

Fix several errors in copy_file_secure_dest() found by Coverity and some
minor formatting issues:

143603: Correctly handle mkstemp() return value
143605: Avoid accessing dest_filename[-1] on readlink() error
143606, 143610: Avoid accessing dest_filename[sizeof(dest_filename)]
143607: Fix incorrectly passing sizeof(pointer) to fread()
143608, 143611: Cleanup resources on early exit
143609: Explicitly set umask before calling mkstemp()

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>

1 files changed, 51 insertions, 34 deletions

diff --git a/lib/file/file.c b/lib/file/file.c
index 6a270a3..0d18788 100644
--- a/lib/file/file.c
+++ b/lib/file/file.c
@@ -33,32 +33,52 @@
 
 static const int max_file_size = 1024 * 1024;
 
-int copy_file_secure_dest(void *ctx,
-	const char *source_file, char **destination_file) {
-	int result = 0;
-	char template[] = "/tmp/petitbootXXXXXX";
+int copy_file_secure_dest(void *ctx, const char *source_file,
+		char **destination_file)
+{
+	char readlink_buffer[MAX_FILENAME_SIZE + 1];
 	char dest_filename[MAX_FILENAME_SIZE] = "";
-	FILE *source_handle = fopen(source_file, "r");
-	int destination_fd = mkstemp(template);
-	FILE *destination_handle = fdopen(destination_fd, "w");
-	if (!source_handle || !(destination_handle)) {
-		// handle open error
-		pb_log("%s: failed: unable to open source file '%s'\n",
+	char template[] = "/tmp/petitbootXXXXXX";
+	FILE *destination_handle, *source_handle;
+	int destination_fd, result = 0;
+	unsigned char *buffer;
+	ssize_t r;
+	size_t l1;
+	mode_t oldmask;
+
+	source_handle = fopen(source_file, "r");
+	if (!source_handle) {
+		pb_log("%s: unable to open source file '%s': %m\n",
 			__func__, source_file);
+			return -1;
+	}
+
+	oldmask = umask(0644);
+	destination_fd = mkstemp(template);
+	umask(oldmask);
+	if (destination_fd < 0) {
+		pb_log("%s: unable to create temp file, %m\n", __func__);
+		fclose(source_handle);
+		return -1;
+	}
+	destination_handle = fdopen(destination_fd, "w");
+	if (!destination_handle) {
+		pb_log("%s: unable to open destination file, %m\n", __func__);
+		fclose(source_handle);
+		close(destination_fd);
 		return -1;
 	}
 
-	size_t l1;
-	unsigned char *buffer;
 	buffer = talloc_array(ctx, unsigned char, FILE_XFER_BUFFER_SIZE);
 	if (!buffer) {
 		pb_log("%s: failed: unable to allocate file transfer buffer\n",
 			__func__);
-		return -1;
+		result = -1;
+		goto out;
 	}
 
 	/* Copy data */
-	while ((l1 = fread(buffer, 1, sizeof buffer, source_handle)) > 0) {
+	while ((l1 = fread(buffer, 1, FILE_XFER_BUFFER_SIZE, source_handle)) > 0) {
 		size_t l2 = fwrite(buffer, 1, l1, destination_handle);
 		if (l2 < l1) {
 			if (ferror(destination_handle)) {
@@ -76,32 +96,29 @@ int copy_file_secure_dest(void *ctx,
 		}
 	}
 
-	talloc_free(buffer);
-
 	if (result) {
-		dest_filename[0] = '\0';
+		*destination_file = NULL;
+		goto out;
 	}
-	else {
-		ssize_t r;
-		char readlink_buffer[MAX_FILENAME_SIZE];
-		snprintf(readlink_buffer, MAX_FILENAME_SIZE, "/proc/self/fd/%d",
-			destination_fd);
-		r = readlink(readlink_buffer, dest_filename,
-			MAX_FILENAME_SIZE);
-		if (r < 0) {
-			/* readlink failed */
-			result = -1;
-			pb_log("%s: failed: unable to obtain temporary filename"
-				"\n", __func__);
-		}
-		dest_filename[r] = '\0';
+
+	snprintf(readlink_buffer, MAX_FILENAME_SIZE, "/proc/self/fd/%d",
+		destination_fd);
+	r = readlink(readlink_buffer, dest_filename, MAX_FILENAME_SIZE);
+	if (r < 0) {
+		/* readlink failed */
+		result = -1;
+		r = 0;
+		pb_log("%s: failed: unable to obtain temporary filename\n",
+			__func__);
 	}
+	dest_filename[r] = '\0';
 
+	*destination_file = talloc_strdup(ctx, dest_filename);
+out:
+	talloc_free(buffer);
 	fclose(source_handle);
 	fclose(destination_handle);
-
-	*destination_file = talloc_strdup(ctx, dest_filename);
-
+	close(destination_fd);
 	return result;
 }