fix(targa): Validate header sizes when loading Targa images by bobtista · Pull Request #3572 · TheSuperHackers/GeneralsGameCode

bobtista · 2026-10-22T17:17:32Z

Targa::Load trusts the sizes in the file header. The color map is read at CMapStart * depth for CMapLength * depth bytes into the caller's palette buffer, and the texture loaders pass a 1024 byte array. Width and Height are signed, and their product with the pixel depth can overflow. DecodeImage keeps writing when a packet count is larger than the pixels left.

Now SetPalette and SetImage take the buffer size, and Load rejects a color map or image that does not fit its buffer. Invalid dimensions and depths, and sizes that overflow, are rejected. DecodeImage stops at a packet that would pass the end of the image.

Todo:

Load valid 8, 16, 24 and 32 bit, RLE and color mapped TGAs in a MinGW-w64 i686 test harness: unchanged
Load malformed headers in the same harness: rejected after the fix
Build vc6 and win32

coderabbitai · 2026-10-22T17:17:52Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Last reviewed commit: 28c90c1
Latest commit, not reviewed: d473dba

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review.

Walkthrough

Targa now tracks image and palette buffer capacities. It validates header-derived sizes during allocation, loading, and decoding. Texture-loading paths pass palette buffer sizes to SetPalette.

Changes

TGA Buffer Validation

Layer / File(s)	Summary
Buffer sizing and capacity tracking `Core/Libraries/Source/WWVegas/WWLib/TARGA.h`, `Core/Libraries/Source/WWVegas/WWLib/TARGA.cpp`	`Targa` accepts and records image and palette buffer sizes. Size helpers validate header dimensions and depths. Automatic allocation uses validated sizes.
Load and decode bounds checks `Core/Libraries/Source/WWVegas/WWLib/TARGA.cpp`, `Core/Libraries/Source/WWVegas/WW3D2/textureloader.cpp`, `Core/Libraries/Source/WWVegas/WW3D2/texturethumbnail.cpp`	TGA loading checks buffer capacities. Image decoding rejects invalid sizes and RLE packets that exceed the remaining pixel count. Texture-loading paths pass palette buffer sizes to `SetPalette`.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: mauller

Merge Risk: Minimal · previous review 28c90

The reviewed capacity and decoding paths show no remaining issue that would prevent merging after normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title names the main change: Targa image loading now validates header sizes. The raw summary shows buffer sizes are passed to SetPalette and SetImage, Load-related paths reject sizes that do not f…
Description check		The description explains the problem and the fix. It covers the palette buffer overflow risk, signed dimension overflow, and the RLE packet overrun. It also states the fix: size arguments for SetPalet…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: Targa image loading now validates header sizes. The raw summary shows buffer sizes are passed to SetPalette and SetImage, Load-related paths reject sizes that do not fit, and RLE decoding is bounded. The title is specific and matches the changeset, and it follows the conventional commit style used in the objectives.

Full details: Description check

Explanation

The description explains the problem and the fix. It covers the palette buffer overflow risk, signed dimension overflow, and the RLE packet overrun. It also states the fix: size arguments for SetPalette and SetImage, rejection of invalid sizes, and a bounded DecodeImage. It lists the testing done and the unfinished VC6 and win32 build, so it relates directly to the changeset.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

ℹ️ Recent review info

⚙️ Run configuration

Source mode: Auto

Repository: Unavailable

Branch: Unavailable

File: No configuration file from this repository applied

Commit: Unavailable

Layers: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: `0c954de4-7bef-44a7-b424-3efe35858154`

📥 Commits

Reviewing files that changed from the base of the PR and between `1fa78d3` and `28c90c1`.

📒 Files selected for processing: 2

`Core/Libraries/Source/WWVegas/WWLib/TARGA.cpp`

`Core/Libraries/Source/WWVegas/WWLib/TARGA.h`

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

greptile-apps · 2026-10-22T17:21:23Z

Confidence Score: 5/5

[Medium impact] The reviewed changes appear safe to merge.

Summary

This PR checks Targa image and palette sizes before loading data and rejects compressed packets that exceed the remaining image.

Targa loading checks header sizes against assigned buffers.
RLE decoding stops at the image's last pixel.

_{Reviews (7) · Last reviewed commit: "refactor(generals): Pass the image buffe..." · Reviewed by Greptile}

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: d870cdb0-4f81-4b53-9e55-cda8fd9d2e70

📥 Commits

Reviewing files that changed from the base of the PR and between b831e27 and 4de3242.

📒 Files selected for processing: 4

Core/Libraries/Source/WWVegas/WW3D2/textureloader.cpp
Core/Libraries/Source/WWVegas/WW3D2/texturethumbnail.cpp
Core/Libraries/Source/WWVegas/WWLib/TARGA.cpp
Core/Libraries/Source/WWVegas/WWLib/TARGA.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

stephanmeesters · 2026-10-22T17:57:32Z

+static long TGA_PaletteSize(const TGAHeader& header)
+{
+	const long depth = (header.CMapDepth >> 3);
+	if ((header.CMapStart < 0) || (header.CMapLength <= 0) || (depth <= 0) || (depth > 4)) {
+		return 0;
+	}
+	return ((long)header.CMapStart + (long)header.CMapLength) * depth;
+}


Suggested:

static long TGA_PaletteSize(const TGAHeader& header) { if ((header.CMapStart < 0) || (header.CMapLength <= 0) || ((header.CMapDepth != 16) && (header.CMapDepth != 24) && (header.CMapDepth != 32))) { return 0; } const long depth = (header.CMapDepth >> 3); return ((long)header.CMapStart + (long)header.CMapLength) * depth; }

Check CMapDepth explicitly for 16, 24, or 32 before shifting. The current check accepts every value from 8 through 39—for example, 31 is treated like 24 bits. This would reject unsupported palette depths.

I would break the conditions up a bit into at least two if statements to improve readability.

Maybe test the size and depth in different if statements.

Then do the same for image size.

Done. PixelDepth is limited to 8, 16, 24 and 32 too, the depths Get_WW3D_Format supports.

stephanmeesters · 2026-10-22T17:58:08Z

Same fix can also be applied to Core/Tools/WW3D/max2w3d/TARGA.cpp for consistency

bobtista · 2026-10-23T17:05:35Z

Same fix can also be applied to Core/Tools/WW3D/max2w3d/TARGA.cpp for consistency

The max2w3d copy is only used to save. The exporter never loads a TGA through it, so no file header reaches that code.

xezon

THis looks suspicious. Maybe needs AI review passes?

xezon · 2026-10-23T18:59:34Z

 		void YFlip();

-		char* SetImage(char* buffer);
+		char* SetImage(char* buffer, long size = 0);


Is the zero default deliberate? If yes why?

Yes. 0 means the buffer capacity is unknown, and Load rejects it. The callers that leave it out only use the buffer to save an image, which doesn't need it.

The asymmetry to SetPalette, which does not take a 0 defaulted size argument, looks unfavourable. Can we make the interface consistent?

Done, SetImage takes a required size now like SetPalette. The callers pass their buffer size.

bobtista · 2026-10-23T21:08:05Z

THis looks suspicious. Maybe needs AI review passes?

Restructured it so Load only uses the member buffers. Tested 28 TGA files in a MinGW i686 harness. The 15 valid ones decode to the same pixels as main, including flips, RLE and nonzero CMapStart. All 13 malformed ones are rejected with no write outside their buffers. Undersized and unsized caller buffers are rejected too.

…e helpers

xezon

This change is difficult to understand.

xezon · 2026-11-01T10:59:34Z

+			count &= 0x7F;
+			count++;

+			if ((unsigned long)count > pixel_count)


I do not understand this code.

Both packet types store the pixel count minus one in the low 7 bits. Main decoded it separately in each branch, so I moved that above the branches and added one check that rejects a packet with more pixels than are left in the image. Without it a bad count writes past the end of mImage.

I still do not understand this code. It is unclear what "a run of pixels" means. Can you express the code and/or comments in a way that this becomes clear?

I assume that "run of pixels" refers to pixels of the same color one after another. But the way the code is expressed do not explain this.

Both packet types store the pixel count minus one in the low 7 bits. Main decoded it separately in each branch, so I moved that above the branches and added one check that rejects a packet with more pixels than are left in the image. Without it a bad count writes past the end of mImage.

bobtista · 2026-11-05T17:56:15Z

Simplified it along the inline comments. The size helpers no longer undo each other, the fail branches come last, and there are two TSH comments instead of four.

xezon · 2026-11-05T19:57:41Z

Update is not pushed.

bobtista · 2026-11-05T20:19:11Z

Pushed now, sorry

xezon

Most looks ok, but the "run of pixels" are still confusing.

xezon · 2026-11-07T11:00:00Z

 #include <utility>

+// TheSuperHackers @fix bobtista 22/10/2026 Compute the buffer sizes from the header, or 0 when the header is invalid.
+static long TGA_PaletteBufferSize(const TGAHeader& header)


Instead of making these static functions here, how about making them members of struct _TGAHeader?

Done, the helpers are TGAHeader members now.

xezon · 2026-11-07T11:01:39Z

+	if ((header.Width <= 0) || (header.Height <= 0)) {
+		return 0;
+	}
+	return (long)header.Width * (long)header.Height;


or return max(header.Width, 0) * max(header.Height, 0)

Done, it uses std::max now.

xezon · 2026-11-07T11:14:04Z

+			count &= 0x7F;
+			count++;

+			if ((unsigned long)count > pixel_count)


I still do not understand this code. It is unclear what "a run of pixels" means. Can you express the code and/or comments in a way that this becomes clear?

I assume that "run of pixels" refers to pixels of the same color one after another. But the way the code is expressed do not explain this.

…the SetImage buffer size

bobtista · 2026-11-07T18:02:46Z

Rewrote the comments in DecodeImage to describe the packet format sorry that was confusing

coderabbitai Bot reviewed Oct 22, 2026

View reviewed changes

Comment thread Core/Libraries/Source/WWVegas/WWLib/TARGA.cpp Resolved

stephanmeesters reviewed Oct 22, 2026

View reviewed changes

xezon reviewed Oct 23, 2026

View reviewed changes

bobtista added 3 commits October 28, 2026 14:04

fix(targa): Validate header sizes when loading Targa images

b3aef3b

fix(targa): Reject unsupported color map and pixel depths

6e0af8a

refactor(targa): Load into the member buffers and name the buffer siz…

81167bf

…e helpers

bobtista force-pushed the bobtista/fix/targa-bounds branch from 28c90c1 to 81167bf Compare October 28, 2026 18:04

xezon reviewed Nov 1, 2026

View reviewed changes

refactor(targa): Simplify the size helpers and the RLE packet decoding

af5d4f4

xezon reviewed Nov 7, 2026

View reviewed changes

bobtista added 3 commits November 7, 2026 12:27

refactor(targa): Describe the RLE packet format in DecodeImage

2aae893

refactor(targa): Make the size helpers TGAHeader members and require …

f7bd1c8

…the SetImage buffer size

refactor(generals): Pass the image buffer size to Targa::SetImage

d473dba

Conversation

bobtista commented Oct 22, 2026

Uh oh!

coderabbitai Bot commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

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

Uh oh!

Confidence Score: 5/5

Summary

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mauller Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

stephanmeesters commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobtista commented Oct 23, 2026

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!

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

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!

bobtista commented Oct 23, 2026

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobtista commented Nov 5, 2026

Uh oh!

xezon commented Nov 5, 2026

Uh oh!

bobtista commented Nov 5, 2026

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Oct 22, 2026 •

edited

Loading

greptile-apps Bot commented Oct 22, 2026 •

edited

Loading

Mauller Oct 23, 2026 •

edited

Loading

stephanmeesters commented Oct 22, 2026 •

edited

Loading

xezon Nov 7, 2026 •

edited

Loading