D_BIndVariables memory leak fix proposal.

[devnexen]

refactor string copy from tag allocations.

[fabiangreffrath]

Sorry, but I somehow fail to see the memory leak and how this approach could solve it. 🤷

[devnexen]

master gives
Direct leak of 136 byte(s) in 10 object(s) allocated from: #0 0x557489f4c513 in strdup (/home/dcarlier/Contribs/chocolate-doom/build/src/chocolate-doom+0x12e513) (BuildId: 511527ddd5cc035056fb2c5e1b13001f71457c71) #1 0x557489f9f0d4 in M_StringDuplicate /home/dcarlier/Contribs/chocolate-doom/src/m_misc.c:655:14 #2 0x55748a021fd8 in D_BindVariables /home/dcarlier/Contribs/chocolate-doom/src/doom/d_main.c:382:26

with the changes, it no longer appears.

[fabiangreffrath]

Yes, sure, this silences a valgrind warning, because M_StringDuplicate() calls strdup() which allocates memory for the duplicated string that is never free()d. However, by moving it to zone memory you merely create a duplicated string in memory that has already been allocated before.

[turol]

This change doesn't belong here.

[turol]

I'm not sure this is a good idea. There were some issues where demo reproducibility depended on size of the memory pool. Does anyone know if the same will happen when we change what goes in there?

Also this doesn't really fix the leak, it just goes inside the memory pool. It will still conceptually leak if no-one cleans it out.

How would this interact with any fix to #1389?

[turol]

These are the bugs: #530 #534. Changing the amount of free zone memory can alter demo behavior and therefore so can this change I think.

[fragglet]

These are the bugs: #530 #534. Changing the amount of free zone memory can alter demo behavior and therefore so can this change I think.

We shouldn't let this affect our decision - if a demo's sync depends on heap allocations then we need a more robust solution anyway. We already do different heap allocations to vanilla anyway and I'd rather not rely on fragile solutions to problems.

[fragglet]

The function name is confusing because it's more equivalent to M_StringDuplicate in my opinion.

But I'm not convinced we should be using the zone heap to allocate strings anyway. That's really for larger allocations like WAD lumps and things that need to be efficiently allocated and cached while the game is running. Should we just switch to using the normal C heap for the dehacked string table?