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

[sairedis] Replace memset functions #1110

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

maipbui
Copy link
Contributor

@maipbui maipbui commented Aug 22, 2022

Signed-off-by: maipbui maibui@microsoft.com

Why I did it
memset() is an insecure function that can cause buffer overflow.
memset() may not be called by the compiler. Due to compiler optimization: an optimizing compiler, which identifies and removes the function that overwrites the contents as a dead store because the memory is not used subsequently. If sensitive data is in memory, it's dangerous if compiler optimization happens.
Ref:
https://cwe.mitre.org/data/definitions/14.html
https://registry.semgrep.dev/rule/c.lang.security.insecure-use-memset.insecure-use-memset

How I did it
Remove memset(), initialize the variable to 0.

@maipbui maipbui requested a review from qiluo-msft August 22, 2022 17:44
lib/Switch.cpp Outdated Show resolved Hide resolved
@kcudnik
Copy link
Collaborator

kcudnik commented Aug 23, 2022

can we use memset_s in all places instead doing ifdef ?

@maipbui maipbui force-pushed the sairedis_replace_memset branch from e343fdd to 9040fc9 Compare August 23, 2022 18:08
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui maipbui force-pushed the sairedis_replace_memset branch from 2d0aa51 to af946f8 Compare August 25, 2022 17:40
@maipbui
Copy link
Contributor Author

maipbui commented Aug 25, 2022

can we use memset_s in all places instead doing ifdef ?

I don't think so, seems like gcc/g++ doesn't support memset_s

Signed-off-by: maipbui <maibui@microsoft.com>
Signed-off-by: maipbui <maibui@microsoft.com>
Signed-off-by: maipbui <maibui@microsoft.com>
Signed-off-by: maipbui <maibui@microsoft.com>
@maipbui
Copy link
Contributor Author

maipbui commented Sep 8, 2022

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qiluo-msft
Copy link
Contributor

@kcudnik Could you review again?

@maipbui maipbui marked this pull request as ready for review September 12, 2022 22:52
@kcudnik
Copy link
Collaborator

kcudnik commented Sep 13, 2022

please satisfy code coverage: https://dev.azure.com/mssonic/build/_build/results?buildId=147452&view=codecoverage-tab in unittests

@kcudnik
Copy link
Collaborator

kcudnik commented Sep 27, 2022

i added some unittest to cover old code #1133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants