Skip to content

[GEN][ZH] Fix several potential buffer overruns in game code #849

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

Merged
merged 12 commits into from
May 15, 2025

Conversation

slurmlord
Copy link

This PR addresses some buffer size issues that had the potential of writing beyond the bounds.

  • The wide char methods FormatMessageW, GetDateFormatW and GetDateFormatW take the buffer size in (wide) characters, not bytes.
  • The copying of colors in dynamesh.h seem like a simple bug, as the correct behavior is just below in the same method.
  • In the case malloc was called in FastAllocator, assert that it wasn't 0 before dereferencing it. Not a buffer overflow but still better with an assertion than an access violation, IMO.
  • For ftp, the buffer is being used in Cftp::GetNextFileBlock for a strncpy call with a hard-coded limit of 256.
  • The change in rinfo is to compare with 1 below the array size, as OverrideFlagLevel is immediately incremented before indexing into the array.

@xezon xezon added this to the Stability fixes milestone May 13, 2025
@xezon xezon added Major Severity: Minor < Major < Critical < Blocker Fix Is fixing something Stability Concerns stability of the runtime labels May 13, 2025
@xezon xezon changed the title [GEN][ZH] Fix some buffer size calculations [GEN][ZH] Fix several potential buffer overruns in game code May 13, 2025
@@ -235,7 +235,7 @@ void W3DMouse::initD3DAssets(void)
for (Int j=0; j < MAX_2D_CURSOR_ANIM_FRAMES; j++)
{
cursorTextures[i][j]=NULL;//am->Get_Texture(m_cursorInfo[i].textureName.str());
m_currentD3DSurface[i]=NULL;
m_currentD3DSurface[j]=NULL;
Copy link

Choose a reason for hiding this comment

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

The double loop is not the correct location to write to this member.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, it was a compromise in order to keep the diff as minimal as possible. Went with a separate loop for clearing m_current_D3DSurface instead.

@xezon
Copy link

xezon commented May 13, 2025

Does any of these changes fix an observable bug in the game?

@slurmlord
Copy link
Author

Does any of these changes fix an observable bug in the game?

Not that I know of, but I'm unfortunately not familiar enough with the set of known bugs either to tell. The issues were identified with the MSVC static code analysis and some manual check to see if it seemed plausible or not, so it's approaching from a bit of a different angle than a known repro.

@xezon xezon merged commit ce756df into TheSuperHackers:main May 15, 2025
18 checks passed
@xezon xezon added Gen Relates to Generals ZH Relates to Zero Hour labels May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Is fixing something Gen Relates to Generals Major Severity: Minor < Major < Critical < Blocker Stability Concerns stability of the runtime ZH Relates to Zero Hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants