From 63e6e6463a06728097edeed5350b37e73b5f0356 Mon Sep 17 00:00:00 2001 From: Kai Vehmanen Date: Thu, 20 Apr 2023 18:17:24 +0300 Subject: [PATCH 1/8] topology1: fix buffer size calculation if period-size >44ms Calculation of SOF_TKN_BUF_SIZE in COMP_PERIOD_FRAMES() macro led to incorrect results with large period size values. For example at 48000Hz sampling rate, period size larger than 44739us would be incorrectly calculated. This happens as m4 eval does arithmetic in 32bit signed values and multiplication of period size and sampling rate can easily exceed 2^31. Fix the issue by splitting the arithmetic in steps that fit available value range. Link: https://github.com/thesofproject/sof/issues/7476 Signed-off-by: Kai Vehmanen (cherry picked from commit be43b4e4ebb9e774a05318c4b54c0445c7938db3) --- tools/topology/topology1/m4/buffer.m4 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/topology/topology1/m4/buffer.m4 b/tools/topology/topology1/m4/buffer.m4 index 9a8f5b73f8f4..376e206c6c09 100644 --- a/tools/topology/topology1/m4/buffer.m4 +++ b/tools/topology/topology1/m4/buffer.m4 @@ -44,6 +44,8 @@ dnl COMP_BUFFER_SIZE( num_periods, sample_size, channels, fmames) define(`COMP_BUFFER_SIZE', `eval(`$1 * $2 * $3 * $4')') dnl COMP_PERIOD_FRAMES( sample_rate, period_us) -define(`COMP_PERIOD_FRAMES', `eval(`($1 * $2) / 1000000')') +dnl note: m4 eval arithmetic is 32bit signed, so split the 10^6 +dnl division to avoid overflow. +define(`COMP_PERIOD_FRAMES', `eval(`$1 / 100 * $2 / 10000')') divert(0)dnl From c5aa813ee620f778a2b48fff93c75dadf76f6df2 Mon Sep 17 00:00:00 2001 From: Adrian Warecki Date: Mon, 18 Sep 2023 15:08:26 +0200 Subject: [PATCH 2/8] smex: elf: Removed unnecessary initialization of local variables The values assigned when declaring variables were overwritten in the code. Redundant initialization was removed. Signed-off-by: Adrian Warecki --- smex/elf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/smex/elf.c b/smex/elf.c index bd088716691d..3aeb62e3d4fc 100644 --- a/smex/elf.c +++ b/smex/elf.c @@ -17,7 +17,7 @@ static int elf_read_sections(struct elf_module *module, bool verbose) { Elf32_Ehdr *hdr = &module->hdr; - Elf32_Shdr *section = module->section; + Elf32_Shdr *section; size_t count; int i, ret; uint32_t valid = (SHF_WRITE | SHF_ALLOC | SHF_EXECINSTR); @@ -128,7 +128,7 @@ static int elf_read_sections(struct elf_module *module, bool verbose) static int elf_read_programs(struct elf_module *module, bool verbose) { Elf32_Ehdr *hdr = &module->hdr; - Elf32_Phdr *prg = module->prg; + Elf32_Phdr *prg; size_t count; int i, ret; @@ -431,7 +431,7 @@ int elf_read_section(const struct elf_module *module, const char *section_name, const Elf32_Shdr **dst_section, void **dst_buff) { const Elf32_Shdr *section; - int section_index = -1; + int section_index; int read; section_index = elf_find_section(module, section_name); From ae60b936979bef4cc04d28c539c15f3fd48ec74e Mon Sep 17 00:00:00 2001 From: Adrian Warecki Date: Mon, 18 Sep 2023 15:10:11 +0200 Subject: [PATCH 3/8] smex: elf: Added checking of value returned by file operation function Added checking of the value returned by fseek function and added memory release when an error is detected. Signed-off-by: Adrian Warecki --- smex/elf.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/smex/elf.c b/smex/elf.c index 3aeb62e3d4fc..9a4f13be7d2c 100644 --- a/smex/elf.c +++ b/smex/elf.c @@ -432,7 +432,7 @@ int elf_read_section(const struct elf_module *module, const char *section_name, { const Elf32_Shdr *section; int section_index; - int read; + int ret; section_index = elf_find_section(module, section_name); if (section_index < 0) { @@ -451,17 +451,25 @@ int elf_read_section(const struct elf_module *module, const char *section_name, return -ENOMEM; /* fill buffer with section content */ - fseek(module->fd, section->off, SEEK_SET); - read = fread(*dst_buff, 1, section->size, module->fd); - if (read != section->size) { - fprintf(stderr, - "error: can't read %s section %d\n", section_name, - -errno); - free(*dst_buff); - return -errno; + ret = fseek(module->fd, section->off, SEEK_SET); + if (ret) { + fprintf(stderr, "error: can't seek to %s section %d\n", section_name, -errno); + ret = -errno; + goto error; + } + + ret = fread(*dst_buff, 1, section->size, module->fd); + if (ret != section->size) { + fprintf(stderr, "error: can't read %s section %d\n", section_name, -errno); + ret = -errno; + goto error; } return section->size; + +error: + free(*dst_buff); + return ret; } int elf_read_module(struct elf_module *module, const char *name, bool verbose) From 837c8d61055e792df316d2ef6fd7a4cad60d6540 Mon Sep 17 00:00:00 2001 From: Adrian Warecki Date: Mon, 18 Sep 2023 15:11:09 +0200 Subject: [PATCH 4/8] smex: elf: elf_find_section: Check function input data String terminator was added to the buffer with a list of section names in the elf file. Added check to the section name index to make sure it doesn't go beyond the buffer size. Signed-off-by: Adrian Warecki --- smex/elf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/smex/elf.c b/smex/elf.c index 9a4f13be7d2c..711e061bcdd7 100644 --- a/smex/elf.c +++ b/smex/elf.c @@ -408,10 +408,17 @@ int elf_find_section(const struct elf_module *module, const char *name) ret = -errno; goto out; } + buffer[section->size - 1] = '\0'; /* find section with name */ for (i = 0; i < hdr->shnum; i++) { s = &module->section[i]; + if (s->name >= section->size) { + fprintf(stderr, "error: invalid section name string index %d\n", s->name); + ret = -EINVAL; + goto out; + } + if (!strcmp(name, buffer + s->name)) { ret = i; goto out; From 792e9763f5c37eccff01c6d1ff08d233abcaacb5 Mon Sep 17 00:00:00 2001 From: Adrian Warecki Date: Mon, 18 Sep 2023 15:23:36 +0200 Subject: [PATCH 5/8] logger: convert: Fixed handling of an error reported by clock_gettime The clock_gettime function only returns information that an error occurred. The error code should be taken from the errno variable. Signed-off-by: Adrian Warecki --- tools/logger/convert.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/logger/convert.c b/tools/logger/convert.c index 569f81579f52..5cd3bfecd5d1 100644 --- a/tools/logger/convert.c +++ b/tools/logger/convert.c @@ -337,7 +337,7 @@ static inline void print_table_header(void) if (gettime_ret) { log_err("clock_gettime() failed: %s\n", - strerror(gettime_ret)); + strerror(errno)); exit(1); } From 08c11ccb7980c8d4d408d724e343b108d9ee47b8 Mon Sep 17 00:00:00 2001 From: Adrian Warecki Date: Mon, 18 Sep 2023 15:31:28 +0200 Subject: [PATCH 6/8] logger: convert: Simplified printing of a timestamp The timestamp printing process has been simplified by eliminating the dynamic creation of the formatting string. All necessary parameters are now passed directly to the printing function. Signed-off-by: Adrian Warecki --- tools/logger/convert.c | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-) diff --git a/tools/logger/convert.c b/tools/logger/convert.c index 5cd3bfecd5d1..3839df30b652 100644 --- a/tools/logger/convert.c +++ b/tools/logger/convert.c @@ -327,7 +327,6 @@ static inline void print_table_header(void) { FILE *out_fd = global_config->out_fd; int hide_location = global_config->hide_location; - char time_fmt[32]; char date_string[64]; const time_t epoc_secs = time(NULL); @@ -342,11 +341,9 @@ static inline void print_table_header(void) } if (global_config->time_precision >= 0) { - const unsigned int ts_width = - timestamp_width(global_config->time_precision); - snprintf(time_fmt, sizeof(time_fmt), "%%-%ds(us)%%%ds ", - ts_width, ts_width); - fprintf(out_fd, time_fmt, " TIMESTAMP", "DELTA"); + const unsigned int ts_width = timestamp_width(global_config->time_precision); + + fprintf(out_fd, "%*s(us)%*s ", -ts_width, " TIMESTAMP", ts_width, "DELTA"); } fprintf(out_fd, "%2s %-18s ", "C#", "COMPONENT"); @@ -461,7 +458,6 @@ static void print_entry_params(const struct log_entry_header *dma_log, char ids[TRACE_MAX_IDS_STR]; float dt = to_usecs(dma_log->timestamp - last_timestamp); struct proc_ldc_entry proc_entry; - static char time_fmt[64]; int ret; if (raw_output) @@ -502,13 +498,7 @@ static void print_entry_params(const struct log_entry_header *dma_log, ids[0] = '\0'; if (raw_output) { /* "raw" means script-friendly (not all hex) */ - const char *entry_fmt = "%s%u %u %s%s%s "; - - if (time_precision >= 0) - snprintf(time_fmt, sizeof(time_fmt), "%%.%df %%.%df ", - time_precision, time_precision); - - fprintf(out_fd, entry_fmt, + fprintf(out_fd, "%s%u %u %s%s%s ", entry->header.level == use_colors ? (LOG_LEVEL_CRITICAL ? KRED : KNRM) : "", dma_log->core_id, @@ -516,9 +506,12 @@ static void print_entry_params(const struct log_entry_header *dma_log, get_component_name(entry->header.component_class, dma_log->uid), raw_output && strlen(ids) ? "-" : "", ids); + if (time_precision >= 0) - fprintf(out_fd, time_fmt, - to_usecs(dma_log->timestamp - timestamp_origin), dt); + fprintf(out_fd, "%.*f %.*f ", + time_precision, to_usecs(dma_log->timestamp - timestamp_origin), + time_precision, dt); + if (!hide_location) fprintf(out_fd, "(%s:%u) ", format_file_name(entry->file_name, raw_output), @@ -527,13 +520,11 @@ static void print_entry_params(const struct log_entry_header *dma_log, if (time_precision >= 0) { const unsigned int ts_width = timestamp_width(time_precision); - snprintf(time_fmt, sizeof(time_fmt), - "%%s[%%%d.%df] (%%%d.%df)%%s ", - ts_width, time_precision, ts_width, time_precision); - - fprintf(out_fd, time_fmt, + fprintf(out_fd, "%s[%*.*f] (%*.*f)%s ", use_colors ? KGRN : "", - to_usecs(dma_log->timestamp - timestamp_origin), dt, + ts_width, time_precision, + to_usecs(dma_log->timestamp - timestamp_origin), + ts_width, time_precision, dt, use_colors ? KNRM : ""); } From 7bfad5884e1683d6ce6e214b84a731083db89418 Mon Sep 17 00:00:00 2001 From: Adrian Warecki Date: Mon, 18 Sep 2023 15:35:11 +0200 Subject: [PATCH 7/8] logger: convert: read_entry_from_ldc_file: Make sure string null terminated Added a null string terminator to be sure that strings read from a file are terminated correctly. Signed-off-by: Adrian Warecki --- tools/logger/convert.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tools/logger/convert.c b/tools/logger/convert.c index 3839df30b652..4c209ab04701 100644 --- a/tools/logger/convert.c +++ b/tools/logger/convert.c @@ -616,7 +616,7 @@ static int read_entry_from_ldc_file(struct ldc_entry *entry, uint32_t log_entry_ ret = -EINVAL; goto out; } - entry->file_name = (char *)malloc(entry->header.file_name_len); + entry->file_name = (char *)malloc(entry->header.file_name_len + 1); if (!entry->file_name) { log_err("can't allocate %d byte for entry.file_name\n", @@ -627,6 +627,8 @@ static int read_entry_from_ldc_file(struct ldc_entry *entry, uint32_t log_entry_ ret = fread(entry->file_name, sizeof(char), entry->header.file_name_len, global_config->ldc_fd); + entry->file_name[entry->header.file_name_len] = '\0'; + if (ret != entry->header.file_name_len) { log_err("Failed to read source filename for offset 0x%x in dictionary.\n", entry_offset); @@ -640,7 +642,7 @@ static int read_entry_from_ldc_file(struct ldc_entry *entry, uint32_t log_entry_ ret = -EINVAL; goto out; } - entry->text = (char *)malloc(entry->header.text_len); + entry->text = (char *)malloc(entry->header.text_len + 1); if (!entry->text) { log_err("can't allocate %d byte for entry.text\n", entry->header.text_len); ret = -ENOMEM; @@ -653,6 +655,7 @@ static int read_entry_from_ldc_file(struct ldc_entry *entry, uint32_t log_entry_ ret = -1; goto out; } + entry->text[entry->header.text_len] = '\0'; return 0; From c45e586eb56d62dcfb475755f102d817cf4fc978 Mon Sep 17 00:00:00 2001 From: Adrian Warecki Date: Mon, 18 Sep 2023 15:37:12 +0200 Subject: [PATCH 8/8] logger: convert: Code quality improvements The precision check condition has been simplified, the unsigned value cannot be negative. Added definitions containing an error message instead of using a constant variable. Signed-off-by: Adrian Warecki --- tools/logger/convert.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/logger/convert.c b/tools/logger/convert.c index 4c209ab04701..2e23f061c156 100644 --- a/tools/logger/convert.c +++ b/tools/logger/convert.c @@ -60,8 +60,7 @@ struct proc_ldc_entry { uintptr_t params[TRACE_MAX_PARAMS_COUNT]; }; -static const char *BAD_PTR_STR = ""; - +#define BAD_PTR_STR "" #define UUID_LOWER "%s%s%s<%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x>%s%s%s" #define UUID_UPPER "%s%s%s<%08X-%04X-%04X-%02X%02X-%02X%02X%02X%02X%02X%02X>%s%s%s" @@ -313,7 +312,7 @@ static unsigned int timestamp_width(unsigned int precision) * gcc 9.3, this avoids a very long precision causing snprintf() * to truncate time_fmt */ - assert(precision >= 0 && precision < 20); + assert(precision < 20); /* * 12 digits for units is enough for 1M seconds = 11 days which * should be enough for most test runs.