5
\$\begingroup\$

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|MB_DEFBUTTON3|MB_TOPMOST))
  {
    case IDABORT:
      std::abort();
      break;
    case IDRETRY:
      DebugBreak();
      break;
  }
}

#define ASSERT(f) \
  if (f)          \
  {}              \
  else            \
    _AssertDebug((#f), __FILE__, __LINE__)
\$\endgroup\$
  • 1
    \$\begingroup\$ Returned is the malloc'ed pointer offset by 1 Offset by 1 what? One Byte is probably wrong. One unit the size of the object is probably OK (but probably excessive). By one maximum aligned unit (std::max_align_t) is probably the minimum without doing some fancy maths. \$\endgroup\$ – Martin York Feb 4 '18 at 18:53
  • \$\begingroup\$ @MartinYork : Oops, forgot to adjust from first draft. Corrected. \$\endgroup\$ – Zsar Feb 4 '18 at 19:32
  • \$\begingroup\$ IIRC, Valgrind will normally spot a mismatched delete operator (not sure if that's true with an allocation size of 1 object). But it can be very time-consuming to run a full program, so your code still has value against it. \$\endgroup\$ – Toby Speight Feb 5 '18 at 16:42
  • \$\begingroup\$ @MartinYork : I have pondered your note on the offset for a bit and find that I am in need of a bit of a pointer (hè). I inherited the current way of just offsetting by the length of the prefix from the old code (in fact, the old code used the same strings but with an additional '0' character at the end - so an offset of 9, not even some 2^x). I only have anecdotical evidence that this seems to have worked since at least VS2010 and still does. If you could point me towards further reading on the theoretical dangers of this approach, I'd be oblieged. \$\endgroup\$ – Zsar Feb 7 '18 at 22:08
  • \$\begingroup\$ Here is a link to the standard. open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4659.pdf See: 6.11 Alignment [basic.align] Object types have alignment requirements (6.9.1, 6.9.2) which place restrictions on the addresses at which an object of that type may be allocated. \$\endgroup\$ – Martin York Feb 7 '18 at 23:26
4
\$\begingroup\$
#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.

\$\endgroup\$
  • \$\begingroup\$ A slight nit, since you say you write size_t instead of std::size_t. That perhaps works for you on your platform, which evidently exercises its right to also declare std names into the global namespace, but that is not portable unless you include the (deprecated) <*.h> versions of the C Standard Library rather than the (modern) <c*> versions. \$\endgroup\$ – Toby Speight Feb 5 '18 at 16:46
  • \$\begingroup\$ @TobySpeight: Notice that I did include the <*.h> versions in my rewrite (since all I needed was the libc stuff, not the namespaces and whatnot that <c*> would drag in). However, I'm also curious: can you name a platform where #include <cstddef> ... size_t ... doesn't work? (If so, awesome, I'll update my advice in the future to include that caveat. If not, then you were misusing the word "portable"; you meant something more like "standard-conforming".) \$\endgroup\$ – Quuxplusone Feb 5 '18 at 20:36
  • 1
    \$\begingroup\$ Ah, I didn't spot that because you hadn't mentioned the change to the old-fashioned compatibility headers before mentioning unqualified size_t. I'm surprised that you prefer the headers that don't keep their names tidily in the std namespace, as I always recommend the <c*> headers for that reason. I can't name any platforms that don't take advantage of the leeway given to also populate the global namespace (I don't use many different current compilers, and absolutely no future compilers yet), but I'd say it's more likely for C++ compilers or libraries that don't also support C. \$\endgroup\$ – Toby Speight Feb 6 '18 at 8:51
  • \$\begingroup\$ Mmh. You do have a couple of good points that I'll adapt (though not in this question, if I understood the meta correctly?) but overall I am not convinced. Less dry and contains magic numbers. Also relies on wrong-language headers, while the C and C++ standards continue to drift apart. Also I am not sure why to prefer raw pointer over array parameter to a template - while within the body the latter will have decayed to the former, for signature matching, the latter will be a better match than the former aka it's a true difference - that is how std::size works for arrays, after all. \$\endgroup\$ – Zsar Feb 7 '18 at 22:01
  • \$\begingroup\$ Ah, I made a mistake with the array parameter. It ought to have been const char (& PREFIX)[OFFSET] - then one can (and should) indeed auto-deduce the OFFSET. Shows how scatterbrained I am - only just now did I start wondering why I had put it there in the first place. \$\endgroup\$ – Zsar Feb 7 '18 at 22:23
0
\$\begingroup\$

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); }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.