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

strutils: fix buffer overflow in formatBiggestFloat #1423

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

alaviss
Copy link
Contributor

@alaviss alaviss commented Aug 19, 2024

Summary

Fixed a potential buffer overflow in struts.formatBiggestFloat.

Details

Previously, strutils.formatBiggestFloat was implemented using
sprintf and as such came with a risk of buffer overflow. This was
observed via tests in disruptek/insideout.

  • Replaced sprintf with snprintf.
  • Remove the 2 KiB temporary buffer in favor of directly outputting to
    the target buffer.
  • Added extra error checking code for errors from snprintf.

Previously, strutils.formatBiggestFloat was implemented using sprintf
and as such come with a risk of buffer overflow. This was observed via
tests within disruptek/insideout.

Switch the implementation to snprintf and remove the temporary buffer
used. This solves the overflow and shave ~2 KiB from the stack size
requirement for this procedure.

Additionally, extra error checking code is added for potential failures
of snprintf.
@alaviss alaviss added bug Something isn't working stdlib Standard library labels Aug 19, 2024
Copy link
Collaborator

@saem saem left a comment

Choose a reason for hiding this comment

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

Don't suppose there is an easy enough reproduction for a test, or do you think it's too implementation specific?

lib/pure/strutils.nim Show resolved Hide resolved
@alaviss
Copy link
Contributor Author

alaviss commented Aug 19, 2024

Don't suppose there is an easy enough reproduction for a test, or do you think it's too implementation specific?

It's too implementation specific and won't be of any help if we move to a native impl at one point.

@saem
Copy link
Collaborator

saem commented Aug 19, 2024

/merge

(updated the PR body slightly)

Copy link

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:


Notes for Reviewers

  • While Dragonbox is bundled with the project, it is not suitable for generating fixed-precision numbers. As such the current printf-based method continues to be used and migration to a native algorithm will have to be done later.

@chore-runner chore-runner bot added this pull request to the merge queue Aug 19, 2024
Merged via the queue into nim-works:devel with commit 3ad12ae Aug 19, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stdlib Standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants