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>

Author: xiaojingrich

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,20 +74,29 @@ static char *strtriml(char *str)
 
 static char *strtrimr(char *str)
 {
-	char *end = str + strlen(str) - 1;
-
-	while (*end == ' ' && end >= str) {
-		*end = 0;
-		end--;
+	size_t len;
+	char *end;
+
+	len = strlen(str);
+	if (len > 0) {
+		end = str + strlen(str) - 1;
+		while (*end == ' ' && end >= str) {
+			*end = 0;
+			end--;
+		}
 	}
 
 	return 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,24 +72,29 @@ 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;
 	ssize_t rc;
 	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 @@ bool usercrashd_connect(int pid, int *usercrashd_socket,
  * 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);
 	}