-
Notifications
You must be signed in to change notification settings - Fork 118
fix: Replace strcpy with strlcpy to prevent potential buffer overflows #1808
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
Conversation
xezon
left a comment
There was a problem hiding this 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.
Core/GameEngineDevice/Source/Win32Device/Common/Win32LocalFileSystem.cpp
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/Shadow/W3DProjectedShadow.cpp
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DBridgeBuffer.cpp
Show resolved
Hide resolved
There was a problem hiding this 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.
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DBridgeBuffer.cpp
Show resolved
Hide resolved
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. |
|
The TGA2_SIGNATURE thing definitely needs looking at. Can do in separate change. RoadOptions conflict needs resolving. |
Resolving code review feedback Addressing additional review comments More review stuff
2c26ba8 to
e8f6976
Compare
|
Rebased Created task #1828 to follow up on TGA2_SIGNATURE |
xezon
left a comment
There was a problem hiding this 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_SIZEIt then gives compiler error when ARRAY_SIZE is used on non array type.
|
Comments processed. |
xezon
left a comment
There was a problem hiding this 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.
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.