From 58afecc286b530d94313fa0ea0e90c6bda1ae2d0 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Sat, 18 Jul 2026 00:33:50 -0400 Subject: [PATCH 1/3] Fix heap overrun in texture readback for the model transparency scan gr_opengl_get_bitmap_from_texture() sized its readback buffer from the texture cache slot dimensions and the bmpman frame count, but glGetTexImage() returns whatever the bound GL texture actually contains. When texture detail below maximum culls textures on upload, the two can disagree, and the readback overruns the buffer (observed as heap corruption when loading Zeus.pof with Texture Detail: Minimum). Size the readback from the dimensions and layer count OpenGL itself reports for the texture, and pad the staging buffer to 4x4 block granularity in case a driver writes tiny compressed mips back in whole blocks. The readback now also allocates the returned buffer and reports the dimensions of the data it returns, because the second half of the bug was on the consumer side: bitmap_lookup indexed the readback data by the bitmap's own dimensions, so with any culling in effect the transparency scan read mostly uninitialized memory. bitmap_lookup (and its cache) now carries the actual readback dimensions, and it skips the texture cleanly if the readback fails. Also pairs the cache's vm_malloc allocations with vm_free instead of free, and no longer ignores a failed tcache_set before reading back. Co-Authored-By: Claude Fable 5 --- code/bmpman/bmpman.cpp | 36 +++++++++-------- code/graphics/2d.h | 5 ++- code/graphics/grstub.cpp | 6 ++- code/graphics/opengl/gropengltexture.cpp | 49 +++++++++++++++++------- code/graphics/opengl/gropengltexture.h | 2 +- code/graphics/vulkan/vulkan_stubs.cpp | 7 +++- 6 files changed, 71 insertions(+), 34 deletions(-) diff --git a/code/bmpman/bmpman.cpp b/code/bmpman/bmpman.cpp index 3592f6d56fa..7dfcc651ead 100644 --- a/code/bmpman/bmpman.cpp +++ b/code/bmpman/bmpman.cpp @@ -83,7 +83,12 @@ static bool bm_inited = false; static uint Bm_next_signature = 0x1234; static int Bm_low_mem = 0; -SCP_map bm_lookup_cache; +struct bm_lookup_cache_entry { + ubyte* data; + int width; + int height; +}; +SCP_map bm_lookup_cache; /** * How much RAM bmpman can use for textures. @@ -144,30 +149,31 @@ bitmap_slot* bm_get_slot(int handle, bool separate_ani_frames) { // Declaration of private functions and templates(declared as static type func(type param);) bitmap_lookup::bitmap_lookup(int bitmap_num): - Bitmap_data(NULL) + Bitmap_data(nullptr), + Width(0), + Height(0), + Num_channels(3) { if ( !bm_is_valid(bitmap_num) ) return; - Num_channels = 3; - if ( bm_has_alpha_channel(bitmap_num) ) { Num_channels = 4; } - bitmap_entry *be = bm_get_entry(bitmap_num); - - Width = be->bm.w; - Height = be->bm.h; - - auto cache_search = bm_lookup_cache.find(bitmap_num); if (cache_search == bm_lookup_cache.end()) { - Bitmap_data = (ubyte*)vm_malloc(Width * Height * Num_channels * sizeof(ubyte)); + // The texture in graphics memory may be smaller than the bitmap itself, e.g. when mipmap levels + // are culled at lower texture detail settings, so the readback reports the dimensions of the + // data it actually returns. + Bitmap_data = gr_get_bitmap_from_texture(bitmap_num, &Width, &Height); - gr_get_bitmap_from_texture((void*)Bitmap_data, bitmap_num); - bm_lookup_cache.insert({bitmap_num, Bitmap_data}); + if (Bitmap_data != nullptr) { + bm_lookup_cache.insert({bitmap_num, {Bitmap_data, Width, Height}}); + } } else { - Bitmap_data = cache_search->second; + Bitmap_data = cache_search->second.data; + Width = cache_search->second.width; + Height = cache_search->second.height; } } @@ -198,7 +204,7 @@ float bitmap_lookup::get_channel_alpha(float u, float v) void clear_bm_lookup_cache() { for(auto &iter: bm_lookup_cache) { - free(iter.second); + vm_free(iter.second.data); } bm_lookup_cache.clear(); } diff --git a/code/graphics/2d.h b/code/graphics/2d.h index 4b7aa08bf20..280d60edec0 100644 --- a/code/graphics/2d.h +++ b/code/graphics/2d.h @@ -840,7 +840,10 @@ typedef struct screen { std::function gf_clear_states; std::function gf_update_texture; - std::function gf_get_bitmap_from_texture; + // Reads the texture uploaded for a bitmap back from graphics memory. Returns a vm_malloc'd buffer + // (freed by the caller with vm_free) and reports the dimensions of the returned data, which may be + // smaller than the bitmap if the texture was culled at upload; returns nullptr on failure. + std::function gf_get_bitmap_from_texture; std::function gf_shadow_map_start; std::function gf_shadow_map_end; diff --git a/code/graphics/grstub.cpp b/code/graphics/grstub.cpp index 3803c668b8e..edca96d8b8d 100644 --- a/code/graphics/grstub.cpp +++ b/code/graphics/grstub.cpp @@ -259,9 +259,11 @@ void gr_stub_update_texture(int /*bitmap_handle*/, int /*bpp*/, const ubyte * { } -void gr_stub_get_bitmap_from_texture(void* /*data_out*/, int /*bitmap_num*/) +ubyte* gr_stub_get_bitmap_from_texture(int /*bitmap_num*/, int* width_out, int* height_out) { - + *width_out = 0; + *height_out = 0; + return nullptr; } int gr_stub_bm_make_render_target(int /*n*/, int * /*width*/, int * /*height*/, int * /*bpp*/, int * /*mm_lvl*/, int /*flags*/) diff --git a/code/graphics/opengl/gropengltexture.cpp b/code/graphics/opengl/gropengltexture.cpp index 26dfa57fd9b..9ef22e42931 100644 --- a/code/graphics/opengl/gropengltexture.cpp +++ b/code/graphics/opengl/gropengltexture.cpp @@ -1413,15 +1413,20 @@ int opengl_get_texture( GLenum target, GLenum pixel_format, GLenum data_format, return m_offset; } -void gr_opengl_get_bitmap_from_texture(void* data_out, int bitmap_num) +ubyte* gr_opengl_get_bitmap_from_texture(int bitmap_num, int* width_out, int* height_out) { + *width_out = 0; + *height_out = 0; + float u,v; uint32_t array_index = 0; - gr_opengl_tcache_set(bitmap_num, TCACHE_TYPE_NORMAL, &u, &v, &array_index); + if ( !gr_opengl_tcache_set(bitmap_num, TCACHE_TYPE_NORMAL, &u, &v, &array_index) ) { + return nullptr; + } auto *ts = bm_get_gr_info(bitmap_num, true); - + GLenum pixel_format = GL_RGB; GLenum data_format = GL_UNSIGNED_BYTE; int bytes_per_pixel = 3 * sizeof(ubyte); @@ -1431,24 +1436,40 @@ void gr_opengl_get_bitmap_from_texture(void* data_out, int bitmap_num) bytes_per_pixel = 4 * sizeof(ubyte); } - // We can't read a specific layer of the texture so we need to read the entire texture and then memcpy the right part from that... - int num_frames = 0; - bm_get_info(bitmap_num, nullptr, nullptr, nullptr, &num_frames); - if (!bm_is_texture_array(bitmap_num)) { - num_frames = 1; + Assertion(ts->texture_target == GL_TEXTURE_2D_ARRAY, "Unexpected texture target encountered!"); + + // The texture in graphics memory may not match the dimensions or frame count that bmpman reports for + // this handle, e.g. when mipmap levels are culled at lower texture detail settings, so size the + // readback from what OpenGL will actually write, not from the bitmap or the texture cache slot. + GLint gl_width = 0, gl_height = 0, gl_layers = 0; + glGetTexLevelParameteriv(ts->texture_target, 0, GL_TEXTURE_WIDTH, &gl_width); + glGetTexLevelParameteriv(ts->texture_target, 0, GL_TEXTURE_HEIGHT, &gl_height); + glGetTexLevelParameteriv(ts->texture_target, 0, GL_TEXTURE_DEPTH, &gl_layers); + + if ( (gl_width < 1) || (gl_height < 1) || (gl_layers < 1) || (array_index >= static_cast(gl_layers)) ) { + mprintf(("Cannot read bitmap %d (%s) back from its texture; OpenGL reports level 0 as %dx%d with %d layer(s) but layer %u is needed.\n", + bitmap_num, bm_get_filename(bitmap_num), gl_width, gl_height, gl_layers, array_index)); + return nullptr; } - // The size of a single frame in the array - auto slice_size = ts->w * ts->h * bytes_per_pixel; - std::unique_ptr buffer(new std::uint8_t[num_frames * slice_size]); + // The size of a single layer in the array + size_t slice_size = static_cast(gl_width) * gl_height * bytes_per_pixel; - Assertion(ts->texture_target == GL_TEXTURE_2D_ARRAY, "Unexpected texture target encountered!"); + // We can't read a specific layer of the texture so we need to read the entire texture and then memcpy the right part from that... + // Some drivers write tiny compressed textures back in whole 4x4 block granularity, so pad each layer + // of the staging buffer to full blocks to keep any such overwrite within bounds. + size_t padded_slice_size = static_cast((gl_width + 3) & ~3) * ((gl_height + 3) & ~3) * bytes_per_pixel; + std::unique_ptr buffer(new std::uint8_t[padded_slice_size * gl_layers]); // Copy the entire texture level into the bitmap glGetTexImage(ts->texture_target, 0, pixel_format, data_format, buffer.get()); - auto buffer_offset = array_index * slice_size; - memcpy(data_out, buffer.get() + buffer_offset, slice_size); + auto data_out = reinterpret_cast(vm_malloc(slice_size)); + memcpy(data_out, buffer.get() + array_index * slice_size, slice_size); + + *width_out = gl_width; + *height_out = gl_height; + return data_out; } void gr_opengl_get_texture_scale(int bitmap_handle, float *u_scale, float *v_scale) diff --git a/code/graphics/opengl/gropengltexture.h b/code/graphics/opengl/gropengltexture.h index 26be90d02e0..e8933fd3e3a 100644 --- a/code/graphics/opengl/gropengltexture.h +++ b/code/graphics/opengl/gropengltexture.h @@ -83,7 +83,7 @@ void opengl_preload_init(); void opengl_kill_render_target(bitmap_slot* slot); int opengl_make_render_target(int handle, int *w, int *h, int *bpp, int *mm_lvl, int flags); int opengl_set_render_target(int slot, int face = -1, int is_static = 0); -void gr_opengl_get_bitmap_from_texture(void* data_out, int bitmap_num); +ubyte* gr_opengl_get_bitmap_from_texture(int bitmap_num, int* width_out, int* height_out); size_t opengl_export_render_target( int slot, int width, int height, int alpha, int num_mipmaps, ubyte *image_data ); void opengl_set_texture_target(GLenum target = GL_TEXTURE_2D); void opengl_set_texture_face(GLenum face = GL_TEXTURE_2D); diff --git a/code/graphics/vulkan/vulkan_stubs.cpp b/code/graphics/vulkan/vulkan_stubs.cpp index 6d4f3d0230a..bd115750544 100644 --- a/code/graphics/vulkan/vulkan_stubs.cpp +++ b/code/graphics/vulkan/vulkan_stubs.cpp @@ -106,7 +106,12 @@ void stub_clear_states() {} void stub_update_texture(int /*bitmap_handle*/, int /*bpp*/, const ubyte* /*data*/, int /*width*/, int /*height*/) {} -void stub_get_bitmap_from_texture(void* /*data_out*/, int /*bitmap_num*/) {} +ubyte* stub_get_bitmap_from_texture(int /*bitmap_num*/, int* width_out, int* height_out) +{ + *width_out = 0; + *height_out = 0; + return nullptr; +} int stub_bm_make_render_target(int /*n*/, int* /*width*/, int* /*height*/, int* /*bpp*/, int* /*mm_lvl*/, int /*flags*/) { From d54b7e064f27a6952d87bcc9be3a046f16f05e40 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Sat, 18 Jul 2026 01:04:59 -0400 Subject: [PATCH 2/3] Harden the transparency-scan texture readback Return fully opaque from bitmap_lookup::get_channel_alpha() when the bitmap has no alpha channel, since indexing channel 3 of a 3-channel buffer reads out of bounds (currently unreachable because the only caller checks bm_has_alpha_channel() first, but no longer a trap). Per review, also assert at maximum texture detail, where no culling occurs on upload, that the readback dimensions OpenGL reports match the texture cache slot, so any future slot mismatch is caught immediately in debug builds instead of surfacing as corruption. Co-Authored-By: Claude Fable 5 --- code/bmpman/bmpman.cpp | 5 +++++ code/graphics/opengl/gropengltexture.cpp | 6 ++++++ 2 files changed, 11 insertions(+) diff --git a/code/bmpman/bmpman.cpp b/code/bmpman/bmpman.cpp index 7dfcc651ead..c0f284a327e 100644 --- a/code/bmpman/bmpman.cpp +++ b/code/bmpman/bmpman.cpp @@ -196,6 +196,11 @@ float bitmap_lookup::get_channel_alpha(float u, float v) { Assert( Bitmap_data != NULL ); + // without an alpha channel there is nothing to look up, and indexing channel 3 would read out of bounds + if ( Num_channels < 4 ) { + return 1.0f; + } + int x = fl2i(map_texture_address(u) * (Width-1)); int y = fl2i(map_texture_address(v) * (Height-1)); diff --git a/code/graphics/opengl/gropengltexture.cpp b/code/graphics/opengl/gropengltexture.cpp index 9ef22e42931..240eb982f47 100644 --- a/code/graphics/opengl/gropengltexture.cpp +++ b/code/graphics/opengl/gropengltexture.cpp @@ -1452,6 +1452,12 @@ ubyte* gr_opengl_get_bitmap_from_texture(int bitmap_num, int* width_out, int* he return nullptr; } + // At maximum texture detail no culling occurs on upload, so the texture must match the dimensions + // recorded in the texture cache slot; if this trips, we have another slot mismatch. + Assertion((Detail.hardware_textures < 4) || ((gl_width == ts->w) && (gl_height == ts->h)), + "Texture readback size mismatch for bitmap %d (%s): OpenGL reports level 0 as %dx%d but the texture cache slot expects %dx%d.", + bitmap_num, bm_get_filename(bitmap_num), gl_width, gl_height, ts->w, ts->h); + // The size of a single layer in the array size_t slice_size = static_cast(gl_width) * gl_height * bytes_per_pixel; From 86585ce23f2ce10ef8e809ac9b198164257fe152 Mon Sep 17 00:00:00 2001 From: Goober5000 Date: Sat, 18 Jul 2026 01:22:17 -0400 Subject: [PATCH 3/3] Fix inverted dimension check in opengl_export_render_target The validation rejected the export only if both the width and the height mismatched the texture cache slot, so a texture wrong in one dimension passed through into opengl_get_texture, which trusts the caller-supplied dimensions for its mipmap loop. Co-Authored-By: Claude Fable 5 --- code/graphics/opengl/gropengltexture.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/graphics/opengl/gropengltexture.cpp b/code/graphics/opengl/gropengltexture.cpp index 240eb982f47..b31c54945e8 100644 --- a/code/graphics/opengl/gropengltexture.cpp +++ b/code/graphics/opengl/gropengltexture.cpp @@ -1521,7 +1521,7 @@ size_t opengl_export_render_target( int slot, int width, int height, int alpha, return 0; } - if ( (ts->w != width) && (ts->h != height) ) { + if ( (ts->w != width) || (ts->h != height) ) { mprintf(("OpenGL ERROR: Passed width and height do not match values for texture!\n")); return 0; }