Skip to content

fix Klocwork errors and criticals from logger sources #6738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aborisovich wants to merge 1 commit into from
Closed

fix Klocwork errors and criticals from logger sources #6738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions tools/logger/convert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,8 @@ static const char *format_uid(uint32_t uid_ptr, int use_colors, bool be, bool up
if (uid_ptr < uids_dict->base_address ||
uid_ptr >= uids_dict->base_address + uids_dict->data_length) {
str = calloc(1, strlen(BAD_PTR_STR) + 1 + 6);
sprintf(str, BAD_PTR_STR, uid_ptr);
if (str)
sprintf(str, BAD_PTR_STR, uid_ptr);
Copy link
Collaborator

@marc-hb marc-hb Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this to some assert. If it started to happen because of some bug or other catastrophe then it may happen again later, shouldn't keep going and leave random and confusing gaps in the logs.

} else {
uid_entry = get_uuid_entry(uid_ptr);
str = format_uid_raw(uid_entry, use_colors, 1, be, upper);
Expand Down Expand Up @@ -358,8 +359,10 @@ static inline void print_table_header(void)
/* e.g.: ktime=4263.487s @ 2021-04-27 14:21:13 -0700 PDT */
fprintf(out_fd, "\tktime=%lu.%03lus",
ktime.tv_sec, ktime.tv_nsec / 1000000);
if (strftime(date_string, sizeof(date_string),
"%F %X %z %Z", localtime(&epoc_secs)))
struct tm *info = localtime(&epoc_secs);

if (info && strftime(date_string, sizeof(date_string),
"%F %X %z %Z", localtime(&epoc_secs)))
fprintf(out_fd, " @ %s", date_string);
}

Expand Down Expand Up @@ -961,7 +964,8 @@ static int verify_ldc_checksum(const uint32_t ldc_sum)
count = fread(&ver, sizeof(ver), 1, global_config->version_fd);
if (!count) {
log_err("Error while reading %s.\n", global_config->version_file);
return -ferror(global_config->version_fd);
int result = -ferror(global_config->version_fd);
return result;
Copy link
Collaborator

@paulstelian97 paulstelian97 Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointless refactor.

Copy link
Contributor Author

@aborisovich aborisovich Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not pointless refactor. Calling return on ferror causes a dangling pointer to global_config, which is no longer in scope. And static analysis tools reports that fact.

Copy link
Collaborator

@paulstelian97 paulstelian97 Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific change would also have the exact same dangling pointer because it literally compiles to the same thing if you have at least -O1...

Copy link
Collaborator

@marc-hb marc-hb Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is fatal, the tool exits when it happens. KW is not smart enough to see that, this is yet another false positive.

Indeed this change makes absolutely no difference, I hope KW is not so dumb to be deceived by it.

}

/* compare source hash value from version file and ldc file */
Expand Down Expand Up @@ -1017,6 +1021,10 @@ static int dump_ldc_info(void)
while (remaining > 0) {
name = format_uid_raw(&uid_ptr[cnt], 0, 0, false, false);
uid_addr = get_uuid_key(&uid_ptr[cnt]);
if (!name) {
log_err("Failed to allocate memory for uid name\n");
exit(1);
}
fprintf(out_fd, "\t%p %s\n", (void *)uid_addr, name);

if (name) {
Expand Down Expand Up @@ -1045,19 +1053,25 @@ int convert(struct convert_config *config)
count = fread(&snd, sizeof(snd), 1, config->ldc_fd);
if (!count) {
log_err("Error while reading %s.\n", config->ldc_file);
return -ferror(config->ldc_fd);
int result = -ferror(config->ldc_fd);

config = NULL;
return result;
Copy link
Collaborator

@paulstelian97 paulstelian97 Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointless refactor.

Copy link
Contributor Author

@aborisovich aborisovich Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that you do not see the issue is not meaning that it is pointless refactor.

Copy link
Collaborator

@paulstelian97 paulstelian97 Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is pointless because it fixes absolutely nothing here.

Copy link
Contributor

@kfrydryx kfrydryx Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error in this return rather comes from line 1048, config->logs_header = &snd;. So on return, we're left with config->logs_header (config is passed here as a ptr from scope higher) and local variable address (now dangling) from local scope.

Copy link
Collaborator

@paulstelian97 paulstelian97 Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the issue is still weird -- why do you set a local variable to NULL? That thing can get optimized out and you still get the same code. If "config" is a global variable (so that this change does become meaningful) disregard.

Copy link
Contributor Author

@aborisovich aborisovich Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config is not local variable, it is function argument here int convert(struct convert_config *config);

Copy link
Collaborator

@paulstelian97 paulstelian97 Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function parameters are still local variables actually.

}

if (strncmp((char *) snd.sig, SND_SOF_LOGS_SIG, SND_SOF_LOGS_SIG_SIZE)) {
log_err("Invalid ldc file signature.\n");
config = NULL;
return -EINVAL;
}

if (global_config->version_fw && /* -n option */
!global_config->dump_ldc) {
ret = verify_ldc_checksum(global_config->logs_header->version.src_hash);
if (ret)
if (ret) {
config = NULL;
return ret;
}
}

/* default logger and ldc_file abi verification */
Expand All @@ -1073,6 +1087,7 @@ int convert(struct convert_config *config)
SOF_ABI_VERSION_MAJOR(snd.version.abi_version),
SOF_ABI_VERSION_MINOR(snd.version.abi_version),
SOF_ABI_VERSION_PATCH(snd.version.abi_version));
config = NULL;
return -EINVAL;
}

Expand All @@ -1081,7 +1096,10 @@ int convert(struct convert_config *config)
count = fread(&uids_hdr, sizeof(uids_hdr), 1, config->ldc_fd);
if (!count) {
log_err("Error while reading uuids header from %s.\n", config->ldc_file);
return -ferror(config->ldc_fd);
int result = -ferror(config->ldc_fd);

config = NULL;
return result;
Copy link
Collaborator

@paulstelian97 paulstelian97 Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiler error (due to typo) and it's a pointless refactor anyway.

After a change: no longer typo, but it still remains pointless if "config" is a local variable.

}
if (strncmp((char *)uids_hdr.sig, SND_SOF_UIDS_SIG,
SND_SOF_UIDS_SIG_SIZE)) {
Expand Down Expand Up @@ -1118,5 +1136,6 @@ int convert(struct convert_config *config)
ret = logger_read();
out:
free(config->uids_dict);
config = NULL;
return ret;
}
5 changes: 4 additions & 1 deletion tools/logger/logger.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ int main(int argc, char *argv[])

if (config.input_std) {
config.in_fd = stdin;
} else if (baud) {
} else if (baud && config.in_file) {
Copy link
Collaborator

@paulstelian97 paulstelian97 Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only meaningfully good change I can see.

config.serial_fd = configure_uart(config.in_file, baud);
if (config.serial_fd < 0) {
ret = -config.serial_fd;
Expand Down Expand Up @@ -508,5 +508,8 @@ int main(int argc, char *argv[])
if (config.version_fd)
fclose(config.version_fd);

if (config.in_file)
fclose(config.in_file);

return ret;
}