WUtil: rewrote wcopy_file for better error handling and to fix Coverity #50234

The original code used the libc "fopen" kind of operation, which are handy
when manipulating text files, but:
 - bring an overhead for binary files that we don't need here;
 - does not provide the mechanisms for safe error handling and special cases

As Coverity reported a Time-of-Check/Time-of-Use type of security issue,
took the opportunity to fix it and increased the size of the buffer used
for data to allow better use of modern disk performances.

Signed-off-by: Christophe CURIS <christophe.curis@free.fr>
Signed-off-by: Carlos R. Mafra <crmafra@gmail.com>

WINGs/findfile.c

@@ -23,7 +23,9 @@
 
 #include "WUtil.h"
 
+#include <sys/types.h>
 #include <sys/stat.h>
+#include <fcntl.h>
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -422,60 +424,104 @@ char *wfindfileinarray(WMPropList *array, const char *file)
 	return NULL;
 }
 
-int wcopy_file(const char *dir, const char *src_file, const char *dest_file)
+int wcopy_file(const char *dest_dir, const char *src_file, const char *dest_file)
 {
-	FILE *src, *dst;
+	char *path_dst;
-	size_t nread, nwritten;
+	int fd_src, fd_dst;
-	char *dstpath;
+	struct stat stat_src;
-	struct stat st;
+	mode_t permission_dst;
-	char buf[4096];
+	const size_t buffer_size = 2 * 1024 * 1024;	/* 4MB is a decent start choice to allow the OS to take advantage of modern disk's performance */
+	char *buffer;	/* The buffer is not created on the stack to avoid possible stack overflow as our buffer is big */
 
-	/* only to a directory */
+ try_again_src:
-	if (stat(dir, &st) != 0 || !S_ISDIR(st.st_mode))
+	fd_src = open(src_file, O_RDONLY | O_NOFOLLOW);
-		return -1;
+	if (fd_src == -1) {
-	/* only copy files */
+		if (errno == EINTR)
-	if (stat(src_file, &st) != 0 || !S_ISREG(st.st_mode))
+			goto try_again_src;
-		return -1;
+		werror(_("Could not open input file \"%s\": %s"), src_file, strerror(errno));
-
-	do {
-		src = fopen(src_file, "rb");
-	} while ((src == NULL) && (errno == EINTR));
-	if (src == NULL) {
-		werror(_("Could not open input file \"%s\""), src_file);
 		return -1;
 	}
 
-	dstpath = wstrconcat(dir, dest_file);
+	/* Only accept to copy regular files */
-	do {
+	if (fstat(fd_src, &stat_src) != 0 || !S_ISREG(stat_src.st_mode)) {
-		dst = fopen(dstpath, "wb");
+		close(fd_src);
-	} while ((dst == NULL) && (errno == EINTR));
-	if (dst == NULL) {
-		werror(_("Could not create target file \"%s\""), dstpath);
-		wfree(dstpath);
-		fclose(src);
 		return -1;
 	}
 
-	do {
+	path_dst = wstrconcat(dest_dir, dest_file);
-		nread = fread(buf, 1, sizeof(buf), src);
+ try_again_dst:
-		if (ferror(src))
+	fd_dst = open(path_dst, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
-			break;
+	if (fd_dst == -1) {
+		if (errno == EINTR)
+			goto try_again_dst;
+		werror(_("Could not create target file \"%s\": %s"), path_dst, strerror(errno));
+		wfree(path_dst);
+		close(fd_src);
+		return -1;
+	}
 
-		nwritten = fwrite(buf, 1, nread, dst);
+	buffer = malloc(buffer_size);	/* Don't use wmalloc to avoid the memset(0) we don't need */
-		if (ferror(dst) || feof(src) || nread != nwritten)
+	if (buffer == NULL) {
-			break;
+		werror(_("could not allocate memory for the copy buffer"));
+		close(fd_dst);
+		goto cleanup_and_return_failure;
+	}
 
-	} while (1);
+	for (;;) {
+		ssize_t size_data;
+		const char *write_ptr;
+		size_t write_remain;
 
-	if (ferror(src) || ferror(dst))
+	try_again_read:
-		unlink(dstpath);
+		size_data = read(fd_src, buffer, buffer_size);
+		if (size_data == 0)
+			break; /* End of File have been reached */
+		if (size_data < 0) {
+			if (errno == EINTR)
+				goto try_again_read;
+			werror(_("could not read from file \"%s\": %s"), src_file, strerror(errno));
+			close(fd_dst);
+			goto cleanup_and_return_failure;
+		}
 
-	fclose(src);
+		write_ptr = buffer;
-	fchmod(fileno(dst), st.st_mode);
+		write_remain = size_data;
-	fsync(fileno(dst));
+		while (write_remain > 0) {
-	if (fclose(dst))
+			ssize_t write_done;
-		wwarning("error occured during fclose(\"%s\")", dstpath);
+
-	wfree(dstpath);
+		try_again_write:
+			write_done = write(fd_dst, write_ptr, write_remain);
+			if (write_done < 0) {
+				if (errno == EINTR)
+					goto try_again_write;
+				werror(_("could not write data to file \"%s\": %s"), path_dst, strerror(errno));
+				close(fd_dst);
+				goto cleanup_and_return_failure;
+			}
+			write_ptr    += write_done;
+			write_remain -= write_done;
+		}
+	}
+
+	/* Keep only the permission-related part of the field: */
+	permission_dst = stat_src.st_mode & (S_IRWXU | S_IRWXG | S_IRWXO | S_ISUID | S_ISGID | S_ISVTX);
+	if (fchmod(fd_dst, permission_dst) != 0)
+		wwarning(_("could not set permission 0%03o on file \"%s\": %s"),
+		         permission_dst, path_dst, strerror(errno));
+
+	if (close(fd_dst) != 0) {
+		werror(_("could not close the file \"%s\": %s"), path_dst, strerror(errno));
+	cleanup_and_return_failure:
+		free(buffer);
+		wfree(path_dst);
+		close(fd_src);
+		unlink(path_dst);
+		return -1;
+	}
+
+	free(buffer);
+	wfree(path_dst);
+	close(fd_src);
 
 	return 0;
 }