Skip to content
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

Jump to bottom

[systemd-sonic-generator] Fix overlapping strings being passed to strcpy/strcat #13647

Merged
merged 1 commit into from
Feb 18, 2023
Merged

[systemd-sonic-generator] Fix overlapping strings being passed to strcpy/strcat #13647

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
7 changes: 5 additions & 2 deletions src/systemd-sonic-generator/systemd-sonic-generator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,11 +608,14 @@ int ssg_main(int argc, char **argv) {
for (int i = 0; i < num_unit_files; i++) {
unit_instance = strdup(unit_files[i]);
if ((num_asics == 1) && strstr(unit_instance, "@") != NULL) {
prefix = strtok_r(unit_instance, "@", &saveptr);
suffix = strtok_r(NULL, "@", &saveptr);
prefix = strdup(strtok_r(unit_instance, "@", &saveptr));
Copy link
Collaborator

Choose a reason for hiding this comment

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

strdup

Thanks for fixing the bug! Could you extract this part into a small function, and add unit test?

Copy link
Collaborator Author

@stepanblyschak stepanblyschak Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@qiluo-msft This code has been already covered by UT. This bug is observed with -O2 -fstack-protector-strong but UT are not usually compiled with such flags. I think we need to run UT with some address sanitizer that can catch it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the fix.

Is my understanding correc that the issue is because we are using unit_instance variable in strtok_r and also to copy and create final unit_instance string?

If that is the case, will it be better to create a new string final_unit_instance to avoid any issue?

#include<string.h>
#include<stdio.h>
int main(){
        char unit_file[] = "database@.service";
        printf("\nunit_file=%s\n", unit_file);
        char *unit_instance = strdup(unit_file);
        char *prefix;
        char *suffix;
        char *save_ptr;

        prefix = strtok_r(unit_instance, "@", &save_ptr);
        printf("\nunit_instance = %s\n", unit_instance);
        suffix = strtok_r(NULL, "\n", &save_ptr);

        printf("\nprefix = %s\n", prefix);
        printf("\nprefix=%s, suffix=%s\n", prefix, suffix);
        printf("\nunit_instance=%s\n", unit_instance);
}
unit_file=database@.service

unit_instance = database

prefix = database

prefix=database, suffix=.service   -> should we concatenate the two to a new string and use that string in next steps?

unit_instance=database

Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@stepanblyschak strictly speaking, strdup is not efficient in this use case. If we can use C++, do we have much better solution? I found a C++20 solution https://stackoverflow.com/a/69550517/2514803.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@theasianpianist as original owner of this file claims that there is no enforcement to stick with C language.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@stepanblyschak strictly speaking, strdup is not efficient in this use case. If we can use C++, do we have much better solution? I found a C++20 solution https://stackoverflow.com/a/69550517/2514803.

@qiluo-msft I didn't aim to improve efficiency. With the current approach taken to to remove @ from a string, the algorithm is - tokenize into prefix and suffix and concatenate both - that requires memory allocation. This is not the most efficient way to solve this, I agree, but this is a blocking issue for us, so instead of rewriting algorithm a fix for the originally intended algorithm was proposed to fix a bug.

suffix = strdup(strtok_r(NULL, "@", &saveptr));

strcpy(unit_instance, prefix);
strcat(unit_instance, suffix);

free(prefix);
free(suffix);
}

num_targets = get_install_targets(unit_instance, targets);
Expand Down