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

[stepanblyschak]

Signed-off-by: Stepan Blyschak [email protected]

Why I did it

Fix an issue that services do not start automatically on first boot and start only after hostcfgd enables them.
This is due to a bug in systemd-sonic-generator:

admin@arc-switch1004:~$ /usr/lib/systemd/system-generators/systemd-sonic-generator dir
Failed to open file /usr/lib/systemd/system/database.servcee
Error parsing targets for database.servcee
Error parsing database.servcee
Failed to open file /usr/lib/systemd/system/bgp.servcee
Error parsing targets for bgp.servcee
Error parsing bgp.servcee
Failed to open file /usr/lib/systemd/system/lldp.servcee
Error parsing targets for lldp.servcee
Error parsing lldp.servcee
Failed to open file /usr/lib/systemd/system/swss.servcee
Error parsing targets for swss.servcee
Error parsing swss.servcee
Failed to open file /usr/lib/systemd/system/teamd.servcee
Error parsing targets for teamd.servcee
Error parsing teamd.servcee
Failed to open file /usr/lib/systemd/system/syncd.servcee
Error parsing targets for syncd.servcee
Error parsing syncd.servcee

A wrong file name is generated (e.g database.servcee).

How I did it

Fixed overlapping strings being passed to strcpy/strcat that receive restirct* pointers (strings should not overlap).

How to verify it

Perform first boot and observe services start immidiatelly after boot.

Which release branch to backport (provide reason below if selected)

• 201811
• 201911
• 202006
• 202012
• 202106
• 202111
• 202205
• 202211

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 under sonic-utilities repo. where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

[liat-grozovik]

/azp run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[stepanblyschak]

/azpw run Azure.sonic-buildimage

[mssonicbld]

/AzurePipelines run Azure.sonic-buildimage

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[qiluo-msft]

strdup

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

[stepanblyschak]

@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.

[SuvarnaMeenakshi]

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[] = "[email protected]";
        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);
}
[email protected]

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

[qiluo-msft]

@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]

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

[stepanblyschak]

@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.

reset

[mssonicbld]

Cherry-pick PR to 202211: #13891

[mssonicbld]

Cherry-pick PR to 202205: #13892