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

replace atoi with strtol #6

Merged
merged 10 commits into from
Jun 27, 2023
Merged

Conversation

maipbui
Copy link
Contributor

@maipbui maipbui commented May 1, 2023

From Semgrep: https://semgrep.dev/r?q=c.lang.correctness.incorrect-use-ato-fn.incorrect-use-ato-fn

Avoid the 'ato*()' family of functions. Their use can lead to undefined behavior, integer overflows, and lack of appropriate error handling. Instead prefer the 'strtol*()' family of functions.

From atoi() man page: https://www.man7.org/linux/man-pages/man3/atoi.3.html

The atoi() function converts the initial portion of the string pointed to by nptr to int. The behavior is the same as
strtol(nptr, NULL, 10);
except that atoi() does not detect errors.

Therefore, replace atoi() with strtol()

Verified by kill dhcpmon process, installing new .deb change to dhcp_relay docker, execute dhcpmon process, check if counters is printing in syslog, compare counters before and after this change.
logs:
before.txt
after.txt

example commands:
command.txt

@maipbui
Copy link
Contributor Author

maipbui commented May 2, 2023

/azp run sonic-net.sonic-dhcpmon

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 6 in repo sonic-net/sonic-dhcpmon

@maipbui
Copy link
Contributor Author

maipbui commented May 2, 2023

/azpw run sonic-net.sonic-dhcpmon

@mssonicbld
Copy link
Collaborator

/AzurePipelines run sonic-net.sonic-dhcpmon

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 6 in repo sonic-net/sonic-dhcpmon

@maipbui maipbui marked this pull request as ready for review May 2, 2023 20:06
@maipbui
Copy link
Contributor Author

maipbui commented May 2, 2023

@qiluo-msft @kellyyeh could you help review this PR? I don't have permission to add reviewers.

@qiluo-msft qiluo-msft requested a review from kellyyeh May 2, 2023 20:49
src/main.cpp Outdated
@@ -150,15 +150,15 @@ int main(int argc, char **argv)
i++;
break;
case 's':
snaplen = atoi(argv[i + 1]);
snaplen = strtol(argv[i + 1], NULL, 10);
Copy link

@qiluo-msft qiluo-msft May 2, 2023

Choose a reason for hiding this comment

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

strtol

This new function has better error handling, you may consider add more code to check for error cases, and report back to user. #Closed

maipbui added 4 commits May 5, 2023 15:45
Signed-off-by: Mai Bui <maibui@microsoft.com>
Signed-off-by: Mai Bui <maibui@microsoft.com>
Signed-off-by: Mai Bui <maibui@microsoft.com>
Signed-off-by: Mai Bui <maibui@microsoft.com>
src/main.cpp Outdated
@@ -116,9 +117,10 @@ int main(int argc, char **argv)
int i;
int window_interval = dhcpmon_default_health_check_window;
int max_unhealthy_count = dhcpmon_default_unhealthy_max_count;
size_t snaplen = dhcpmon_default_snaplen;
int snaplen = dhcpmon_default_snaplen;
Copy link

@qiluo-msft qiluo-msft May 5, 2023

Choose a reason for hiding this comment

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

int

In theory, it is long int. #Closed

Choose a reason for hiding this comment

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

Sorry for misleading. Original types are good.

maipbui added 3 commits May 5, 2023 19:21
Signed-off-by: Mai Bui <maibui@microsoft.com>
Signed-off-by: Mai Bui <maibui@microsoft.com>
Signed-off-by: Mai Bui <maibui@microsoft.com>
@maipbui
Copy link
Contributor Author

maipbui commented May 5, 2023

@kellyyeh could you help review?

@qiluo-msft qiluo-msft requested a review from jcaiMR May 9, 2023 23:10
@qiluo-msft
Copy link

/azp run

@qiluo-msft
Copy link

/azp run sonic-net.sonic-dhcpmon

@qiluo-msft
Copy link

/azp run sonic-net.sonic-dhcpmon

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qiluo-msft qiluo-msft merged commit 824a144 into sonic-net:master Jun 27, 2023
@maipbui maipbui deleted the replace_atoi branch June 27, 2023 00:20
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.

5 participants