From 3e8e607d5ba95803ce7a58cf4d9cd62c4f6e7e9d Mon Sep 17 00:00:00 2001 From: xiaojin2 Date: Fri, 8 Jun 2018 13:15:29 +0800 Subject: [PATCH] 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 Reviewed-by: Zhi Jin Acked-by: Chen Gang --- .../acrn-crashlog/acrnprobe/android_events.c | 36 ++++++++-- tools/acrn-crashlog/acrnprobe/channels.c | 50 ++++++++------ .../acrnprobe/crash_reclassify.c | 2 +- tools/acrn-crashlog/acrnprobe/history.c | 12 +++- .../acrnprobe/include/load_conf.h | 66 +++++++++---------- tools/acrn-crashlog/acrnprobe/probeutils.c | 13 ++-- tools/acrn-crashlog/acrnprobe/sender.c | 11 +++- 7 files changed, 123 insertions(+), 67 deletions(-) diff --git a/tools/acrn-crashlog/acrnprobe/android_events.c b/tools/acrn-crashlog/acrnprobe/android_events.c index e49a32fff..476c5c7dc 100644 --- a/tools/acrn-crashlog/acrnprobe/android_events.c +++ b/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 @@ out: 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) { diff --git a/tools/acrn-crashlog/acrnprobe/channels.c b/tools/acrn-crashlog/acrnprobe/channels.c index 1ac175084..872507ff1 100644 --- a/tools/acrn-crashlog/acrnprobe/channels.c +++ b/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); diff --git a/tools/acrn-crashlog/acrnprobe/crash_reclassify.c b/tools/acrn-crashlog/acrnprobe/crash_reclassify.c index 316c2af04..1eabd4458 100644 --- a/tools/acrn-crashlog/acrnprobe/crash_reclassify.c +++ b/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)); diff --git a/tools/acrn-crashlog/acrnprobe/history.c b/tools/acrn-crashlog/acrnprobe/history.c index 30514e575..24cae832d 100644 --- a/tools/acrn-crashlog/acrnprobe/history.c +++ b/tools/acrn-crashlog/acrnprobe/history.c @@ -132,6 +132,9 @@ 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", @@ -139,9 +142,11 @@ void hist_raise_event(char *event, char *type, char *log, char *lastuptime, 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); diff --git a/tools/acrn-crashlog/acrnprobe/include/load_conf.h b/tools/acrn-crashlog/acrnprobe/include/load_conf.h index 05a56d933..b696e003c 100644 --- a/tools/acrn-crashlog/acrnprobe/include/load_conf.h +++ b/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) diff --git a/tools/acrn-crashlog/acrnprobe/probeutils.c b/tools/acrn-crashlog/acrnprobe/probeutils.c index ab8d096ce..1564581dd 100644 --- a/tools/acrn-crashlog/acrnprobe/probeutils.c +++ b/tools/acrn-crashlog/acrnprobe/probeutils.c @@ -79,10 +79,10 @@ int get_current_time_long(char buf[32]) time(&t); time_val = localtime((const time_t *)&t); + if (!time_val) + return -1; - strftime(buf, 32, "%Y-%m-%d/%H:%M:%S ", time_val); - - return 0; + return strftime(buf, 32, "%Y-%m-%d/%H:%M:%S ", time_val); } /** @@ -286,6 +286,9 @@ static int reserve_log_folder(enum e_dir_mode mode, char *dir, unsigned int maxdirs; crashlog = get_sender_by_name("crashlog"); + if (!crashlog) + return -1; + outdir = crashlog->outdir; switch (mode) { @@ -352,7 +355,9 @@ void generate_crashfile(char *dir, char *event, char *hashkey, const int fmtsize = 128; int filesize; - get_current_time_long(datetime); + ret = get_current_time_long(datetime); + if (ret <= 0) + return; get_uptime_string(uptime, &hours); filesize = fmtsize + strlen(event) + diff --git a/tools/acrn-crashlog/acrnprobe/sender.c b/tools/acrn-crashlog/acrnprobe/sender.c index cedda8c87..91b0ab3d4 100644 --- a/tools/acrn-crashlog/acrnprobe/sender.c +++ b/tools/acrn-crashlog/acrnprobe/sender.c @@ -185,7 +185,7 @@ static void get_log_rotation(struct log_t *log, char *desdir) if (count > 2) { for (i = 0; i < count; i++) { name = strrchr(files[i], '/') + 1; - if (!strstr(name, prefix)) + if (!name && !strstr(name, prefix)) continue; number = atoi(strrchr(name, '.') + 1); @@ -793,7 +793,7 @@ static void crashlog_send_crash(struct event_t *e) char *data1; char *data2; int id; - int ret; + int ret = 0; int quota; struct crash_t *rcrash = (struct crash_t *)e->private; @@ -931,8 +931,13 @@ static void crashlog_send_reboot(void) { char reason[MAXLINESIZE]; char *key; + struct sender_t *crashlog; - if (swupdated(get_sender_by_name("crashlog"))) { + crashlog = get_sender_by_name("crashlog"); + if (!crashlog) + return; + + if (swupdated(crashlog)) { key = generate_event_id("INFO", "SWUPDATE"); if (key == NULL) { LOGE("generate event id failed, error (%s)\n",