Feature request: detect malloc(0) dereference

[guidovranken]

malloc(0) and then dereferencing the acquired pointer is undefined behavior, and I believe that the OpenBSD libc deliberately segfaults in this case.

#include <stdlib.h>
int main(void)
{
    char* p = malloc(0);
    *p = 0;
    free(p);
    return 0;
}

It would be nice if ASAN would return an invalid non-null pointer (eg. 0x00000012) that is guaranteed to crash if dereferenced.
It would help find a class of bugs that might be rare but if present could lead to denial-of-service situations or worse in public facing software.
(Sorry if this has been brought up before)

[kcc]

We've tried that before, (to return NULL on malloc(0), not to return 0x00000012 though) and it caused lots of incompatibilities.

Right now, malloc(0) is effectively equivalent to malloc(1) and we don't catch this bug:

static volatile int zero = 0;
char *p = new char[zero];
int main() {
  return p[0];
}

I don't think the standard allows us to return a non-null but fixed address for all instances of malloc(0):

What we can do, is to still allocate a valid pointer but poison all the memory.
Something like this, but will need to add tests and ensure that the bug reports are not confusing:

--- lib/asan/asan_allocator.cc  (revision 354938)
+++ lib/asan/asan_allocator.cc  (working copy)
@@ -411,6 +411,7 @@
         ComputeUserRequestedAlignmentLog(alignment);
     if (alignment < min_alignment)
       alignment = min_alignment;
+    bool orig_size_is_zero = size == 0;
     if (size == 0) {
       // We'd be happy to avoid allocating memory for zero-size requests, but
       // some programs/tests depend on this behavior and assume that malloc
@@ -520,6 +521,9 @@
       *shadow = fl.poison_partial ? (size & (SHADOW_GRANULARITY - 1)) : 0;
     }
 
+    if (orig_size_is_zero)
+      *(u8 *)MemToShadow(user_beg) = kAsanHeapLeftRedzoneMagic;
+
     AsanStats &thread_stats = GetCurrentThreadStats();
     thread_stats.mallocs++;
     thread_stats.malloced += size;

[dvyukov]

We also could:

void *malloc(size_t size) {
  if (size == 0) {
    static long long z = 0x5555555555555555;
    return (void*)__atomic_fetch_add_n(&z, 1, __ATOMIC_RELAXED);
  }
  ...
}

void free(void *p) {
  if ((long)p >= 0x5555555555555555 && (long)p < 0x6666666666666666)
    return;
  ...
}

But I don't know if it's better... probably not... no stacks, free(p + 100) is not detected.

[guidovranken]

We've tried that before, (to return NULL on malloc(0), not to return 0x00000012 though) and it caused lots of incompatibilities.

In my experience, returning NULL from malloc activates code paths that in most software have literally never been tested, leading to all kinds of unexpected behavior.
Returning an invalid pointer (or using the poison option) for malloc(0) on the other hand will probably only rarely lead to a crash, and if it does, it is a legitimate bug worth fixing.
Thanks for considering to implement this!