StringTable::Erase() and StringTable::Reclaim() leak memory

[hexagonrecursion]

Observed behaviour

StringTable::Erase() appears to leak memory. I detected this by running ./build/unittest -tc=StringName -sc=Creation under a memory leak detector. StringTable::Reclaim() also leaks because it uses StringTable::Erase()

Expected behaviour

No memory leak.

Steps to reproduce

1. Enable sanitizers when creating the build directory. Note: be aware that cmake does not change CXXFLAGS or LDFLAGS for an already populated build directory.

   CXXFLAGS="-fsanitize=address,pointer-compare,pointer-subtract,undefined" LDFLAGS="$CXXFLAGS" cmake -S . -B build-sanitize/
   

2. make -j2 -C build-sanitize/

3. ./build-sanitize/unittest -tc=StringName -sc=Creation

==4578==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 26 byte(s) in 1 object(s) allocated from:
    #0 0x7f3cbc3692ef in malloc (/lib64/libasan.so.8+0xd92ef) (BuildId: 7fcb7759bc17ef47f9682414b6d99732d6a6ab0c)
    #1 0x69813b in StringName::make_data(char const*, unsigned int, unsigned int) /home/cdda/git/pioneer/src/core/StringName.cpp:60

SUMMARY: AddressSanitizer: 26 byte(s) leaked in 1 allocation(s).

My pioneer version (and OS): 97d7be6 (and Fedora release 39)

Notes

Memory is allocated here:

pioneer/src/core/StringName.cpp

Lines 56 to 67 in 97d7be6

	StringName::StringData *StringName::make_data(const char *c, uint32_t s, uint32_t h)
	{
	auto &entry = StringTable::Get()->FindOrCreate(h);
	if (!entry) {
	entry = new (std::malloc(sizeof(StringData) + s + 1)) StringData();
	std::memcpy(entry->get(), c, s);
	entry->get()[s] = '\0';
	}
	entry->ref();
	return entry;
	}

I think memory is leaked here:

pioneer/src/core/StringName.cpp

Lines 134 to 154 in 97d7be6

	if (probed_key == key) {
	entries--;
	// backshift all following entries
	for (uint32_t jdx = idx + 1;; idx++, jdx++) {
	idx &= keys.size() - 1;
	jdx &= keys.size() - 1;
	// stopcode, no need to continue swapping
	if (!keys[jdx] || !dist[jdx]) {
	keys[idx] = 0;
	dist[idx] = 0;
	values[idx] = nullptr;
	return;
	} else {
	keys[idx] = keys[jdx];
	dist[idx] = dist[jdx] - 1;
	values[idx] = values[jdx];
	}
	}
	}

Sanity check: you allocate memory via std::malloc(), but I see precisely 0 occurrences of "free" in StringName.h or in StringName.cpp

@Web-eWorks , you wrote the code. The code is complicated. I am not sure I understand it. Can you confirm?

[Web-eWorks]

I believe your surmise to be correct. The StringTable maps interned string hashes to string data, which is stored in an equivalent manner to a C-style flexible array member (the StringName::StringData structure is used as a header for the allocated string character data). It does look like I missed a deletion of that string data when erasing a string from the table, though that technically leaves dangling references if erasing a string with a non-zero refcount (i.e. outside of Reclaim).

If interested in fixing this, the allocated string data should be freed in Reclaim() before calling Erase(), and the Erase method ideally made private to the class.

[hexagonrecursion]

No promises