Skip to content

fix: Replace strcpy with strlcpy for robustness #1712

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: Replace strcpy with strlcpy for robustness #1712

wants to merge 2 commits into from

Conversation

@Skyaero42
Copy link

@Skyaero42 Skyaero42 commented Oct 18, 2025 •
edited
Loading

Replacing strcpy for strlcpy where applicable

  • No replacement when strcpy contains a magic string (verified that magic string fits into target parameter)
  • As with strlcat, ASCIIString and UnicodeString have not been touched.
  • No replacement when target parameter is a pointer instead of a char[].

TODO:

  • Replicate in Generals.

@Skyaero42 Skyaero42 added this to the Stability fixes milestone Oct 18, 2025
@Skyaero42 Skyaero42 self-assigned this Oct 18, 2025
@Skyaero42 Skyaero42 added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing Stability Concerns stability of the runtime labels Oct 18, 2025
@Skyaero42 Skyaero42 force-pushed the skyaero/strcpy-to-strlcpy branch from 498a8e9 to db9bc33 Compare October 18, 2025 20:05
Skyaero42
Skyaero42 commented Oct 19, 2025
View reviewed changes
@Skyaero42 Skyaero42 force-pushed the skyaero/strcpy-to-strlcpy branch from db9bc33 to a11e0e0 Compare October 19, 2025 19:27
@Skyaero42 Skyaero42 marked this pull request as ready for review October 19, 2025 19:27
@Skyaero42 Skyaero42 changed the title fix: Replace strcat with strlcat for robustness fix: Replace strcpy with strlcpy for robustness Oct 21, 2025
@Skyaero42 Skyaero42 mentioned this pull request Oct 21, 2025
Open
11 tasks
@xezon xezon mentioned this pull request Oct 25, 2025
Open
@CollabCheck-StockholmUni

This comment was marked as off-topic.

Sign in to view
xezon
xezon reviewed Oct 27, 2025
View reviewed changes
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

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

The Munkees really loved their string buffers.

Before continuing this change, I think we need another change that gets rid of all the redundant and obsolete buffer copies that I have raised in this review. I don't know if I caught them all, so more reviewing is required.

Core/Libraries/Source/WWVegas/WWLib/registry.cpp
*/
case REG_BINARY:
strcpy(save_name, "BIN_");
strlcpy(save_name, "BIN_", ARRAY_SIZE(save_name));
Copy link

Choose a reason for hiding this comment

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

strcpy is safe here

GeneralsMD/Code/Tools/WorldBuilder/src/OpenMap.cpp Outdated
if (pList == NULL) return;
pList->ResetContent();
strcpy(findBuf, dirBuf);
strlcpy(findBuf, dirBuf, ARRAY_SIZE(findBuf));
Copy link

Choose a reason for hiding this comment

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

strcpy is safe

GeneralsMD/Code/Tools/WorldBuilder/src/SaveMap.cpp Outdated
if (pList == NULL) return;
pList->ResetContent();
strcpy(findBuf, dirBuf);
strlcpy(findBuf, dirBuf, ARRAY_SIZE(fileBuf));
Copy link

Choose a reason for hiding this comment

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

strcpy is safe

GeneralsMD/Code/Tools/WorldBuilder/src/SaveMap.cpp Outdated
continue;
}
strcpy(fileBuf, dirBuf);
strlcpy(fileBuf, dirBuf, ARRAY_SIZE(fileBuf));
Copy link

Choose a reason for hiding this comment

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

strcpy is safe

GeneralsMD/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp Outdated
dirBuf[len] = 0;
}
strcpy(findBuf, dirBuf);
strlcpy(findBuf, dirBuf, ARRAY_SIZE(findBuf));
Copy link

Choose a reason for hiding this comment

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

strcpy is safe

GeneralsMD/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp Outdated
AsciiString filename = *it;
//strcpy(fileBuf, dirBuf);
strcpy(fileBuf, filename.str());
//strlcpy(fileBuf, dirBuf, ARRAY_SIZE(fileBuf));
Copy link

Choose a reason for hiding this comment

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

Can just remove this commented code.

This was referenced Oct 29, 2025
Merged
Open
@Skyaero42 Skyaero42 force-pushed the skyaero/strcpy-to-strlcpy branch from a11e0e0 to cd6da64 Compare October 31, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xezon xezon xezon left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Skyaero42 Skyaero42

Labels

Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing Stability Concerns stability of the runtime

Projects

None yet

Milestone

Stability fixes

Development

Successfully merging this pull request may close these issues.

3 participants

@Skyaero42 @CollabCheck-StockholmUni @xezon