Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughWorldBuilder changes flip-state sizing and blend-tile serialization, adjusts image and blend handling, removes the raw tile data API, and updates boundary editing and lookup. ChangesHeight-Map Data Handling
Boundary Editing and Lookup
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: 🟡 Moderate · up to Terrain blending can leave an invalid index when the blend table fills, risking unsafe memory access during later editing. Guard the failed remap before merging; the API-removal concern is resolved. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The terrain-blending change can store a failed allocation result as a valid blend index, exposing later editor operations to out-of-bounds memory access. The demonstrated scope is local WorldBuilder editing. The shared map loader repairs invalid indices, which limits propagation through saved maps. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: b7d8f3e0-b520-425e-9af7-399518604f30
📒 Files selected for processing (13)
Core/Tools/CMakeLists.txtCore/Tools/WorldBuilder/CMakeLists.txtCore/Tools/WorldBuilder/include/BorderTool.hCore/Tools/WorldBuilder/include/WHeightMapEdit.hCore/Tools/WorldBuilder/src/BorderTool.cppCore/Tools/WorldBuilder/src/WHeightMapEdit.cppGenerals/Code/Tools/WorldBuilder/CMakeLists.txtGenerals/Code/Tools/WorldBuilder/include/BorderTool.hGenerals/Code/Tools/WorldBuilder/include/WHeightMapEdit.hGenerals/Code/Tools/WorldBuilder/src/BorderTool.cppGenerals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppGeneralsMD/Code/Tools/WorldBuilder/CMakeLists.txtscripts/cpp/unify_move_files.py
💤 Files with no reviewable changes (3)
- Generals/Code/Tools/WorldBuilder/include/BorderTool.h
- Generals/Code/Tools/WorldBuilder/include/WHeightMapEdit.h
- Generals/Code/Tools/WorldBuilder/src/BorderTool.cpp
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| if (blendTileVersion <= K_BLEND_TILE_VERSION_7) { | ||
| // Version 7 uses the legacy row width, even when the in-memory rows are wider. | ||
| Int byteWidth = (m_width+1)/8; | ||
| for (Int j=0; j<m_height; j++) { | ||
| chunkWriter.writeArrayOfBytes((char*)m_cellCliffState + j*m_flipStateWidth, byteWidth); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'setWidth|setDimensions|m_width[[:space:]]*=|mapWidth|heightMapWidth|resize|newMap|createMap|RETAIL_COMPATIBLE_DATA' Core/Tools/WorldBuilder Core/GameEngineDevice/Source/W3DDevice Generals/Code/Tools/WorldBuilder
rg -n 'width.*(multiple|%|/|64|128)|m_width.*(8|16|32|64|128)' Core/Tools/WorldBuilder Core/GameEngineDevice/Source/W3DDevice/GameClientRepository: TheSuperHackers/GeneralsGameCode
Length of output: 26448
🏁 Script executed:
set -e
printf '%s\n' '--- WHeightMapEdit declarations and construction ---'
sed -n '80,235p' Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp
sed -n '110,175p' Core/Tools/WorldBuilder/include/WHeightMapEdit.h
printf '%s\n' '--- resize implementation ---'
sed -n '1760,1910p' Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- WorldBuilderDoc map creation/loading entrypoints ---'
sed -n '960,1070p' Core/Tools/WorldBuilder/src/WorldBuilderDoc.cpp
rg -n -C 5 'new WorldHeightMapEdit|WorldHeightMapEdit\(|resize\(|initialHeight|xExtent|yExtent|borderWidth' Core/Tools/WorldBuilder/src Core/Tools/WorldBuilder/include
printf '%s\n' '--- dimension validation and format readers ---'
rg -n -C 6 'xSize|ySize|xExtent|yExtent|width.*8|width.*64|MAP_SIZE|map size|MapSize|newWidth|newHeight' Core/Tools/WorldBuilder/src Core/Tools/WorldBuilder/include Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 11905
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- WorldBuilder document creation/loading and resize callers ---'
rg -n -C 8 'WorldHeightMapEdit|resize\(|xExtent|yExtent|initialHeight|borderWidth|NewHeightMap|load' Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp Generals/Code/Tools/WorldBuilder/src Generals/Code/Tools/WorldBuilder/include
printf '%s\n' '--- NewHeightMap declarations and implementation ---'
fd -i 'NewHeightMap' Generals/Code/Tools/WorldBuilder
printf '%s\n' '--- WorldHeightMap loading dimension handling ---'
sed -n '820,985p' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp
sed -n '1025,1090p' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp
printf '%s\n' '--- all dimension-related guards in relevant WorldBuilder files ---'
rg -n -C 4 'BAD_VALUE|<= *0|< *0|% *8|/ *8|multiple|extent|size' Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp Generals/Code/Tools/WorldBuilder/src Generals/Code/Tools/WorldBuilder/include Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp Core/Tools/WorldBuilder/include/WHeightMapEdit.hRepository: TheSuperHackers/GeneralsGameCode
Length of output: 45686
🏁 Script executed:
set -e
printf '%s\n' '--- NewHeightMap files ---'
fd -i 'NewHeightMap' Generals/Code/Tools/WorldBuilder Core/Tools/WorldBuilder
printf '%s\n' '--- NewHeightMap validation ---'
rg -n -C 10 'OnInitDialog|OnOK|DDX|xExtent|yExtent|borderWidth|forResize|SetLimitText|GetDlgItem|BAD_VALUE|<=|< *0|% *8|/ *8' Generals/Code/Tools/WorldBuilder/src/NewHeightMap.cpp Generals/Code/Tools/WorldBuilder/include/NewHeightMap.h Core/Tools/WorldBuilder/src/NewHeightMap.cpp Core/Tools/WorldBuilder/include/NewHeightMap.h 2>/dev/null || true
printf '%s\n' '--- WorldHeightMap constructors and readers ---'
rg -n -C 10 'WorldHeightMap::WorldHeightMap|readInt\(\)|K_BLEND_TILE_VERSION|numBytesX|byteWidth|m_width|m_height' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp Core/GameEngineDevice/Include/W3DDevice/GameClient/WorldHeightMap.hRepository: TheSuperHackers/GeneralsGameCode
Length of output: 41736
🏁 Script executed:
set -e
printf '%s\n' '--- save function and changed hunk ---'
sed -n '560,635p' Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp
rg -n -C 10 'saveToFile\(|SaveMap|OnSaveDocument|OnFileSave|write.*height|heightMap.*save' Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp Generals/Code/Tools/WorldBuilder/src/SaveMap.cpp Generals/Code/Tools/WorldBuilder/include/SaveMap.h 2>/dev/null || true
printf '%s\n' '--- exact parsing and flag readers ---'
rg -n -C 12 'ParseHeightMapData|ParseSizeOnly|m_dataSize.*m_width|byteWidth|numBytesX' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 41363
🏁 Script executed:
set -e
sed -n '250,320p' Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp
sed -n '780,855p' Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp
rg -n -C 6 'throw ERROR|ERROR_CORRUPT_FILE_FORMAT|height map file write failed|saveToFile' Core/Tools/WorldBuilder/src Generals/Code/Tools/WorldBuilder/srcRepository: TheSuperHackers/GeneralsGameCode
Length of output: 12421
🏁 Script executed:
set -e
rg -n -C 12 'setCliffState|m_cellCliffState\[|setCellCliffFlagFromHeights|initCliffFlagsFromHeights' Core/GameEngineDevice/Source/W3DDevice/GameClient/WorldHeightMap.cpp Core/GameEngineDevice/Include/W3DDevice/GameClient/WorldHeightMap.h Core/Tools/WorldBuilder/src/WHeightMapEdit.cpp Core/Tools/WorldBuilder/include/WHeightMapEdit.hRepository: TheSuperHackers/GeneralsGameCode
Length of output: 28808
Reject version-7 saves when the omitted cliff-state byte is non-zero.
WorldBuilder can resize maps to widths where version 7 writes fewer cliff-state bytes than the in-memory row. Normal cliff recalculation can set bits in that omitted byte. The retail version-7 reader does not read it, so reopening the saved map clears those bits.
Version 8 preserves the bits but violates the retail compatibility contract. Check the omitted byte before writing any chunks and reject the save when it contains data.
Suggested fix
void WorldHeightMapEdit::saveToFile(DataChunkOutput &chunkWriter)
{
+ #if RTS_GENERALS && RETAIL_COMPATIBLE_DATA
+ Int legacyByteWidth = (m_width+1)/8;
+ if (legacyByteWidth < m_flipStateWidth) {
+ for (Int j=0; j<m_height; j++) {
+ if (m_cellCliffState[j*m_flipStateWidth + legacyByteWidth] != 0) {
+ throw(ERROR_CORRUPT_FILE_FORMAT);
+ }
+ }
+ }
+ #endif
+
// This is the chunk writer stuff.
int i;
16019dd to
41e4e15
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: e8f4db5c-e63f-4bae-add6-3392b599ac21
📒 Files selected for processing (4)
Generals/Code/Tools/WorldBuilder/include/WHeightMapEdit.hGenerals/Code/Tools/WorldBuilder/src/BorderTool.cppGenerals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppGeneralsMD/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
| Short newNdx = findOrCreateBlendTile(&tempBlendTileInfo); | ||
| m_blendTileNdxes[ndx] = newNdx; //remap this tile to use a new one. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
ast-grep outline Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp \
--match findOrCreateBlendTile --view expanded
ast-grep run --lang cpp \
--pattern 'Int WorldHeightMapEdit::findOrCreateBlendTile(TBlendTileInfo *$INFO) { $$$BODY }' \
Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 306
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- helper ---'
sed -n '1040,1115p' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- changed blend block ---'
sed -n '1145,1215p' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- consumers ---'
rg -n -C 5 'm_blendTileNdxes|autoBlendOut|optimizeTiles' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- requested diff ---'
git diff --unified=35 e70f26db9f0dec4e23f2443921b5341e04958a5f 41e4e15b934fd0d37a60f3bb82c80a161349e1d0 -- Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 41942
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- autoBlendOut relevant range ---'
sed -n '1210,1360p' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp
printf '%s\n' '--- optimizeTiles relevant range ---'
sed -n '1660,1765p' Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cppRepository: TheSuperHackers/GeneralsGameCode
Length of output: 8401
Handle failure before replacing the primary blend index.
findOrCreateBlendTile can return -1 when the blend table is full. The second allocation stores that value in m_blendTileNdxes[ndx] without a check. optimizeTiles and later blend operations can then use -1 as an array index.
Allocate the flipped description before committing the cell indices. If it fails, preserve the previous cell state.
Suggested fix
if (newNdx >= 0) {
Int ndx = (yIndex*m_width)+xIndex;
- m_tileNdxes[ndx] = curTileNdx;
- if (TheGlobalData->m_use3WayTerrainBlends && m_blendTileNdxes[ndx] != 0)
+ Bool hasBaseBlend = TheGlobalData->m_use3WayTerrainBlends &&
+ m_blendTileNdxes[ndx] != 0;
+ Short primaryNdx = hasBaseBlend ? m_blendTileNdxes[ndx] : newNdx;
+ if (hasBaseBlend)
{
//this tile already has a blend applied to it. So we put the new blend into the
//secondary layer.
- m_extraBlendTileNdxes[ndx]=newNdx;
//force the primary layer to flip if the extra blend layer needs flip.
//we only do this on vertical/horizontal base blends because they work in either flip cases.
if (flipped && !baseIsDiagonal)
{
//Find a new tile so as not to affect other cells using the base one.
TBlendTileInfo tempBlendTileInfo=m_blendedTiles[m_blendTileNdxes[ndx]];
tempBlendTileInfo.inverted |= FLIPPED_MASK;
- Short newNdx = findOrCreateBlendTile(&tempBlendTileInfo);
- m_blendTileNdxes[ndx] = newNdx; //remap this tile to use a new one.
+ Short flippedNdx = findOrCreateBlendTile(&tempBlendTileInfo);
+ if (flippedNdx < 0)
+ return;
+ primaryNdx = flippedNdx;
}
- }
- else
- m_blendTileNdxes[ndx] = newNdx;
+ }
+ m_tileNdxes[ndx] = curTileNdx;
+ if (hasBaseBlend)
+ m_extraBlendTileNdxes[ndx] = newNdx;
+ m_blendTileNdxes[ndx] = primaryNdx;
}
41e4e15 to
16fe4d2
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| m_blendedTiles[m_blendTileNdxes[ndx]].inverted |= FLIPPED_MASK; | ||
| { | ||
| //Find a new tile so as not to affect other cells using the base one. | ||
| TBlendTileInfo tempBlendTileInfo=m_blendedTiles[m_blendTileNdxes[ndx]]; |
There was a problem hiding this comment.
🟠 High src/WHeightMapEdit.cpp:1192
When findOrCreateBlendTile returns -1 because the blend-tile table is full or the source tile is unavailable, this path overwrites the valid base index in m_blendTileNdxes[ndx] with -1; subsequent blend/render lookups then index out of bounds and can crash WorldBuilder. Check the allocation result before remapping the cell.
🤖 Copy this AI Prompt to have your agent fix this:
In file @Generals/Code/Tools/WorldBuilder/src/WHeightMapEdit.cpp around line 1192:
When `findOrCreateBlendTile` returns `-1` because the blend-tile table is full or the source tile is unavailable, this path overwrites the valid base index in `m_blendTileNdxes[ndx]` with `-1`; subsequent blend/render lookups then index out of bounds and can crash WorldBuilder. Check the allocation result before remapping the cell.
|
Needs rebase. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Short newNdx = findOrCreateBlendTile(&tempBlendTileInfo); | ||
| m_blendTileNdxes[ndx] = newNdx; //remap this tile to use a new one. |
There was a problem hiding this comment.
findOrCreateBlendTile returns -1. This assignment stores -1 as the cell’s blend index, and later code uses nonzero indices to access the table. Editing a sufficiently complex map can therefore access memory outside the table or corrupt blend data. Check the result before remapping the cell.
| else if (motion == -1) | ||
| { | ||
| // add a boundary | ||
| m_addingNewBorder = true; | ||
|
|
||
| ICoord2D initialBoundary = { 1, 1 }; | ||
| pDoc->addBoundary(&initialBoundary); |
There was a problem hiding this comment.
{1,1} boundary, even if the user releases without dragging. Mouse-up does not register an undo action or discard that boundary, so an accidental miss leaves an unwanted boundary in the map. Make creation undoable or discard an uncompleted boundary.
16fe4d2 to
1bb7eff
Compare
2 participants
Applies the same fix to the Generals and Zero Hour copies of
WHeightMapEdit.cppin their existing directories.findBoundaryNear()permits a nulloutHandle, but its no-match path dereferences it unconditionally. This can crash a lookup when no nearby boundary exists.Guard the final handle assignment, matching the existing checks on successful lookups. The function still sets
outNdxto-1when no boundary matches.Validation:
This branch contains one fix commit directly above main and is independent of the blend-allocation fix in #3384.
Codex generated the fix and local regression fixtures