Skip to content

Conversation

@Skyaero42
Copy link

To keep comments and old code visible for the split offs, I've chosen to open a new PR for the last part of the strcpy saga.

@Skyaero42 Skyaero42 self-assigned this Nov 5, 2025
@Skyaero42 Skyaero42 added Refactor Edits the code with insignificant behavior changes, is never user facing Stability Concerns stability of the runtime labels Nov 5, 2025
@xezon xezon changed the title refactor: Replace strcpy with strlcpy to prevent buffer overflows fix: Replace strcpy with strlcpy to prevent potential buffer overflows Nov 6, 2025
@xezon xezon added this to the Stability fixes milestone Nov 6, 2025
@xezon xezon added Fix Is fixing something, but is not user facing and removed Refactor Edits the code with insignificant behavior changes, is never user facing labels Nov 6, 2025
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.

Only looked over Core + Generals Code.

I think strcpy from buffer[] to buffer[] need another review pass and find all places where strcpy + static_assert is good enough.

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.

Something is wrong with the following line. Perhaps this was meant to be strcpy. Buffer has same length as TGA2_SIGNATURE.

strlcpy(footer.Signature, TGA2_SIGNATURE, sizeof(footer.Signature));

There are more copies that can use static_assert's instead.

strlcpy(Name,w3d_data.Name,sizeof(Name));
strlcpy(header.Name,AnimName,sizeof(header.Name));
strlcpy(header.HierarchyName,HierarchyName,sizeof(header.HierarchyName));
strlcpy(Name, aheader.HierarchyName, ARRAY_SIZE(Name));
strlcat(Name, ".", ARRAY_SIZE(Name));
strlcat(Name, aheader.Name, ARRAY_SIZE(Name));
strlcpy(HierarchyName,aheader.HierarchyName,W3D_NAME_LEN);

WWVegas code needs another review pass to use static_assert + strcpy instead of strlcpy where so possible.

@Skyaero42
Copy link
Author

Skyaero42 commented Nov 8, 2025

strlcpy(footer.Signature, TGA2_SIGNATURE, sizeof(footer.Signature));

These were all strncpy and were addressed in a different commit. I can re-address them, but I think it is better to do in a separate PR.

I checked all the strcpy/strlcpy in this commit and those should all have been addressed with static_assert if possible.

@xezon
Copy link

xezon commented Nov 8, 2025

The TGA2_SIGNATURE thing definitely needs looking at. Can do in separate change.

RoadOptions conflict needs resolving.

Bart Roossien and others added 4 commits November 8, 2025 22:58
@Skyaero42 Skyaero42 force-pushed the skyaero/strcpy-strlcpy branch from 2c26ba8 to e8f6976 Compare November 8, 2025 21:59
@Skyaero42
Copy link
Author

Rebased

Created task #1828 to follow up on TGA2_SIGNATURE

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.

In WWCommon.h, put the following

#ifndef ARRAY_SIZE
#if defined(_MSC_VER) && _MSC_VER < 1300
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
#else
template <typename Type, size_t Size> char (*ArraySizeHelper(Type (&)[Size]))[Size];
#define ARRAY_SIZE(arr) sizeof(*ArraySizeHelper(arr))
#endif
#endif // ARRAY_SIZE

It then gives compiler error when ARRAY_SIZE is used on non array type.

@Skyaero42
Copy link
Author

Comments processed.

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.

Looks good as far as I can tell.

@xezon xezon added Major Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels Nov 9, 2025
@xezon xezon merged commit 038f51f into TheSuperHackers:main Nov 9, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fix Is fixing something, but is not user facing Major 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

Development

Successfully merging this pull request may close these issues.

2 participants