tools: acrn-crashlog: Fix potential issues under acrnprobe

This patch is to fix potential issues, which are reported by static
analysis tool, for acrnprobe.

Changes:
1. Check the return value of sender_id() and get_sender_by_name(),
   since it could be -1 or NULL.
2. Remove the parameter len from create_event, take parameter path
   as a NULL-terminated string as default.
3. Modify for_each_* functions to avoid overflow.

Signed-off-by: Liu Xinwu <xinwu.liu@intel.com>
Reviewed-by: Zhi Jin <zhi.jin@intel.com>
Acked-by: Chen Gang <gang.c.chen@intel.com>

Author: xiaojingrich

tools/acrn-crashlog/acrnprobe/android_events.c

@@ -119,17 +119,22 @@ static int refresh_key_synced_stage1(struct sender_t *sender, struct vm_t *vm,
 					char *key, enum refresh_type_t type)
 {
 	char log_new[64];
-	char *log_vmrecordid = sender->log_vmrecordid;
+	char *log_vmrecordid;
+	int sid;
 
 	if (!key || !sender || !vm)
 		return -1;
 
+	sid = sender_id(sender);
+	if (sid == -1)
+		return -1;
+	log_vmrecordid = sender->log_vmrecordid;
 	/* the length of key must be 20, and its value can not be
 	 * 00000000000000000000.
 	 */
 	if ((strlen(key) == 20) &&
 	    strcmp(key, "00000000000000000000")) {
-		sprintf(vm->last_synced_line_key[sender_id(sender)],
+		sprintf(vm->last_synced_line_key[sid],
 			"%s", key);
 		if (type == MM_ONLY)
 			return 0;
@@ -158,6 +163,9 @@ static int refresh_key_synced_stage2(struct mm_file_t *m_vm_records, char *key)
 	if (*key) {
 		begin = strstr(m_vm_records->begin, " <==");
 		end = strrstr(m_vm_records->begin, key);
+		if (!begin || !end)
+			return -1;
+
 		end = strchr(end, '\n');
 
 		for (p = begin; p < end; p++) {
@@ -183,6 +191,8 @@ static int get_vm_history(struct vm_t *vm, struct sender_t *sender,
 		return -1;
 
 	sid = sender_id(sender);
+	if (sid == -1)
+		return -1;
 
 	snprintf(vm_history, sizeof(vm_history), "/tmp/%s_%s",
 		 "vm_hist", vm->name);
@@ -221,6 +231,9 @@ static void sync_lines_stage1(struct sender_t *sender, void *data[])
 	char *vm_format = "%*[^ ]%*[ ]%[^ ]%*c";
 
 	sid = sender_id(sender);
+	if (sid == -1)
+		return;
+
 	for_each_vm(id, vm, conf) {
 		if (!vm)
 			continue;
@@ -287,6 +300,10 @@ static void sync_lines_stage2(struct sender_t *sender, void *data[],
 						       strerror(errno));
 		return;
 	}
+	if (!m_vm_records->size) {
+		LOGE("size(0b) of (%s)\n", sender->log_vmrecordid);
+		return;
+	}
 
 	cursor = strstr(m_vm_records->begin, " <==");
 	if (!cursor)
@@ -334,11 +351,19 @@ static void sync_lines_stage2(struct sender_t *sender, void *data[],
 static void get_last_line_synced(struct sender_t *sender)
 {
 	int id;
+	int sid;
 	int ret;
 	struct vm_t *vm;
 	char vmkey[SHA_DIGEST_LENGTH + 10] = {0};
 	char word[256];
 
+	if (!sender)
+		return;
+
+	sid = sender_id(sender);
+	if (sid == -1)
+		return;
+
 	for_each_vm(id, vm, conf) {
 		if (!vm)
 			continue;
@@ -347,7 +372,7 @@ static void get_last_line_synced(struct sender_t *sender)
 			continue;
 
 		/* generally only exec for each vm once */
-		if (vm->last_synced_line_key[sender_id(sender)][0])
+		if (vm->last_synced_line_key[sid][0])
 			continue;
 
 		snprintf(word, sizeof(word), "%s ", vm->name);
@@ -406,6 +431,7 @@ static char *setup_loop_dev(void)
 	 * launch_UOS.sh, we need mount its data partition to loop device
 	 */
 	char *out;
+	char *end;
 	char loop_dev_tmp[32];
 	int i;
 
@@ -440,7 +466,9 @@ static char *setup_loop_dev(void)
 			}
 		}
 	}
-	*strchr(loop_dev, '\n') = 0;
+	end = strchr(loop_dev, '\n');
+	if (end)
+		*end = 0;
 
 	out = exec_out2mem("fdisk -lu %s", android_img);
 	if (!out) {

tools/acrn-crashlog/acrnprobe/channels.c

@@ -41,9 +41,9 @@ static struct channel_t channels[] = {
 };
 
 #define for_each_channel(i, channel) \
-	for (i = 0, channel = &channels[0]; \
-	     i < (int)ARRAY_SIZE(channels); \
-	     i++, channel++)
+	for (i = 0; \
+	     (i < (int)ARRAY_SIZE(channels)) && (channel = &channels[i]); \
+	     i++)
 
 /**
  * Helper function to create a event and fill event_t structure.
@@ -53,30 +53,35 @@ static struct channel_t channels[] = {
  * @param private The corresponding configuration info to the event.
  * @param watchfd For watch channel, so far, only used by inotify.
  * @param path File which trigger this event.
- * @param len Length of path.
  *
  * @return a pointer to the filled event if successful,
  *  or NULL on error.
  */
 static struct event_t *create_event(enum event_type_t event_type,
 				char *channel, void *private,
-				int watchfd, char *path, int len)
+				int watchfd, char *path)
 {
 	struct event_t *e;
+	int path_len = 0;
 
-	if (len < 0)
-		return NULL;
+	if (path) {
+		path_len = strlen(path);
+		if (path_len > PATH_MAX) {
+			LOGE("invalid path, drop event.\n");
+			return NULL;
+		}
+	}
 
-	e = malloc(sizeof(*e) + len + 1);
+	e = malloc(sizeof(*e) + path_len + 1);
 	if (e) {
-		memset(e, 0, sizeof(*e) + len + 1);
+		memset(e, 0, sizeof(*e) + path_len + 1);
 		e->watchfd = watchfd;
 		e->channel = channel;
-		e->private = (void *)private;
+		e->private = private;
 		e->event_type = event_type;
-		if (path && (len > 0)) {
-			e->len = len;
-			strncpy(e->path, path, len);
+		if (path_len > 0) {
+			e->len = path_len;
+			memcpy(e->path, path, path_len + 1);
 		}
 	} else {
 		LOGE("malloc failed, error (%s)\n", strerror(errno));
@@ -100,7 +105,7 @@ static void channel_oneshot(struct channel_t *cnl)
 
 	LOGD("initializing channel %s ...\n", cname);
 
-	e = create_event(REBOOT, cname, NULL, 0, NULL, 0);
+	e = create_event(REBOOT, cname, NULL, 0, NULL);
 	if (e)
 		event_enqueue(e);
 
@@ -116,8 +121,7 @@ static void channel_oneshot(struct channel_t *cnl)
 		    !strcmp("file", crash->trigger->type) &&
 		    file_exists(crash->trigger->path)) {
 			e = create_event(CRASH, cname, (void *)crash,
-					 0, crash->trigger->path,
-					 strlen(crash->trigger->path));
+					 0, crash->trigger->path);
 			if (e)
 				event_enqueue(e);
 		}
@@ -134,7 +138,7 @@ static void channel_oneshot(struct channel_t *cnl)
 		    !strcmp("file", info->trigger->type) &&
 		    file_exists(info->trigger->path)) {
 			e = create_event(INFO, cname, (void *)info,
-					 0, NULL, 0);
+					 0, NULL);
 			if (e)
 				event_enqueue(e);
 		}
@@ -156,7 +160,7 @@ static struct polling_job_t {
 static void polling_vm(union sigval v __attribute__((unused)))
 {
 
-	struct event_t *e = create_event(VM, "polling", NULL, 0, NULL, 0);
+	struct event_t *e = create_event(VM, "polling", NULL, 0, NULL);
 
 	if (e)
 		event_enqueue(e);
@@ -319,6 +323,7 @@ static int receive_inotify_events(struct channel_t *channel)
 	int read_left;
 	char buf[256];
 	char *p;
+	char *name;
 	struct event_t *e;
 	struct inotify_event *ievent;
 	enum event_type_t event_type;
@@ -360,9 +365,14 @@ static int receive_inotify_events(struct channel_t *channel)
 			if (event_type == UNKNOWN) {
 				LOGE("get a unknown event\n");
 			} else {
+				if (!ievent->len)
+					name = NULL;
+				else
+					name = ievent->name;
+
 				e = create_event(event_type, channel->name,
 						 private, channel->fd,
-						 ievent->name, ievent->len);
+						 name);
 				if (e)
 					event_enqueue(e);
 			}
@@ -384,7 +394,7 @@ static int receive_inotify_events(struct channel_t *channel)
  */
 static void heart_beat(void)
 {
-	struct event_t *e = create_event(HEART_BEAT, NULL, NULL, 0, NULL, 0);
+	struct event_t *e = create_event(HEART_BEAT, NULL, NULL, 0, NULL);
 
 	if (e)
 		event_enqueue(e);

tools/acrn-crashlog/acrnprobe/crash_reclassify.c

@@ -233,7 +233,7 @@ static struct crash_t *crash_reclassify_by_content(struct crash_t *rcrash,
 		return NULL;
 
 	if (trfile) {
-		ret = read_full_binary_file(trfile, &size, &file);
+		ret = read_file(trfile, &size, &file);
 		if (ret == -1) {
 			LOGE("read %s failed, error (%s)\n",
 			     trfile, strerror(errno));

tools/acrn-crashlog/acrnprobe/history.c

@@ -132,16 +132,21 @@ void hist_raise_event(char *event, char *type, char *log, char *lastuptime,
 
 	/* here means user have configured the crashlog sender */
 	crashlog = get_sender_by_name("crashlog");
+	if (!crashlog)
+		return;
+
 	maxlines = atoi(crashlog->maxlines);
 	if (++current_lines >= maxlines) {
 		LOGW("lines of (%s) meet quota %d, backup... Pls clean!\n",
 		     history_file, maxlines);
 		backup_history();
 	}
 
-	get_current_time_long(eventtime);
-	entry.eventtime = eventtime;
+	ret = get_current_time_long(eventtime);
+	if (ret <= 0)
+		return;
 
+	entry.eventtime = eventtime;
 	entry_to_history_line(&entry, line);
 	ret = append_file(history_file, line);
 	if (ret < 0) {
@@ -165,6 +170,9 @@ void hist_raise_uptime(char *lastuptime)
 
 	/* user have configured the crashlog sender */
 	crashlog = get_sender_by_name("crashlog");
+	if (!crashlog)
+		return;
+
 	uptime = crashlog->uptime;
 	uptime_hours = atoi(uptime->eventhours);
 

tools/acrn-crashlog/acrnprobe/include/load_conf.h

@@ -111,54 +111,54 @@ struct conf_t {
 struct conf_t conf;
 
 #define for_each_sender(id, sender, conf) \
-		for (id = 0, sender = conf.sender[0]; \
-		     id < SENDER_MAX; \
-		     id++, sender = conf.sender[id])
+		for (id = 0; \
+		     id < SENDER_MAX && (sender = conf.sender[id]); \
+		     id++)
 
 #define for_each_trigger(id, trigger, conf) \
-		for (id = 0, trigger = conf.trigger[0]; \
-		     id < TRIGGER_MAX; \
-		     id++, trigger = conf.trigger[id])
+		for (id = 0; \
+		     id < TRIGGER_MAX && (trigger = conf.trigger[id]); \
+		     id++)
 
 #define for_each_vm(id, vm, conf) \
-		for (id = 0, vm = conf.vm[0]; \
-		     id < VM_MAX; \
-		     id++, vm = conf.vm[id])
+		for (id = 0; \
+		     id < VM_MAX && (vm = conf.vm[id]); \
+		     id++)
 
 #define for_each_syncevent_vm(id, event, vm) \
-		for (id = 0, event = vm->syncevent[0]; \
-		     id < VM_EVENT_TYPE_MAX; \
-		     id++, event = vm->syncevent[id])
+		for (id = 0; \
+		     id < VM_EVENT_TYPE_MAX && (event = vm->syncevent[id]); \
+		     id++)
 
 #define for_each_info(id, info, conf) \
-		for (id = 0, info = conf.info[0]; \
-		     id < INFO_MAX; \
-		     id++, info = conf.info[id])
+		for (id = 0; \
+		     id < INFO_MAX && (info = conf.info[id]); \
+		     id++)
 
 #define for_each_log(id, log, conf) \
-		for (id = 0, log = conf.log[0]; \
-		     id < LOG_MAX; \
-		     id++, log = conf.log[id])
+		for (id = 0; \
+		     id < LOG_MAX && (log = conf.log[id]); \
+		     id++)
 
 #define for_each_crash(id, crash, conf) \
-		for (id = 0, crash = conf.crash[0]; \
-		     id < CRASH_MAX; \
-		     id++, crash = conf.crash[id])
+		for (id = 0; \
+		     id < CRASH_MAX && (crash = conf.crash[id]); \
+		     id++)
 
 #define for_each_log_collect(id, log, type) \
-		for (id = 0, log = type->log[0]; \
-		     id < LOG_MAX; \
-		     id++, log = type->log[id])
+		for (id = 0; \
+		     id < LOG_MAX && (log = type->log[id]); \
+		     id++)
 
 #define for_each_content_crash(id, content, crash) \
-		for (id = 0, content = crash->content[0]; \
-		     id < CONTENT_MAX; \
-		     id++, content = crash->content[id])
+		for (id = 0; \
+		     id < CONTENT_MAX && (content = crash->content[id]); \
+		     id++)
 
 #define for_each_content_expression(id, content, exp) \
-		for (id = 0, content = exp[0]; \
-		     id < CONTENT_MAX; \
-		     id++, content = exp[id])
+		for (id = 0; \
+		     id < CONTENT_MAX && (content = exp[id]); \
+		     id++)
 
 #define exp_valid(exp) \
 (__extension__ \
@@ -175,9 +175,9 @@ struct conf_t conf;
 )
 
 #define for_each_expression_crash(id, exp, crash) \
-		for (id = 0, exp = crash->mightcontent[0]; \
-		     id < EXPRESSION_MAX; \
-		     id++, exp = crash->mightcontent[id])
+		for (id = 0; \
+		     id < EXPRESSION_MAX && (exp = crash->mightcontent[id]); \
+		     id++)
 
 #define for_crash_children(crash, tcrash) \
 			TAILQ_FOREACH(crash, &tcrash->children, entries)