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.

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

[systemd-sonic-generator] replace sscanf with strtol #15357

Merged
merged 3 commits into from
Jun 22, 2023

Conversation

maipbui
Copy link
Contributor

@maipbui maipbui commented Jun 6, 2023

Why I did it

Avoid 'sscanf()' for number conversions. Its use can lead to undefined behavior, slow processing, and integer overflows. Instead prefer the 'strto*()' family of functions.

Work item tracking
  • Microsoft ADO (number only):

How I did it

replace sscanf with strtol

How to verify it

Manual test

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

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

Tested branch (Please provide the tested image version)

Description for the changelog

Link to config_db schema for YANG module changes

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

Signed-off-by: Mai Bui <maibui@microsoft.com>
char *endptr;
num_asic = strtol(str_num_asic, &endptr, 10);
if (*endptr != '\0') {
fprintf(stderr, "Failed to get num asics.\n");
Copy link
Collaborator

@qiluo-msft qiluo-msft Jun 6, 2023

Choose a reason for hiding this comment

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

fprintf

You will treat any extra chars after num as failure. I am thinking original sscanf just ignore them, and it's safe to ignore them using strtol.

How about just checking num_asic > 0 ? #Closed

Signed-off-by: Mai Bui <maibui@microsoft.com>
qiluo-msft
qiluo-msft previously approved these changes Jun 7, 2023
Copy link
Collaborator

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

LGTM

sscanf(str_num_asic, "%d",&num_asic);
num_asic = strtol(str_num_asic, NULL, 10);
if (num_asic > 0) {
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not understand the usage of break here?
If num_asic is not greater than 0, then what might happen?

In what scenario will this check be useful?
Previously, we were breaking after parsing the line starting with "NUM_ASIC=", we could still do the same without check for the value?

Signed-off-by: Mai Bui <maibui@microsoft.com>
@maipbui maipbui requested a review from SuvarnaMeenakshi June 8, 2023 19:03
@maipbui
Copy link
Contributor Author

maipbui commented Jun 9, 2023

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@SuvarnaMeenakshi SuvarnaMeenakshi left a comment

Choose a reason for hiding this comment

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

lgtm

@maipbui maipbui marked this pull request as ready for review June 22, 2023 20:13
@maipbui maipbui requested a review from lguohan as a code owner June 22, 2023 20:13
@qiluo-msft qiluo-msft merged commit fca3458 into sonic-net:master Jun 22, 2023
@maipbui maipbui deleted the systemd_sscanf branch June 22, 2023 20:19
sonic-otn pushed a commit to sonic-otn/sonic-buildimage that referenced this pull request Sep 20, 2023
#### Why I did it
Avoid 'sscanf()' for number conversions. Its use can lead to undefined behavior, slow processing, and integer overflows. Instead prefer the 'strto*()' family of functions.

#### How I did it
replace sscanf with strtol
#### How to verify it
Manual test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants