fix(gamefont): Size glyph buffers to the glyph to prevent an overflow by tintinhamans · Pull Request #3268 · TheSuperHackers/GeneralsGameCode

tintinhamans · 2026-09-07T21:32:47Z

The glyph buffer was a fixed uint16[32768]. Store_GDI_Char writes
width * height pixels with no bound check, so a big glyph writes past the end.

A pointSize > 100 cap used to hide this, but it was removed in #3051 so 4K UI scaling
can use bigger fonts. A scaled font can now hit the overflow.

FontCharsBuffer allocates its pixels and grows to fit the glyph.
getFont clamps requests to 512 instead of rejecting them, so an oversized
request still returns a usable font rather than nullptr (callers like
W3DDisplayString::setFont ignore null and would show no text).
adjustFontSize clamps to the same max so the scaled size stays in range.

A font size of about 460 is the most any real screen needs (a 48pt heading blown up 9.6x on an 8K display) so I think 512 is a safe value for now.

chatgpt-codex-connector · 2026-09-07T21:33:01Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
🔒 Security Review	✅ Completed 2026-09-07T21:42:55.605071Z	`dbebfc4`	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

qodo-free-for-open-source-projects · 2026-09-07T21:33:43Z

PR Summary by Qodo

Prevent large glyphs from overflowing font buffers

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Prevents large GDI glyphs from overflowing fixed pixel buffers.
• Allocates each backing buffer to fit its triggering glyph.
• Rejects font requests outside the supported 1–512 point range.

Diagram

graph TD
    A["Font request"] --> B{"Size valid?"} -->|No| C["Reject request"]
    B -->|Yes| D["GDI glyph"] --> E{"Buffer fits?"} -->|Yes| F["Store pixels"]
    E -->|No| G["Allocate buffer"] --> F

High-Level Assessment

The proposed defense-in-depth approach is appropriate: validate externally supplied font sizes and independently size internal storage from measured glyph dimensions. Retaining pooled default buffers for ordinary glyphs avoids unnecessary allocations, while oversized glyphs receive dedicated capacity using the engine's existing allocation conventions.

Files changed (3) +28 / -7

Bug fix (3) +28 / -7

GameFont.cppCap accepted font sizes at 512 points +2/-2

Cap accepted font sizes at 512 points

• Extends font request validation to reject sizes above 512 points while continuing to reject zero and negative sizes. This limits content-driven memory usage without restoring the previous 100-point restriction that blocked high-DPI scaling.

Core/GameEngine/Source/GameClient/GUI/GameFont.cpp

render2dsentence.cppAllocate glyph buffers according to required capacity +19/-3

Allocate glyph buffers according to required capacity

• Adds allocation and cleanup for dynamically sized pixel arrays. Buffer selection now checks each buffer's actual capacity and allocates at least the glyph's width-by-height pixel count, preventing large glyph writes from exceeding the former fixed allocation.

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

render2dsentence.hTrack dynamic font buffer storage and length +7/-2

Track dynamic font buffer storage and length
• Replaces the fixed 32,768-element pixel array with an owned pointer and explicit capacity. Declares lifecycle methods and retains the original constant as the default allocation size for ordinary glyphs.
Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.h

qodo-free-for-open-source-projects · 2026-09-07T21:35:19Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Oversized interface text disappears 🐞 Bug ≡ Correctness

Description

FontLibrary::getFont returns nullptr rather than clamping requests above 512, while
display-string consumers silently ignore a null font. When resolution scaling pushes configured
credits, tooltip, or header fonts over the threshold, new strings retain no font and compute zero
extents, while existing strings can retain a stale font.

Code

Core/GameEngine/Source/GameClient/GUI/GameFont.cpp[R182-184]
+	if (pointSize < 1 || pointSize > 512)
	{
		return nullptr;

Evidence
Resolution scaling can produce a size above 512, and credits pass that value directly to getFont.
A null result is ignored by W3DDisplayString::setFont; after text changes, computeExtents
explicitly assigns zero dimensions when no font is installed, demonstrating the missing-text
outcome.
Core/GameEngine/Source/GameClient/GlobalLanguage.cpp[278-286]
Core/GameEngine/Source/GameClient/Credits.cpp[243-247]
Core/GameEngine/Source/GameClient/GUI/HeaderTemplate.cpp[217-227]
Core/GameEngine/Source/GameClient/Input/Mouse.cpp[621-635]
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp[292-314]
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp[356-375]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Font requests above 512 are rejected with `nullptr`, but callers generally expect a usable font and may consequently display no text. Preserve the allocation limit by clamping positive oversized requests to 512 while continuing to reject invalid non-positive sizes.

## Issue Context
DPI-adjusted font sizes flow directly into `getFont`, and display-string font assignment ignores null values. Clamping enforces the intended maximum glyph allocation without requiring every font consumer to implement fallback behavior.

## Fix Focus Areas
- Core/GameEngine/Source/GameClient/GUI/GameFont.cpp[179-185]
- Core/GameEngine/Source/GameClient/GlobalLanguage.cpp[278-286]
- Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DDisplayString.cpp[292-314]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Context sources

Review mode: ⚖️ Balanced: This is a security-relevant memory-safety fix spanning allocation, buffer lifecycle, glyph sizing, and font-size validation, so it warrants a complete single-pass review.

Tip of the day

💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

greptile-apps · 2026-09-07T21:35:25Z

Greptile Summary

The PR prevents large glyphs from overflowing the former fixed-size pixel buffer and bounds font scaling to a supported maximum.

Replaces fixed glyph storage with dynamically allocated buffers sized to fit each glyph.
Stores lightweight buffer descriptors by value while retaining explicit ownership in FontCharsClass.
Clamps direct and resolution-adjusted font sizes to 512 points.

Confidence Score: 5/5

The PR appears safe to merge, with no outstanding actionable defects identified in the changes since the previous review.

The revised value-based buffer descriptors preserve stable pixel allocations through vector movement, and each backing array remains explicitly released once by its owning FontCharsClass.

Important Files Changed

Filename	Overview
Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.cpp	Allocates glyph buffers according to required capacity and explicitly releases their backing arrays.
Core/Libraries/Source/WWVegas/WW3D2/render2dsentence.h	Converts glyph buffers into value-stored descriptors containing allocation length and pixel pointer.
Core/GameEngine/Source/GameClient/GUI/GameFont.cpp	Clamps oversized font requests while preserving rejection of invalid non-positive sizes.
Core/GameEngine/Source/GameClient/GlobalLanguage.cpp	Applies the same maximum after resolution-based font scaling.
Core/GameEngine/Include/GameClient/GameFont.h	Defines the shared 512-point maximum used by font creation and scaling.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Request[Requested font size] --> Clamp[Clamp to 512 points]
  Clamp --> Metrics[Measure glyph dimensions]
  Metrics --> Required[Calculate required pixel count]
  Required --> Check{Current buffer has room?}
  Check -->|Yes| Write[Write glyph pixels]
  Check -->|No| Allocate[Allocate max of default size and glyph size]
  Allocate --> Write
  Write --> Cache[Store stable pointer to glyph pixels]

_{Reviews (5): Last reviewed commit: "refactor(gamefont): Store glyph buffer d..." | Re-trigger Greptile}

stephanmeesters · 2026-09-07T21:51:32Z

Can't we use std::vector<uint16> Buffer?

At what font size did it overflow the original buffer? Was this leading to crashes?

tintinhamans · 2026-09-07T22:01:29Z

At what font size did it overflow the original buffer? Was this leading to crashes?

Theoretically somewhere around 130 to 150 point, can also be triggered by something like a custom map with DISPLAY_CINEMATIC_TEXT so might have some security implications.

Can't we use std::vector Buffer?

Each glyph caches a raw pointer into its slab (char_data->Buffer = slab->Buffer + CurrPixelOffset), idk how I'd handle that in a vector.

xezon

Needs fixing the slop.

xezon · 2026-09-08T08:12:09Z

-		FontCharsBuffer* new_buffer = W3DNEW FontCharsBuffer;
+		// TheSuperHackers @fix arcticdolphin 07/09/2026 Grow the buffer to the glyph so a big one cannot overrun it.
+		const int length = (char_len > CHAR_BUFFER_LEN) ? char_len : CHAR_BUFFER_LEN;
+		FontCharsBuffer* new_buffer = W3DNEW FontCharsBuffer( length );


This buffer now has 2 levels of indirection for every use case. Can we make this more optimal? Maybe BufferList should now carry FontCharsBuffer as value.

Dropped the useless W3DMPO_CODE pooling and left BufferList as a pointer vector. Making FontCharsBuffer a value type is riskier than it looks DynamicVectorClass copies elements around internally when it grows.

Making FontCharsBuffer a value type is riskier than it looks DynamicVectorClass copies elements around internally when it grows.

What is risky about it?

Was worried about the destructor freeing buffers when the vector grows. Moved cleanup to FontCharsClass, so storing values works now.

OmarAglan · 2026-09-08T13:13:45Z

Do this effect PR #3231 ?

xezon · 2026-09-09T08:51:49Z

-	W3DMPO_CODE(FontCharsBuffer)
 public:
-	uint16			Buffer[CHAR_BUFFER_LEN];
+	FontCharsBuffer() : Length( 0 ), Buffer( 0 ) {}


Buffer( nullptr )

xezon · 2026-09-09T08:52:21Z

 	//
 	StringClass							Name;
-	DynamicVectorClass<FontCharsBuffer*>	BufferList;
+	// TheSuperHackers @refactor arcticdolphin 08/09/2026 FontCharsClass owns the pixel arrays; descriptors are non-owning values.


Remove comment

xezon · 2026-09-09T09:27:55Z

+		}
+		FontCharsBuffer new_buffer;
+		new_buffer.Length = length;
+		new_buffer.Buffer = W3DNEWARRAY uint16[length];


Maybe make this a constructor taking both arguments so that both are guaranteed set. Right now a caller could omit length.

xezon · 2026-09-09T09:28:19Z


 enum { CHAR_BUFFER_LEN		= 32768 };

+// TheSuperHackers @fix arcticdolphin 07/09/2026 Buffer length matches the glyph so a large glyph cannot overrun it.


Redundant comment

xezon · 2026-09-09T09:32:15Z

+	Int pointSize = REAL_TO_INT_FLOOR(theFontSize * adjustFactor);
+
+	// TheSuperHackers @fix arcticdolphin 07/09/2026 Keep the scaled size within what getFont can build.
+	if (pointSize > FONT_POINT_SIZE_MAX)


Is this clamp here necessary, considering font already clamps it anyway?

I assume it is needed so that callers work with the correct font sizes when they need it in calculations?

But I think it would be cleaner if callers then take the real font size from the GameFont class after it was created.

xezon · 2026-09-09T09:34:16Z

 	{
-		FontCharsBuffer* new_buffer = W3DNEW FontCharsBuffer;
+		// TheSuperHackers @fix arcticdolphin 07/09/2026 Length may exceed CHAR_BUFFER_LEN to fit this glyph.
+		int length = CHAR_BUFFER_LEN;


max(CHAR_BUFFER_LEN, char_len)

qodo-free-for-open-source-projects Bot reviewed Sep 7, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameClient/GUI/GameFont.cpp Outdated

tintinhamans force-pushed the arctic/fix/font-glyph-buffer-overflow branch from dbebfc4 to 33b6309 Compare September 7, 2026 21:49

xezon reviewed Sep 8, 2026

View reviewed changes

xezon added Minor Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Fix Is fixing something, but is not user facing labels Sep 8, 2026

tintinhamans force-pushed the arctic/fix/font-glyph-buffer-overflow branch from 33b6309 to eae556c Compare September 8, 2026 14:56

bugfix(gamefont): Size glyph buffers to the glyph to prevent an overflow

cb3ed94

tintinhamans force-pushed the arctic/fix/font-glyph-buffer-overflow branch from eae556c to cb3ed94 Compare September 8, 2026 14:58

refactor(gamefont): Store glyph buffer descriptors by value

1ce2f06

tintinhamans requested a review from xezon September 9, 2026 00:56

xezon reviewed Sep 9, 2026

View reviewed changes


		enum { CHAR_BUFFER_LEN = 32768 };

		// TheSuperHackers @fix arcticdolphin 07/09/2026 Buffer length matches the glyph so a large glyph cannot overrun it.

Uh oh!

Conversation

tintinhamans commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 7, 2026

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 7, 2026

Code Review by Qodo

Uh oh!

Uh oh!

greptile-apps Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

stephanmeesters commented Sep 7, 2026

Uh oh!

tintinhamans commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

OmarAglan commented Sep 8, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tintinhamans commented Sep 7, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 7, 2026 •

edited

Loading

greptile-apps Bot commented Sep 7, 2026 •

edited

Loading

tintinhamans commented Sep 7, 2026 •

edited

Loading