trjhtr

A legacy project I am maintaining in my freetime uses overrides of operator delete / operator new. These overrides over-allocate by some bytes using malloc (and free) and treat said bytes as a prefix filled with a signature different between new and new. Checking the signature on delete allows to discover accidental cases of "new ... delete" or "new ... delete". Returned is the malloc'ed pointer offset by the size of the prefix.

These overrides are put behind an #ifdef that does not evaluate in release mode.

I recently rewrote this part, using guidance from the relevant cppreference page and find that I am left with three questions I have trouble to satisfyingly answer. These are my questions:

1. Is this code even still necessary or could I replace it with some builtin feature or plugin? (Currently using latest non-preview MSVC++ toolchain, might switch to MinGW-w64 at some indeterminate time in the future... currently the project does not compile on GCC though.)


2. Did I avoid UB at least in the non-pathological use cases? (And even if, examples of such cases are welcome.)


3. The original legacy code implements both operator delete and delete by calling free, essentially meaning their implementation (prefix handling aside) is identical... if that must be so, then why bother in the first place?

Of course, considerations beyond those pertaining to this list are also welcome.

Code to be reviewed:

#include <cstring>

constexpr char PREFIX_ARRAY = "STR2MED";
constexpr char PREFIX_OBJECT = "str1med";
constexpr auto OFFSET_ARRAY = std::size(PREFIX_ARRAY);
constexpr auto OFFSET_OBJECT = std::size(PREFIX_OBJECT);
using OFFSET_TYPE = decltype(OFFSET_ARRAY + OFFSET_OBJECT);

template<OFFSET_TYPE OFFSET>
inline void * guardDelete(void * pointer, const char PREFIX[OFFSET])

 if(pointer == nullptr)
 return pointer;
 else
 
 char * prefix = reinterpret_cast<char*>(pointer) - OFFSET;
 ASSERT(std::memcmp(prefix, PREFIX, OFFSET) == 0);

 return prefix;
 


void __cdecl operator delete (void * pointer) std::free(guardDelete<OFFSET_OBJECT>(pointer, PREFIX_OBJECT)); 
void __cdecl operator delete(void * pointer) std::free(guardDelete<OFFSET_ARRAY> (pointer, PREFIX_ARRAY)); 

template<OFFSET_TYPE OFFSET>
inline void * guardNew(void * pointer, const char PREFIX[OFFSET])

 ASSERT(pointer != nullptr); // Out of virtual memory?
 std::memcpy(pointer, PREFIX, OFFSET);

 return reinterpret_cast<char*>(pointer) + OFFSET;


void * __cdecl operator new (std::size_t size) return guardNew<OFFSET_OBJECT>(std::malloc(size + OFFSET_OBJECT), PREFIX_OBJECT); 
void * __cdecl operator new(std::size_t size) return guardNew<OFFSET_ARRAY> (std::malloc(size + OFFSET_ARRAY), PREFIX_ARRAY); 

External dependency (ASSERT implementation; still legacy code; not to be reviewed):

// This spawns a handy Windows error message allowing one to start VS
// and attach the debugger while the context is still intact;
// with debugger already attached, it acts as a breakpoint.
void _AssertDebug(char * strAssertion, char * strFile, unsigned int uLine)

 static char str[1024];
 sprintf(str, "n--- Assertion failed: %s file:%s line:%u ---nDo you still want to continue?", strAssertion, strFile, uLine);
 switch(MessageBox(GetActiveWindow(), str, "Fatal error!", MB_ABORTRETRYIGNORE

#define ASSERT(f) 
 if (f) 
 
 else 
 _AssertDebug((#f), __FILE__, __LINE__)

#include <cstring>

Also include <cstdlib> (for std::malloc), and I'd include <new> out of paranoia anytime I'm messing with operator new, even if it might be unnecessary in this case.

constexpr char PREFIX_ARRAY = "STR2MED";
constexpr char PREFIX_OBJECT = "str1med";
constexpr auto OFFSET_ARRAY = std::size(PREFIX_ARRAY);
constexpr auto OFFSET_OBJECT = std::size(PREFIX_OBJECT);

This strikes me as needlessly complicated. Also, such similar names invite accidental typo-bugs (e.g. mistyping OFFSET_ARRAY as OFFSET_OBJECT). Also, the use of the C++14 library algorithm std::size is unnecessary — (sizeof will do exactly what you want); and the use of auto is obfuscatory (the type of sizeof x is invariably size_t).

If you really wanted to use std::size, you should have included at least one of <array>, <deque>, <forward_list>, <iterator>, <list>, <map>, <regex>, <set>, <string>, <unordered_map>, <unordered_set>, <vector>.

Also, if you need these variables at all, you might consider putting them in a namespace, so that they don't conflict with the user's own global variable or function names.

using OFFSET_TYPE = decltype(OFFSET_ARRAY + OFFSET_OBJECT);

This is super duper confusing, or else it's one of those typo-bugs I was warning you about. Here you're adding together OFFSET_ARRAY (i.e. size_t(8)) and OFFSET_OBJECT (i.e. size_t(8)), taking the decltype of that (i.e. size_t), and declaring a typedef named OFFSET_TYPE. If you really wanted this typedef, you should write it as

using OFFSET_TYPE = size_t; // or std::size_t if you enjoy typing

Related: I never write std:: for the stuff that I know is coming from libc anyway. That's a personal quirk, though. I get the impression that a lot of people do write std:: everywhere, even where it's not technically needed.

template<OFFSET_TYPE OFFSET>
inline void * guardDelete(void * pointer, const char PREFIX[OFFSET])

Here I would worry that the programmer didn't know that arrays in C and C++ are passed "by decay to pointer". I would prefer to write this as

template<size_t Offset>
inline void *guardDelete(void *ptr, const char *prefix)

so that it is obvious that Offset is not deducible. Notice also that I'm consistently using snake_case for local variable names, and CamelCase for template parameter names (such as Offset); I find that this makes code easier to decipher than using a mix of lower and UPPER case.

void __cdecl operator delete (void * pointer)

If I cared about portability to non-MSVC targets, I'd preemptively factor out this __cdecl into a macro that I could easily turn off. However, it occurs to me that __cdecl itself is an identifier that could be defined away on non-MSVC platforms; so sure, let's leave it alone.

So putting it all together, I'd have:

#include <string.h>
#include <stdlib.h>
#include <new> // maybe

template<size_t N>
inline void *guardNew(void *pointer, const char (&guard)[N])

 ASSERT(pointer != nullptr); // Out of virtual memory?
 memcpy(ptr, guard, N);
 return reinterpret_cast<char*>(ptr) + N;


template<size_t N>
inline void *guardDelete(void *ptr, const char (&guard)[N])

 if (ptr == nullptr) 
 return ptr;
 else 
 char *prefix = reinterpret_cast<char*>(ptr) - N;
 ASSERT(memcmp(prefix, guard, N) == 0);
 return prefix;
 


void * __cdecl operator new(size_t size) 
 return guardNew(malloc(size + 8), "str1med");

void * __cdecl operator new(size_t size) 
 return guardNew(malloc(size + 8), "STR2MED");

void __cdecl operator delete(void *ptr) 
 free(guardDelete(ptr, "str1med"));

void __cdecl operator delete(void *ptr) 
 free(guardDelete(ptr, "STR2MED"));

However, then I start worrying... is this code even doing a sensible thing? Notice that if I try to new an object of some type T where alignof(T) > 8, then the pointer returned from our operator new will not be properly aligned for that T. And the fact that it's properly aligned for alignments <= 8 feels like an accident. If we changed our guard strings from str1med and STR2MED (not obvious why we picked those strings, by the way — and again the unfortunate mix of lowercase and UPPERCASE) to, let's say, hello and world, this code would break in several different ways. So maybe we want to force the compiler to diagnose that breakage for us:

inline void *guardNew(void *pointer, const char (&guard)[8])

 ASSERT(pointer != nullptr); // Out of virtual memory?
 memcpy(ptr, guard, 8);
 return reinterpret_cast<char*>(ptr) + 8;

Now if we change the string in operator new from str1med to hello, we'll get a compiler error.

But we've still got two instances of the magic number 8 separated by a fair number of lines of code. Let's inline this guardNew helper so that we're malloc'ing and memcpy'ing in the same place:

#include <stdlib.h>
#include <string.h>

void * __cdecl operator new(size_t size) 
 void *p = malloc(size + 8);
 ASSERT(p != nullptr); // Out of memory?
 memcpy(p, "str1med", 8);
 return (char *)p + 8;

void * __cdecl operator new(size_t size) 
 void *p = malloc(size + 8);
 ASSERT(p != nullptr); // Out of memory?
 memcpy(p, "STR2MED", 8);
 return (char *)p + 8;

void __cdecl operator delete(void *ptr) 
 if (ptr == nullptr) return;
 char *p = (char*)ptr - 8;
 ASSERT(memcmp(p, "str1med", 8) == 0);
 free(p);

void __cdecl operator delete(void *ptr) 
 if (ptr == nullptr) return;
 char *p = (char*)ptr - 8;
 ASSERT(memcmp(p, "STR2MED", 8) == 0);
 free(p);

From 36 lines down to 27 — and with no pollution of the global namespace, no templates, and — mainly as a result of flattening and inlining things with complicated dependencies on each other — it's just a little bit harder to break accidentally.

From @Quuxplusone 's answer I have taken the following advice:

• include all used headers, do not rely on headers including others


• remove OFFSET_ constants and instead pass the PREFIX_ constants correctly


• rely on std::size_t instead of unnecessarily inferring the size type (std::size_t is itself an inferred type)


• use sizeof instead of std::size (saves one header)


• consistently use two different naming conventions between constants and variables


• reorder the functions so the operator replacements and business logic implementations are grouped together each

Further improvements based on testing, comments by @MartinYork and answers and comments to this question are:

• pull the calls to std::malloc and std::free into the functions and rename those accordingly (a bit more DRYness for a bit less SRP... mayhap a matter of taste)


• check at delete whether the passed pointer really is offset by the expected length (turns out that e.g. file handles are not necessarily)


• 
  static_assert that the different prefixes have the same length so aforementioned offset check will always work


• compute the offset in such a way that it does not break alignment (this inspired the linked question)


• use std::byte * as pointer type to express that (except for the prefix) the pointer does not at all need to contain printable data


• as per C++20, annotate the replaced delete operators with [[nodiscard]]
  

Cumulating in this revised implementation:

#include <cstddef>
#include <cstdint>
#include <cstdlib>
#include <cstring>

constexpr char PREFIX_ARRAY = "STR2MED";
constexpr char PREFIX_OBJECT = "str1med";
static_assert(sizeof(PREFIX_ARRAY) == sizeof(PREFIX_OBJECT),
 "Size of array and object prefixes must be equal to guarantee that a check always happens.");

constexpr std::size_t offset(std::size_t prefixSize, std::size_t alignment = alignof(std::max_align_t)) noexcept
 return alignment + alignment * (prefixSize / alignment); 

template<std::size_t PREFIX_SIZE>
inline void guardedDelete(void * untypedPointer, const char (& prefix)[PREFIX_SIZE])

 constexpr std::size_t OFFSET = offset(PREFIX_SIZE);
 std::byte * pointer = reinterpret_cast<std::byte*>(untypedPointer);

 if(OFFSET < reinterpret_cast<std::uintptr_t>(untypedPointer)) // Otherwise it's not one of ours. Yes, that *does* happen!
 // Currently calling std::filesystem::current_path() triggers an example.
 pointer -= OFFSET; // -Zsar 2018-02-08
 ASSERT(std::memcmp(pointer, prefix, PREFIX_SIZE) == 0);
 

 std::free(pointer);


template<std::size_t PREFIX_SIZE>
inline void * guardedNew(std::size_t size, const char (& prefix)[PREFIX_SIZE])

 constexpr std::size_t OFFSET = offset(PREFIX_SIZE);
 void * pointer = std::malloc(size + OFFSET);

 ASSERT(pointer != nullptr); // Out of virtual memory?
 std::memcpy(pointer, prefix, PREFIX_SIZE);

 return reinterpret_cast<std::byte*>(pointer) + OFFSET;


 void __cdecl operator delete (void * pointer) guardedDelete(pointer, PREFIX_OBJECT); 
 void __cdecl operator delete(void * pointer) guardedDelete(pointer, PREFIX_ARRAY); 
[[nodiscard]] void * __cdecl operator new (std::size_t size) return guardedNew(size, PREFIX_OBJECT); 
[[nodiscard]] void * __cdecl operator new(std::size_t size) return guardedNew(size, PREFIX_ARRAY);