Overriding operator delete/new to check correct pairs of (non-)array variants 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:


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.)
Did I avoid UB at least in the non-pathological use cases? (And even if, examples of such cases are welcome.)
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 &lt;cstring&gt;

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&lt;OFFSET_TYPE OFFSET&gt;
inline void * guardDelete(void * pointer, const char PREFIX[OFFSET])
{
  if(pointer == nullptr)
    return pointer;
  else
  {
    char * prefix = reinterpret_cast&lt;char*&gt;(pointer) - OFFSET;
    ASSERT(std::memcmp(prefix, PREFIX, OFFSET) == 0);

    return prefix;
  }
}

void __cdecl operator delete  (void * pointer) { std::free(guardDelete&lt;OFFSET_OBJECT&gt;(pointer, PREFIX_OBJECT)); }
void __cdecl operator delete[](void * pointer) { std::free(guardDelete&lt;OFFSET_ARRAY&gt; (pointer, PREFIX_ARRAY)); }

template&lt;OFFSET_TYPE OFFSET&gt;
inline void * guardNew(void * pointer, const char PREFIX[OFFSET])
{
  ASSERT(pointer != nullptr); // Out of virtual memory?
  std::memcpy(pointer, PREFIX, OFFSET);

  return reinterpret_cast&lt;char*&gt;(pointer) + OFFSET;
}

void * __cdecl operator new  (std::size_t size) { return guardNew&lt;OFFSET_OBJECT&gt;(std::malloc(size + OFFSET_OBJECT), PREFIX_OBJECT); }
void * __cdecl operator new[](std::size_t size) { return guardNew&lt;OFFSET_ARRAY&gt; (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|MB_DEFBUTTON3|MB_TOPMOST))
  {
    case IDABORT:
      std::abort();
      break;
    case IDRETRY:
      DebugBreak();
      break;
  }
}

#define ASSERT(f) \
  if (f)          \
  {}              \
  else            \
    _AssertDebug((#f), __FILE__, __LINE__)
#include &lt;cstring&gt;


Also include &lt;cstdlib&gt; (for std::malloc), and I'd include &lt;new&gt; 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 &lt;array&gt;, &lt;deque&gt;, &lt;forward_list&gt;, &lt;iterator&gt;, &lt;list&gt;, &lt;map&gt;, &lt;regex&gt;, &lt;set&gt;, &lt;string&gt;, &lt;unordered_map&gt;, &lt;unordered_set&gt;, &lt;vector&gt;.

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&lt;OFFSET_TYPE OFFSET&gt;
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&lt;size_t Offset&gt;
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 &lt;string.h&gt;
#include &lt;stdlib.h&gt;
#include &lt;new&gt; // maybe

template&lt;size_t N&gt;
inline void *guardNew(void *pointer, const char (&amp;guard)[N])
{
    ASSERT(pointer != nullptr); // Out of virtual memory?
    memcpy(ptr, guard, N);
    return reinterpret_cast&lt;char*&gt;(ptr) + N;
}

template&lt;size_t N&gt;
inline void *guardDelete(void *ptr, const char (&amp;guard)[N])
{
    if (ptr == nullptr) {
        return ptr;
    } else {
        char *prefix = reinterpret_cast&lt;char*&gt;(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) &gt; 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 &lt;= 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 (&amp;guard)[8])
{
    ASSERT(pointer != nullptr); // Out of virtual memory?
    memcpy(ptr, guard, 8);
    return reinterpret_cast&lt;char*&gt;(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 &lt;stdlib.h&gt;
#include &lt;string.h&gt;

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.