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

Prevent possible invalid memory accesses by sprintf #459

Merged
merged 3 commits into from
Nov 21, 2024

Conversation

Master92
Copy link
Contributor

@Master92 Master92 commented Nov 18, 2024

Update (2024-11-19)

Per the discussion below, I replaced all calls to sprintf with respective ones to snprintf that only writes as many chars as fit the target.

Original description

I compile the emulator locally with gcc-14 and it outputs warnings for some of the buffers used to hold numerical values (see example screenshot below).

These changes should not do any harm but use more of the stack memory while calling the changed methods, but it should be ok imho.

grafik

@SumolX
Copy link
Contributor

SumolX commented Nov 18, 2024

I would also update the related code to use snprintf to prevent buffer overflows. sprintf should not be used in any code base.

@Master92
Copy link
Contributor Author

I agree about using snprintf over sprintf. I'm just not sure if I (or you 😉) should better open another PR for that. I like being as specific about only one topic per PR as possible.

@ligenxxxx
Copy link
Member

I use sprintf in many places. I have indeed had an app bounce during development because the buf was not big enough.

@Master92 Master92 changed the title Resize buffers to allow maximum string size Prevent possible invalid memory accesses by sprintf Nov 19, 2024
@ligenxxxx ligenxxxx merged commit 4e963ad into hd-zero:main Nov 21, 2024
1 check passed
@Master92 Master92 deleted the compile_warnings branch November 21, 2024 05:18
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.

3 participants