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

Replacing sprintf with snprintf to avoid overflow. #114

Closed
wants to merge 2 commits into from

Conversation

awiswasi
Copy link

Describe the contribution
Replaced sprintf() with snprintf() to avoid overflow

Testing performed
CTest

Expected behavior changes
Protect against overflow by setting the buffer to n+1 of the char String[50] to avoid overflow that may occur.

System(s) tested on

  • Hardware: PC, Raspberry Pi
  • OS: Ubuntu 22.04 LTS, Windows 10

Additional context
Add any other context about the contribution here.

Third party code
If included, identify any third party code and provide text file of license

Contributor Info - All information REQUIRED for consideration of pull request
Awf Wiswasi - California State University, Fullerton

elf2cfetbl.c Outdated

if (Status == FAILED)
{
sprintf(Result, Map->String, Key);
// Using snprintf to avoid overflow
snprintf(Result, 49, Map->String, Key);

Check failure

Code scanning / CodeQL-security

Non-constant format string

The format string argument to snprintf should be constant to prevent security issues and other potential errors.
Copy link
Contributor

@skliper skliper left a comment

Choose a reason for hiding this comment

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

Looks like whitespace changes were applied We currently use clang-format-10 w/ a config file at the cFS level, see the workflow in cFS for install and application steps.

Copy link
Author

@awiswasi awiswasi left a comment

Choose a reason for hiding this comment

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

I used the cFS clang-format and reuploaded the changes. My apologies as I am new to Clang and formatting, I was trying to contribute to the best of my knowledge. I think the whitespace formatting should be fixed now.

Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

Unfortunately this doesn't really fix the problem because it does not use the correct/actual buffer size ... and furthermore this is just one sprintf() call among many, in a tool that has long been on a list to deprecate/rewrite/replace.

Recommend to not merge this one

@dzbaker
Copy link
Collaborator

dzbaker commented Feb 2, 2023

CCB 2 February 2023: Closing PR. Many instances of sprintf exist across elf2cfetbl, and the tool will likely be replaced in the future.

@dzbaker dzbaker closed this Feb 2, 2023
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