Novum / vkQuake

Vulkan Quake port based on QuakeSpasm
GNU General Public License v2.0
1.84k stars 224 forks source link

Leaks in R_TranslateNewPlayerSkin and TexMgr_ReloadImage #277

Closed rsfb closed 3 years ago

rsfb commented 3 years ago

Note: This is copied from pull request #274 , which was merged and then reverted (see also issue #275).

This was observed on both Debug x64 and Release x64 builds, with an AMD Radeon RX 5700 XT.

There are at least three ways to reproduce the issue:

1- Spamming the following two console commands in succession (probably the fastest way to reproduce the TexMgr_ReloadImage leak):

gl_fullbrights 0 gl_fullbrights 1

2- Playing Deathmatch with a few Omicron bots on DM4 (how I first noticed the problem). With 16 players, takes about 11 minutes to crash on my system;

3- In a deathmatch game, spamming the color command with different parameters to change the player’s texture (similar to what the Omicron bots do internally).

All of these will show a steady increase in GPU memory usage, and eventually cause vkAllocateDescriptorSets to fail with VK_ERROR_OUT_OF_POOL_MEMORY (in the call to TexMgr_LoadImage32).

rsfb commented 3 years ago

In pull request #274 , Alex mentioned a crash with the fix merged - do you happen to have a repro?

Novum commented 3 years ago

My name is Axel. It crashes in ad_tears.

rsfb commented 3 years ago

Sorry for the misreading! :(

Thanks, I'll check that out!

rsfb commented 3 years ago

Calling R_NewGame in Host_ClearMemory seems to fix the ad_tears crash, by resetting to NULL the pointers to deleted textures.

I'm still testing this, but the current work-in progress fix is: https://gist.github.com/rsfb/af56f285f8789994f00b72a20db1d717

Novum commented 3 years ago

That looks scary. R_NewGame probably has other side effects?

rsfb commented 3 years ago

R_NewGame is currently defined as:

void R_NewGame (void)
{
    int i;

    //clear playertexture pointers (the textures themselves were freed by texmgr_newgame)
    for (i=0; i<MAX_SCOREBOARD; i++)
        playertextures[i] = NULL;
}

I'll double-check this, but from memory, the ad_tears crash was related to playertextures.

Novum commented 3 years ago

87b5eec035961db486ced1f9e8577944e97f263a

rsfb commented 3 years ago

Thank you! :)