Fix heap overrun in texture readback for the model transparency scan

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<int,ubyte*> bm_lookup_cache;
+struct bm_lookup_cache_entry {
+	ubyte* data;
+	int width;
+	int height;
+};
+SCP_map<int, bm_lookup_cache_entry> 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;
 	}
 }
 
@@ -190,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));
 
@@ -198,7 +209,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();
 }

code/graphics/2d.h

@@ -840,7 +840,10 @@ typedef struct screen {
 	std::function<void()> gf_clear_states;
 
 	std::function<void(int bitmap_handle, int bpp, const ubyte* data, int width, int height)> gf_update_texture;
-	std::function<void(void* data_out, int bitmap_num)> 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<ubyte*(int bitmap_num, int* width_out, int* height_out)> gf_get_bitmap_from_texture;
 
 	std::function<void(matrix4* shadow_view_matrix, const matrix* light_matrix, vec3d* eye_pos, bool first_pass)> gf_shadow_map_start;
 	std::function<void()> gf_shadow_map_end;

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*/)

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<tcache_slot_opengl>(bitmap_num, true);
-	
+
 	GLenum pixel_format = GL_RGB;
 	GLenum data_format = GL_UNSIGNED_BYTE;
 	int bytes_per_pixel = 3 * sizeof(ubyte);
@@ -1431,24 +1436,46 @@ 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<uint32_t>(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<std::uint8_t[]> buffer(new std::uint8_t[num_frames * slice_size]);
+	// 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);
 
-	Assertion(ts->texture_target == GL_TEXTURE_2D_ARRAY, "Unexpected texture target encountered!");
+	// The size of a single layer in the array
+	size_t slice_size = static_cast<size_t>(gl_width) * gl_height * bytes_per_pixel;
+
+	// 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<size_t>((gl_width + 3) & ~3) * ((gl_height + 3) & ~3) * bytes_per_pixel;
+	std::unique_ptr<std::uint8_t[]> 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<ubyte*>(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)
@@ -1494,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;
 	}

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);

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*/)
 {