fix: Replace strcpy with strlcpy for robustness #1712

Skyaero42 · 2025-10-18T19:44:39Z

Part of: Replace strcpy, strncpy & co with strlcpy family functions #1518
Merge after refactor: Simplify path string formatting in World Builder #1759
Merge after fix: Replace strcpy with strlcpy and update length asserts #1796
Merge after fix(script): Improve string handling of 'Fundamentalist' code hacks #1797

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.

Core/Libraries/Source/debug/debug_io_flat.cpp

xezon

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/WW3D2/hmdldef.cpp

Core/Libraries/Source/WWVegas/WW3D2/ringobj.cpp

Core/Libraries/Source/WWVegas/WW3D2/sphereobj.cpp

xezon · 2025-10-24T17:30:58Z

Core/Libraries/Source/WWVegas/WW3D2/w3d_dep.cpp

 		return buffer;
 	}
-	strcpy(buffer, w3d_name);
+	strlcpy(buffer, w3d_name, ARRAY_SIZE(buffer));


Same Assert as in sphereobj.cpp

Assert was not changed

Core/Libraries/Source/WWVegas/WWLib/registry.cpp

GeneralsMD/Code/Tools/WorldBuilder/src/OpenMap.cpp

GeneralsMD/Code/Tools/WorldBuilder/src/SaveMap.cpp

GeneralsMD/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp

Skyaero42 · 2025-11-01T22:02:35Z

Changes addressing review comments added in a separate commit

GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/Water/W3DWaterTracks.cpp

GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp

xezon

This change now contains a bunch of edits that do not relate to strcpy replacements, but simplifications to get rid of unnecessary strcpy and related junk. How about we move them to a separate Pull Request?

xezon · 2025-11-02T16:50:53Z

Core/Libraries/Source/WWVegas/WW3D2/ringobj.cpp

 	WWASSERT(name != NULL);
 	WWASSERT(strlen(name) < 2*W3D_NAME_LEN);
-	strcpy(Name,name);
+	strlcpy(Name, name, ARRAY_SIZE(Name));


Use assert like in SphereRenderObjClass::Set_Name

xezon · 2025-11-02T16:52:24Z

Core/Libraries/Source/WWVegas/WW3D2/w3d_dep.cpp


 	// Copy all characters from start to end (excluding 'end')
 	// into the w3d_name buffer. Then capitalize the string.
 	int num_chars = end - start;


size_t to get rid of compiler warning

xezon · 2025-11-02T16:53:24Z

Core/Libraries/Source/WWVegas/WW3D2/w3d_dep.cpp

 		return buffer;
 	}
-	strcpy(buffer, w3d_name);
+	strlcpy(buffer, w3d_name, ARRAY_SIZE(buffer));


Assert was not changed

xezon · 2025-11-02T17:00:27Z

GeneralsMD/Code/GameEngine/Source/Common/PerfTimer.cpp

-	strcpy(s_buf, fname);
+	strlcpy(s_buf, fname, ARRAY_SIZE(s_buf));

 	char tmp[256];


Definitely _MAX_PATH

GeneralsMD/Code/GameEngine/Source/Common/System/SaveGame/GameState.cpp

xezon · 2025-11-02T17:54:11Z

GeneralsMD/Code/Tools/GUIEdit/Source/GUIEdit.cpp


 	// copy over the full path and filename
-	strcpy( m_savePathAndFilename, fullPathAndFilename );
+	strlcpy(m_savePathAndFilename, fullPathAndFilename, ARRAY_SIZE(m_saveFilename));


Wrong field for size.

xezon · 2025-11-02T17:58:29Z

GeneralsMD/Code/Tools/WorldBuilder/src/BuildList.cpp

-		strlcat(curbuf, ".ini", ARRAY_SIZE(curbuf));
+		strlcat(buffer, name.str(), ARRAY_SIZE(buffer));
+		strlcat(buffer, "_BuildList", ARRAY_SIZE(buffer));
+		strlcat(buffer, ".ini", ARRAY_SIZE(buffer));


Can merge this cat with the one above.

xezon · 2025-11-02T18:00:37Z

GeneralsMD/Code/Tools/WorldBuilder/src/BuildList.cpp

+		char buffer[ _MAX_PATH ];
+		::GetModuleFileName( NULL, buffer, sizeof( buffer ) );
+		char *pEnd = buffer + strlen( buffer );
+		while( pEnd != buffer )


This thing here looks like a poor mans strrchr and can be simplied. There are 3 of these per game in code base. Can do in separate change because is not directly related to strcpy.

I created task with #1781

xezon · 2025-11-02T18:14:54Z

GeneralsMD/Code/Tools/WorldBuilder/src/MeshMoldOptions.cpp

-
 		FilenameList filenameList;
-		TheFileSystem->getFileListInDirectory(AsciiString(dirBuf), AsciiString("*.w3d"), filenameList, FALSE);
+		TheFileSystem->getFileListInDirectory(AsciiString(".\\data\\Editor\\Molds\\"), AsciiString("*.w3d"), filenameList, FALSE);


Remove AsciiString. You already did in another file too.

xezon · 2025-11-02T18:16:52Z

GeneralsMD/Code/Tools/WorldBuilder/src/ObjectOptions.cpp

-				strlcat(fileBuf, "/", ARRAY_SIZE(findBuf));
-				strlcat(fileBuf, token.str(), ARRAY_SIZE(findBuf));
+				strlcpy(fileBuf, TEST_STRING, ARRAY_SIZE(fileBuf));
+				strlcat(fileBuf, "/", ARRAY_SIZE(fileBuf));


Can merge this cat with the one above. TEST_STRING "/" is valid.

xezon · 2025-11-06T17:48:42Z

Obsoleted pull

Skyaero42 added this to the Stability fixes milestone Oct 18, 2025

Skyaero42 self-assigned this Oct 18, 2025

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 force-pushed the skyaero/strcpy-to-strlcpy branch from 498a8e9 to db9bc33 Compare October 18, 2025 20:05

Skyaero42 commented Oct 19, 2025

View reviewed changes

Core/Libraries/Source/debug/debug_io_flat.cpp Outdated Show resolved Hide resolved

Skyaero42 force-pushed the skyaero/strcpy-to-strlcpy branch from db9bc33 to a11e0e0 Compare October 19, 2025 19:27

Skyaero42 marked this pull request as ready for review October 19, 2025 19:27

Skyaero42 changed the title ~~fix: Replace strcat with strlcat for robustness~~ fix: Replace strcpy with strlcpy for robustness Oct 21, 2025

Skyaero42 mentioned this pull request Oct 21, 2025

Replace strcpy, strncpy & co with strlcpy family functions #1518

Open

11 tasks

xezon mentioned this pull request Oct 25, 2025

unify(network): Move GameNetwork and GameSpy files to Core #1733

Open

This comment was marked as off-topic.

Sign in to view

xezon reviewed Oct 27, 2025

View reviewed changes

This was referenced Oct 29, 2025

refactor: Simplify path string formatting in World Builder #1759

Merged

refactor(maputil): Remove unused variables in MapUtil #1767

Merged

Skyaero42 force-pushed the skyaero/strcpy-to-strlcpy branch 2 times, most recently from cd6da64 to 994e0df Compare November 1, 2025 21:04

fix: Replace strcpy with strlcpy

3dfe8bc

Skyaero42 force-pushed the skyaero/strcpy-to-strlcpy branch 5 times, most recently from 38188de to 40dee38 Compare November 1, 2025 22:01

Resolving code review feedback

fd90bbe

Skyaero42 force-pushed the skyaero/strcpy-to-strlcpy branch from 40dee38 to fd90bbe Compare November 1, 2025 22:07

Skyaero42 requested a review from xezon November 1, 2025 22:13

xezon reviewed Nov 2, 2025

View reviewed changes

GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/Water/W3DWaterTracks.cpp Show resolved Hide resolved

GeneralsMD/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp Outdated Show resolved Hide resolved

Addressing additional review comments

9e78404

xezon reviewed Nov 2, 2025

View reviewed changes

xezon closed this Nov 6, 2025

fix: Replace strcpy with strlcpy for robustness #1712

fix: Replace strcpy with strlcpy for robustness #1712

Uh oh!

Conversation

Skyaero42 commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Skyaero42 commented Nov 1, 2025

Uh oh!

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xezon commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Skyaero42 commented Oct 18, 2025 •

edited

Loading