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

convert strncpy to strncpy_s #4706

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

Conversation

Goober5000
Copy link
Contributor

I have gone through the codebase and replaced most instances of strncpy with strncpy_s which always adds a null-terminator and includes some extra error checking. Quite a few of these places did not use the length properly and have been fixed.

This is based on #4321 but without the safe_strings removal.

@Goober5000 Goober5000 added the cleanup A modification or rewrite of code to make it more understandable or easier to maintain. label Sep 24, 2022
I have gone through the codebase and replaced most instances of `strncpy` with `strncpy_s` which always adds a null-terminator and includes some extra error checking.  Quite a few of these places did not use the length properly and have been fixed.

This is based on scp-fs2open#4321 but without the safe_strings removal.
@Goober5000 Goober5000 force-pushed the strncpy_to_strncpy_s_2 branch from 3f4ee81 to 219175b Compare September 24, 2022 02:53
@notimaginative
Copy link
Contributor

notimaginative commented Sep 26, 2022

I'd like to point out that strncpy_s, on an error such as truncation, will produce an empty dest string. This can give unpredictable, and possibly dangerous, results if such errors aren't accounted for. For something like cfile it could be dangerous to have path construction suddenly reset in the middle of the process and end up trying to act on a very unexpected path string.

strncpy_s is safer for sure, but it's not really a drop-in replacement for strncpy. Some of these places will require the addition of extra error handling or conversion to SCP_string instead.

Also to add, I am in the process of slowing converting much of the PXO related code to use SCP_string wherever possible and SDL_strlcpy() where not since it's safer than a straight strncpy but will still produce a valid, even if truncated, string. Not to tell you to avoid changes to that code, just pointing out how I'm trying to handle a similar conversion to safer strings.

@JohnAFernandez JohnAFernandez added the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Dec 19, 2022
@BMagnu BMagnu removed the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Feb 14, 2023
@JohnAFernandez JohnAFernandez added the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Jan 24, 2024
@Goober5000 Goober5000 removed the Waiting for Stable Marks a pull request that is to be merged after the next stable release, due to a release cycle label Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup A modification or rewrite of code to make it more understandable or easier to maintain.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants