Clear And Reset Danger - Blog : Coconut Lizard

Like the programmer who added the following code to the codebase, I’m in a bit of a rush right now … so I’m just going to paste a function for you to look at. A seriously broken function.

static void ResetOptions(FBXImportOptions *OptionsToReset)
{
   check(OptionsToReset != nullptr);
   FMemory::Memzero(OptionsToReset, sizeof(OptionsToReset));
}

Honestly, this function had one, simple job to do… FBXImportOptions is a huge structure filled with bools, ints, floats, vectors, maps, arrays, names, strings and more. I’m thinking of Donald Trump waving his arms around here like a concertina.. I mean, this structure is MASSIVE. So, yeah, it made sense that to clear it out, drain the swamp so-to-speak, with a simple Reset() function that would clear out the whole memory.

Except, of course, if you have a keen enough eye, ResetOptions() is, in it’s single line of functional code (other than the pointless null pointer check), failing at that very simple task.

ResetOptions() takes a pointer as an input – then passes that to Memzero(), along with it’s size (the pointer’s size, rather than the object being pointed to) – which will be 8 bytes on a 64bit machine, 4 bytes on a 32bit machine. Hardly enough to clear out that huge, Mexican-border sized structure that I was calling a press conference about earlier. No, if we’re going to clear that monster out, we really need to do the job right… we absolutely need to Make It Great Again.

We can do that simply by changing the code to instead pass the size of the structure:-

static void ResetOptions(FBXImportOptions *OptionsToReset)
{
   check(OptionsToReset != nullptr);
   FMemory::Memzero(OptionsToReset, sizeof(FBXImportOptions));
}

BUT… that would still be wrong, just slightly less wrong than before. This code, as the code before, will result in any arrays and maps within the structure to leak memory. The correct fix would be:-

static void ResetOptions(FBXImportOptions *OptionsToReset)
{
   check(OptionsToReset != nullptr);
   *OptionsToReset = FBXImportOptions{};
}

The moral here? Test your shit. Shit happens – but try to make sure it doesn’t happen in your code. And if it does, blame it on the media

Credit(s): Robert Troughton and Gareth Martin (Coconut Lizard)
Status: Fixed in 4.18