search

blue-nebula / base

Main repository of Blue Nebula, a fast-paced shooter with a unique parkour system. It is a fork of Red Eclipse and is free software.
https://blue-nebula.org
15 stars 6 forks source link

Fix all wrong usages of delete and delete[] in code base #176

Open TheAssassin opened 3 years ago

TheAssassin commented 3 years ago

Reference: https://isocpp.org/wiki/faq/freestore-mgmt#mixing-malloc-and-delete

While looking through the code base, I've found many issues with memory management. Among the more problematic ones are most usages of the DELETEA (alias for delete[]) and DELETEP (alias for delete) macros. Those are mostly used on malloc()d data.

In short, one needs to always use free on malloc()d data (basically any memory allocated by libc, also if not directly allocated by the user, e.g., when using strdup). delete is used when constructing objects on the heap using the new keyword in C++ only (which is hardly ever done), delete[] is reserved for arrays allocated using new sometype[]. Technically, both malloc() and new operate on different memory segments (heaps), and thus free() and delete cannot be used interchangeably.

Someone needs to go through the entire code base and fix all these memory cleanups. This should eliminate quite some memory leaks. I suggest to dump those (IMO rather pointless) macros first, and then use the appropriate operators or routines to clean up memory in the code.

robalni commented 3 years ago

I can't really find any uses of malloc in the code. Can you give some examples of where this happens?

TheAssassin commented 3 years ago

Indeed, I just re-checked, and it's a lot less than I expected. Seems I overestimated their use. The few occurrences where I saw it are safe already. Don't ask me why, but I was pretty convinced newstring was using some kind of malloc internally. Tracing all macros, it turns out it uses new char[...]. So, you're right, I'll take that back. There are a few calls like realpath, calloc etc. which should be re-checked at some point. We'd need to make a collection of libc/POSIX methods which we needed to check for.

I still vouch for using delete and delete[]. This will increase the readability of the codebase significantly. I am pretty sure to have observed a few wrong usages of DELETEP and DELETEA (where the other one should've been used). IMO those macros just ruin the readability, and I cannot think of the motivation behind adding those. They don't "save space" or anything anyway...

robalni commented 3 years ago

Yes, I think replacing those macros would be a good idea.