Bug Tracker – Bug 1127

Heap corruption when R_InitSkyMap changes height of sky texture

Last modified: 2016-11-15 00:00:25 UTC
Bug 1127 - Heap corruption when R_InitSkyMap changes height of sky texture
Summary: Heap corruption when R_InitSkyMap changes height of sky texture
Status: ASSIGNED
Alias: None
Product: Odamex
Classification: Unclassified
Component: Client (show other bugs)
Version: 0.7.x (Old)
Hardware: All All
: P1 normal
Assignee: Odamex Bug Reporter
URL:
Depends on:
Blocks:
 
Reported: 2014-10-26 16:43 UTC by RjY
Modified: 2016-11-15 00:00 UTC (History)
2 users (show)

See Also:

Attachments

Test case: 512x128 sky texture made from two 256x200 patches (8.86 KB, application/gzip)
2014-10-26 16:43 UTC, RjY
Details
Recalculate texture allocation if R_InitSkyMap changes its height (1.74 KB, patch)
2014-10-26 16:48 UTC, RjY
Details | Diff
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description RjY 2014-10-26 16:43:03 UTC
Created attachment 515 [details]
Test case: 512x128 sky texture made from two 256x200 patches

R_InitTextures reads and parses TEXTURE1/PNAMES, then calls R_GenerateLookup for each texture to precalculate an allocation size (texturecompositesize[texnum]). The allocation is not made (Z_Malloc2 is not called) until R_GenerateComposite is called to build the texture, on its first use in a level.

However, between these two steps, R_InitSkyMap is called, which detects that a sky of height H1 (128 in this case) is composed of patches of height H2 (200 in this case). Noticing H2 > H1, it sets the height of the texture to H2 without making the necessary adjustment to the precalculated allocation size.

Therefore the allocation made later in R_GenerateComposite is too small, and data is written past the end of the allocated block, resulting in heap corruption, which manifests as a segfault on the next call to Z_Malloc2.

As the wad with the original problem (dk_dm_4.wad) has been fixed, I attach a test case. 256x200.wad replicates the former situation in dk_dm_4.wad of a 512x128 sky texture composed from two 256x200 patches.

Possible solutions:

- Call R_GenerateLookup again in R_InitSkyMap if the latter decides to change the sky texture's height.

- Stop fiddling with the texture height in R_InitSkyMap. (Why is fiddling the height necessary at all? It seems like a hack to support some WAD with broken texture definitions...)

- Dispense with the preallocation step entirely, and do all texture composition lazily. Remove texturecompositesize[], merge R_GenerateLookup into R_GenerateComposite.
Comment 1 RjY 2014-10-26 16:48:47 UTC
Created attachment 516 [details]
Recalculate texture allocation if R_InitSkyMap changes its height

This fixes the crash in 256x200.wad (see other attachment).

But it seems like a hack. Making texture composition more lazy seems like a better solution.
Comment 2 Russell Rice 2014-11-02 05:29:32 UTC
Trippy wad, patch committed in 5144, will leave bug open for further review
Comment 3 Mike Lightner 2014-12-16 00:05:43 UTC
Marking assigned as work has been done on this bug.
Comment 4 Mike Lightner 2014-12-29 06:02:47 UTC
I don't see why we are fiddling with the height there.  It's not in 1.23 code.  I've removed it and this then fixes bug 1129.
Comment 5 HeX_Vulture 2016-11-15 00:00:25 UTC
Is this considered resolved?