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

Change sprintf to snprintf #24

Closed
wants to merge 3 commits into from
Closed

Conversation

Damien-Chen
Copy link
Contributor

@Damien-Chen Damien-Chen commented Jul 29, 2024

sprintf does not perform any bounds checking, which could potentially lead to occurence of buffer overflow and cause undefined behavior.

snprintf require to specify the size of buffer so that it is able to prevent undefined behavior. Adding sizeof(v) + 1 to include null terminator.

Reference:
https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Read https://cbea.ms/git-commit/ carefully and refine your git commit messages. In addition, improve the subject of this pull request to express the motivations and considerations.

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Squash the commits. Do read https://cbea.ms/git-commit/ carefully and express the following:

  • Motivations
  • Considerations
  • Strong reasons to make such changes
  • Impacts
  • References

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Use git rebase instead of git merge.
Check the documentation: https://git-scm.com/docs/git-rebase

@jserv
Copy link
Contributor

jserv commented Jul 29, 2024

snprintf require to specify the size of buffer so that it is able to prevent undefined behavior. Adding sizeof(v) + 1 to include null terminator.

In the calculator application, do you think it is susceptible to a buffer overrun? If so, provide a minimal reproducible example to demonstrate the need for the proposed change.

@Damien-Chen
Copy link
Contributor Author

Yes you are right, current char array is 20-byte long, after investigation I think it is suffcient to store int type integer.

@Damien-Chen
Copy link
Contributor Author

However, there is one strange problem that once the calculator reach the limit of its length, next time I press a button of a number, the number in the calculator shows will randomly change to other number.

@jserv
Copy link
Contributor

jserv commented Jul 29, 2024

However, there is one strange problem that once the calculator reach the limit of its length, next time I press a button of a number, the number in the calculator shows will randomly change to other number.

Write down the steps to reproduce the issue, along with your analysis, and submit it in another GitHub issue.

@jserv jserv closed this Jul 29, 2024
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.

2 participants