tools: acrn-crashlog: fix potential issues under common and usercrash

This patch is to fix buffer overflow, return value not unified and
variable type not matched issues. And add some judge logic to improve
code quality.

Changes:
1. Handle the fd properly in the failing case.
2. Fix buffer overflow issues and null pointer access issues.
3. Fix the format issue in log_sys.c.
4. Remove the useless branch and adjust the function logic.
5. Add some checks for the string length before using strcpy/strcat/memcpy.
6. Fix strncpy null-terminated issues.
7. Change the return value to unify the return type.

Signed-off-by: CHEN Gang <gang.c.chen@intel.com>
Signed-off-by: xiaojin2 <xiaojing.liu@intel.com>
Reviewed-by: Zhi Jin <zhi.jin@intel.com>
Reviewed-by: Liu Xinwu <xinwu.liu@intel.com>
Acked-by: Zhang Di <di.zhang@intel.com>

tools/acrn-crashlog/common/cmdutils.c

@@ -47,7 +47,7 @@ int execv_out2file(char *argv[], char *outfile)
 
 	if (pid == 0) {
 		int res;
-		int fd;
+		int fd = -1;
 
 		if (outfile) {
 			fd = open(outfile, O_WRONLY | O_CREAT | O_TRUNC, 0664);
@@ -65,6 +65,8 @@ int execv_out2file(char *argv[], char *outfile)
 
 		res = execvp(argv[0], argv);
 		if (res == -1) {
+			if (fd > 0)
+				close(fd);
 			LOGE("execvp (%s) failed, error (%s)\n", argv[0],
 			     strerror(errno));
 			return -1;
@@ -211,8 +213,10 @@ char *exec_out2mem(char *fmt, ...)
 			memsize += 1024;
 			new = realloc(out, memsize);
 			if (!new) {
-				if (out)
+				if (out) {
 					free(out);
+					out = NULL;
+				}
 				goto end;
 			} else {
 				out = new;

tools/acrn-crashlog/common/fsutils.c

@@ -114,11 +114,12 @@ char *mm_get_line(struct mm_file_t *mfile, int line)
 
 	if (line <= 0 || line > mm_count_lines(mfile))
 		return NULL;
-	else if (line == 1)
+
+	if (line == 1)
 		ret = begin;
 	else {
 		next = begin;
-		for (i = 2; i <= line; i++)
+		for (i = 2; i <= line && next; i++)
 			next = strchr(next, '\n') + 1;
 		ret = next;
 	}
@@ -820,7 +821,7 @@ int read_full_binary_file(const char *path, unsigned long *size, void **data)
 	close_file(path, f);
 
 	*data = buf;
-	*size = (unsigned int)_size;
+	*size = (unsigned long)_size;
 
 	return 0;
 
@@ -1112,7 +1113,7 @@ int read_file(const char *path, unsigned long *size, void **data)
 	int len = 0;
 	int fd = 0;
 	int memsize = 1; /* for '\0' */
-	size_t result = 0;
+	ssize_t result = 0;
 	char *out = NULL;
 	char *new;
 
@@ -1129,6 +1130,8 @@ int read_file(const char *path, unsigned long *size, void **data)
 		result = read(fd, (void *)tmp, 1024);
 		if (result == 0)
 			break;
+		else if (result == -1)
+			goto free;
 
 		memsize += result;
 		new = realloc(out, memsize);
@@ -1145,7 +1148,7 @@ int read_file(const char *path, unsigned long *size, void **data)
 	close(fd);
 
 	*data = out;
-	*size = (unsigned int)len;
+	*size = (unsigned long)len;
 
 	return 0;
 

tools/acrn-crashlog/common/log_sys.c

@@ -19,7 +19,7 @@ void do_log(const int level,
 	char log[MAX_LOG_LEN];
 	int n = 0;
 #ifdef DEBUG_ACRN_CRASHLOG
-	const char header_fmt[] = "<%-20s%d>: ";
+	const char header_fmt[] = "<%-20s%5d>: ";
 #endif
 
 	if (level > LOG_LEVEL)
@@ -37,7 +37,7 @@ void do_log(const int level,
 #ifdef DEBUG_ACRN_CRASHLOG
 	/* header */
 	n = snprintf(log, sizeof(log), header_fmt, func, line);
-	if (n < 0 || n >= sizeof(log))
+	if (n < 0 || (size_t)n >= sizeof(log))
 		n = 0;
 #endif
 	/* msg */

tools/acrn-crashlog/common/strutils.c

@@ -74,11 +74,16 @@ static char *strtriml(char *str)
 
 static char *strtrimr(char *str)
 {
-	char *end = str + strlen(str) - 1;
+	size_t len;
+	char *end;
 
-	while (*end == ' ' && end >= str) {
-		*end = 0;
-		end--;
+	len = strlen(str);
+	if (len > 0) {
+		end = str + strlen(str) - 1;
+		while (*end == ' ' && end >= str) {
+			*end = 0;
+			end--;
+		}
 	}
 
 	return str;
@@ -86,8 +91,12 @@ static char *strtrimr(char *str)
 
 char *strtrim(char *str)
 {
-	strtrimr(str);
-	return strtriml(str);
+	if (str) {
+		strtrimr(str);
+		return strtriml(str);
+	}
+
+	return NULL;
 }
 
 int strcnt(char *str, char c)

tools/acrn-crashlog/usercrash/client.c

@@ -72,8 +72,8 @@ static int set_timeout(int sockfd)
  * usercrashd_connect is used to connect to server, and get the crash file fd
  * from server
  */
-bool usercrashd_connect(int pid, int *usercrashd_socket,
-		int *output_fd, char *name)
+static int usercrashd_connect(int pid, int *usercrashd_socket,
+		int *output_fd, const char *name)
 {
 	int sockfd;
 	int tmp_output_fd;
@@ -81,15 +81,20 @@ bool usercrashd_connect(int pid, int *usercrashd_socket,
 	int flags;
 	struct crash_packet packet = {0};
 
+	if (name == NULL) {
+		LOGE("crash process name is NULL\n");
+		return -1;
+	}
 	sockfd = socket_local_client(SOCKET_NAME, SOCK_SEQPACKET);
 	if (sockfd == -1) {
 		LOGE("failed to connect to usercrashd, error (%s)\n",
 			strerror(errno));
-		return false;
+		return -1;
 	}
 	packet.packet_type = kDumpRequest;
 	packet.pid = pid;
-	strncpy(packet.name, name, COMM_NAME_LEN - 1);
+	strncpy(packet.name, name, COMM_NAME_LEN);
+	packet.name[COMM_NAME_LEN - 1] = '\0';
 	if (set_timeout(sockfd)) {
 		close(sockfd);
 		return -1;
@@ -99,7 +104,7 @@ bool usercrashd_connect(int pid, int *usercrashd_socket,
 		LOGE("failed to write DumpRequest packet, error (%s)\n",
 			strerror(errno));
 		close(sockfd);
-		return false;
+		return -1;
 	}
 
 	rc = recv_fd(sockfd, &packet, sizeof(packet),
@@ -108,7 +113,7 @@ bool usercrashd_connect(int pid, int *usercrashd_socket,
 		LOGE("failed to read response to DumpRequest packet, ");
 		LOGE("error (%s)\n", strerror(errno));
 		close(sockfd);
-		return false;
+		return -1;
 	} else if (rc != sizeof(packet)) {
 		LOGE("received DumpRequest response packet of incorrect ");
 		LOGE("length (expected %lu, got %ld)\n", sizeof(packet), rc);
@@ -135,11 +140,11 @@ bool usercrashd_connect(int pid, int *usercrashd_socket,
 	*usercrashd_socket = sockfd;
 	*output_fd = tmp_output_fd;
 
-	return true;
+	return 0;
 fail:
 	close(sockfd);
 	close(tmp_output_fd);
-	return false;
+	return -1;
 
 }
 
@@ -149,7 +154,7 @@ fail:
  * dump, the server will pop another crash from the queue and execute the dump
  * process
  */
-bool usercrashd_notify_completion(int usercrashd_socket)
+static int usercrashd_notify_completion(int usercrashd_socket)
 {
 	struct crash_packet packet = {0};
 
@@ -160,14 +165,15 @@ bool usercrashd_notify_completion(int usercrashd_socket)
 	}
 	if (write(usercrashd_socket, &packet,
 				sizeof(packet)) != sizeof(packet)) {
-		return false;
+		return -1;
 	}
-	return true;
+	return 0;
 }
 
 static void print_usage(void)
 {
-	printf("It's the client role of usercrash.\n");
+	printf("usercrash - tool to dump crash info for the crashes in the ");
+	printf("userspace on sos. It's the client role of usercrash.\n");
 	printf("[Usage]\n");
 	printf("\t--coredump, usercrash_c <pid> <comm> <sig> ");
 	printf("(root role to run)\n");
@@ -209,14 +215,14 @@ int main(int argc, char *argv[])
 		pid = atoi(argv[1]);
 		sig = atoi(argv[3]);
 		ret = usercrashd_connect(pid, &sock, &out_fd, argv[2]);
-		if (!ret) {
+		if (ret) {
 			LOGE("usercrashd_connect failed, error (%s)\n",
 					strerror(errno));
 			exit(EXIT_FAILURE);
 		}
 		crash_dump(pid, sig, out_fd);
 		close(out_fd);
-		if (!usercrashd_notify_completion(sock)) {
+		if (usercrashd_notify_completion(sock)) {
 			LOGE("failed to notify usercrashd of completion");
 			close(sock);
 			exit(EXIT_FAILURE);

tools/acrn-crashlog/usercrash/crash_dump.c

@@ -79,43 +79,35 @@ static const char *get_signame(int sig)
  */
 static int save_coredump(const char *filename)
 {
-	size_t read;
+	size_t read_len;
+	size_t write_len;
 	char input_buffer[BUFFER_SIZE];
 	FILE *dump_file;
 
 	/* open dump file in write and binary mode */
 	dump_file = fopen(filename, "wb");
-	if (dump_file != NULL) {
-		/* Read from STDIN and write to dump file */
-		while (1) {
-			read = fread(input_buffer, 1, BUFFER_SIZE, stdin);
-			if (read > 0) {
-				if (fwrite(input_buffer, 1, read, dump_file) <=
-							0) {
-					LOGE("fwrite error\n");
-					goto fail;
-				}
-				continue;
-			} else if (read == 0) {
-				break;
-			}
-
-			LOGE("fread error\n");
-			goto fail;
+	if (dump_file == NULL) {
+		LOGE("fopen core file failed\n");
+		return -1;
+	}
+	/* Read from STDIN and write to dump file */
+	while (1) {
+		read_len = fread(input_buffer, 1, BUFFER_SIZE, stdin);
+		if (read_len == 0)
+			break;
+		write_len = fwrite(input_buffer, 1, read_len, dump_file);
+		if (write_len == 0) {
+			LOGE("fwrite error\n");
+			fclose(dump_file);
+			return -1;
 		}
-		fclose(dump_file);
-		return 0;
 	}
 
-	LOGE("fopen core file failed\n");
-	return -1;
-
-fail:
 	fclose(dump_file);
-	return -1;
+	return 0;
 }
 
-static int get_backtrace(int pid, int fd, int sig, char *comm)
+static int get_backtrace(int pid, int fd, int sig, const char *comm)
 {
 	char *membkt;
 	char format[512];
@@ -150,7 +142,7 @@ static int get_backtrace(int pid, int fd, int sig, char *comm)
  * @name: a string save to file
  * return 0 on success, or -1 on error
  */
-static int save_proc_info(int pid, int fd, char *path, char *name)
+static int save_proc_info(int pid, int fd, const char *path, const char *name)
 {
 	int ret;
 	char *data;
@@ -171,7 +163,7 @@ static int save_proc_info(int pid, int fd, char *path, char *name)
 
 }
 
-static int get_openfiles(int pid, int fd, char *path, char *name)
+static int get_openfiles(int pid, int fd, const char *path, const char *name)
 {
 	int ret;
 	int loop;
@@ -205,7 +197,7 @@ static int get_openfiles(int pid, int fd, char *path, char *name)
 	return 0;
 }
 
-static int save_usercrash_file(int pid, int tgid, char *comm,
+static int save_usercrash_file(int pid, int tgid, const char *comm,
 		int sig, int out_fd)
 {
 	int ret;
@@ -242,7 +234,8 @@ static int save_usercrash_file(int pid, int tgid, char *comm,
 	return 0;
 }
 
-static int get_key_value(int pid, char *path, char *key, char *value)
+static int get_key_value(int pid, const char *path, const char *key,
+		char *value, size_t value_len)
 {
 	int len;
 	int ret;
@@ -256,7 +249,7 @@ static int get_key_value(int pid, char *path, char *key, char *value)
 	memset(format, 0, sizeof(format));
 	snprintf(format, sizeof(format), path, pid);
 	ret = read_file(format, &size, (void *)&data);
-	if (ret) {
+	if (ret || !data) {
 		LOGE("read file failed\n");
 		return -1;
 	}
@@ -271,6 +264,10 @@ static int get_key_value(int pid, char *path, char *key, char *value)
 		end = data + size;
 	start = msg + strlen(key);
 	len = end - start;
+	if (len >= (int)value_len) {
+		free(data);
+		return -1;
+	}
 	memcpy(value, start, len);
 	*(value + len) = 0;
 	free(data);
@@ -301,7 +298,7 @@ void crash_dump(int pid, int sig, int out_fd)
 	}
 	comm[ret] = '\0';
 
-	ret = get_key_value(pid, GET_TID, "Tgid:", result);
+	ret = get_key_value(pid, GET_TID, "Tgid:", result, sizeof(result));
 	if (ret) {
 		LOGE("get Tgid error\n");
 		return;

tools/acrn-crashlog/usercrash/debugger.c

@@ -19,10 +19,9 @@
  */
 static void print_usage(void)
 {
-	printf("debugger - tool to dump process info of a running process. ");
+	printf("debugger - tool to dump process info of a running process.\n");
 	printf("[Usage]\n");
 	printf("\t--shell cmd, debugger <pid>  (root role to run)\n");
-	printf("(root role to run)\n");
 	printf("[Option]\n");
 	printf("\t-h: print this usage message\n");
 	printf("\t-v: print debugger version\n");
@@ -48,7 +47,7 @@ int main(int argc, char *argv[])
 		print_usage();
 
 	if (getuid() != 0) {
-		LOGE("failed to execute debugger, root is required\n");
+		printf("failed to execute debugger, root is required\n");
 		exit(EXIT_FAILURE);
 	}
 

tools/acrn-crashlog/usercrash/include/packet.h

@@ -10,7 +10,6 @@
 #define SOCKET_NAME "user_crash"
 
 #include <stdio.h>
-#include <stdbool.h>
 
 enum CrashPacketType {
 	/* Initial request from crash_dump */

tools/acrn-crashlog/usercrash/protocol.c

@@ -27,23 +27,28 @@
 #include "protocol.h"
 #include "log_sys.h"
 
+#define SUN_PATH_MAX \
+	(sizeof(struct sockaddr_un) - offsetof(struct sockaddr_un, sun_path))
+
 /* Documented in header file. */
 static int socket_make_sockaddr_un(const char *name,
 		struct sockaddr_un *p_addr, socklen_t *alen)
 {
-	size_t namelen;
+	size_t name_len;
+	size_t socket_len;
 
-	memset(p_addr, 0, sizeof(struct sockaddr_un));
-	namelen = strlen(name) + strlen(RESERVED_SOCKET_PREFIX);
-	/* unix_path_max appears to be missing on linux */
-	if (namelen > sizeof(*p_addr)-
-			offsetof(struct sockaddr_un, sun_path) - 1) {
+	socket_len = strlen(RESERVED_SOCKET_PREFIX);
+	if (socket_len >= SUN_PATH_MAX)
 		return -1;
-	}
 	strcpy(p_addr->sun_path, RESERVED_SOCKET_PREFIX);
+	name_len = strlen(name);
+	if (name_len >= (SUN_PATH_MAX - socket_len))
+		return -1;
 	strcat(p_addr->sun_path, name);
+
 	p_addr->sun_family = AF_LOCAL;
-	*alen = namelen + offsetof(struct sockaddr_un, sun_path) + 1;
+	*alen = name_len + socket_len +
+		offsetof(struct sockaddr_un, sun_path) + 1;
 	return 0;
 }
 
@@ -100,8 +105,12 @@ static int socket_bind(int fd, const char *name)
 {
 	struct sockaddr_un addr;
 	socklen_t alen;
+	size_t name_len;
 
 	addr.sun_family = AF_UNIX;
+	name_len = strlen(name);
+	if (name_len >= SUN_PATH_MAX)
+		return -1;
 	strcpy(addr.sun_path, name);
 	unlink(addr.sun_path);
 	alen = strlen(addr.sun_path) + sizeof(addr.sun_family);

tools/acrn-crashlog/usercrash/server.c

@@ -185,8 +185,8 @@ static int get_usercrash(struct crash_node *crash)
 	}
 	next_usercrash = (next_usercrash + 1) % usercrash_count;
 	crash->out_fd = result;
-	strncpy(crash->crash_path, file_name, FILE_PATH_LEN_MAX - 1);
-
+	strncpy(crash->crash_path, file_name, FILE_PATH_LEN_MAX);
+	crash->crash_path[FILE_PATH_LEN_MAX - 1] = '\0';
 	return 0;
 }
 
@@ -295,8 +295,8 @@ static void crash_request_cb(evutil_socket_t sockfd, short ev, void *arg)
 		goto fail;
 	}
 	crash->pid = request.pid;
-	strncpy(crash->name, request.name, COMM_NAME_LEN - 1);
-
+	strncpy(crash->name, request.name, COMM_NAME_LEN);
+	crash->name[COMM_NAME_LEN - 1] = '\0';
 	LOGI("received crash request from pid %d, name: %s\n",
 				crash->pid, crash->name);
 
@@ -452,7 +452,7 @@ int main(int argc, char *argv[])
 			RESERVED_SOCKET_PREFIX, SOCKET_NAME);
 	if (chmod(socket_path, 0622) == -1) {
 		LOGE("failed to change usercrashd_crash priority\n");
-		exit(EXIT_FAILURE);
+		goto fail;
 	}
 
 	evutil_make_socket_nonblocking(crash_socket);
@@ -460,7 +460,7 @@ int main(int argc, char *argv[])
 	base = event_base_new();
 	if (!base) {
 		LOGE("failed to create event_base\n");
-		exit(EXIT_FAILURE);
+		goto fail;
 	}
 
 	TAILQ_INIT(&queue_t);
@@ -468,7 +468,7 @@ int main(int argc, char *argv[])
 			LEV_OPT_CLOSE_ON_FREE, -1, crash_socket);
 	if (!listener) {
 		LOGE("failed to create evconnlistener: %s\n", strerror(errno));
-		exit(EXIT_FAILURE);
+		goto fail;
 	}
 
 	LOGI("usercrash_s successfully initialized\n");
@@ -477,5 +477,10 @@ int main(int argc, char *argv[])
 	evconnlistener_free(listener);
 	event_base_free(base);
 
+	close(crash_socket);
 	return 0;
+
+fail:
+	close(crash_socket);
+	exit(EXIT_FAILURE);
 }