Remove redundant null pointer check in operator delete

[MaskRay]

Summary

C89 4.10.3.2 The free function
C99 7.20.3.2 The free function
C11 7.22.3.3 The free function

If ptr is a null pointer, no action shall occur.

_aligned_free on MSDN:

If memblock is a NULL pointer, this function simply performs no actions.

[MaskRay]

.

[ldionne]

Was this true pre-C11 too? If not, then this needs to be guarded by #if _LIBCPP_STD_VER >= 17, because C++ is only on top of C11 in C++17 and above (Marshall can double-check this).

[MaskRay]

Also remove the check for _aligned_free

[MaskRay]

Quote C89: "If ptr is a null pointer, no action occurs."

[MaskRay]

In D52401#1243054, @ldionne wrote:

Was this true pre-C11 too? If not, then this needs to be guarded by #if _LIBCPP_STD_VER >= 17, because C++ is only on top of C11 in C++17 and above (Marshall can double-check this).

Thanks for the note. It is true in C89 (http://port70.net/~nsz/c/c89/c89-draft.html).
"If ptr is a null pointer, no action occurs." Also similar wording in POSIX "If ptr is a null pointer, no action shall occur."

[MaskRay]

Ping :)

[khng300]

LGTM

[hotpxl]

LGTM

[ldionne]

You did not get a thumbs up from any of the code owners for libc++. Reverted in r342938.

[lichray]

LGTM as well, unless @mclow.lists can tell us some history like interactions with K&R libc :)

[mclow.lists]

I suspect it's fine, but I need to check some stuff on old versions of glibc (I seem to recall a problem with that).

[MaskRay]

I just checked an extremely old version of glibc fetched from https://ftp.gnu.org/pub/gnu/glibc/

glibc-2.0.1.tar.gz 1997-02-04 03:00 3.7M

malloc/malloc.c

#if __STD_C
void fREe(Void_t* mem)
#else
void fREe(mem) Void_t* mem;
#endif
{
  arena *ar_ptr;
  mchunkptr p;                          /* chunk corresponding to mem */

#if defined(_LIBC) || defined(MALLOC_HOOKS)
  if (__free_hook != NULL) {
    (*__free_hook)(mem);
    return;
  }
#endif

  if (mem == 0)                              /* free(0) has no effect */
    return;

__free_hook (defaults to NULL) is a user-supplied hook (https://www.gnu.org/software/libc/manual/html_node/Hooks-for-Malloc.html). If this failed for operator delete, it would mean various other free(NULL) would also fail.

[MaskRay]

I seem to recall a problem with that

I would like to know if your impression came from the common PWN technique when the attacker found a heap buffer overflow :)

[lichray]

In D52401#1250551, @MaskRay wrote:

__free_hook (defaults to NULL) is a user-supplied hook (https://www.gnu.org/software/libc/manual/html_node/Hooks-for-Malloc.html).

Very interesting, that means if we don't apply this patch, we essentially breaks glic __free_hook, because that one expects to be able to observe null pointers.

[mclow.lists]

In D52401#1250552, @MaskRay wrote:

I seem to recall a problem with that

I would like to know if your impression came from the common PWN technique when the attacker found a heap buffer overflow :)

No; that's not what I'm looking into.

[mclow.lists]

Ok. I'm fine with this. Thanks for your patience.

libcxx/trunk/src/new.cpp

 #endif  // _LIBCPP_NO_EXCEPTIONS
     return p;
 }
 
 _LIBCPP_WEAK
 void
 operator delete(void* ptr) _NOEXCEPT
 {
-    if (ptr)
-        ::free(ptr);
 }
 
 _LIBCPP_WEAK
 void
 operator delete(void* ptr, const std::nothrow_t&) _NOEXCEPT
 {
     ::operator delete(ptr);
 }

 #endif  // _LIBCPP_NO_EXCEPTIONS
     return p;
 }
 
 _LIBCPP_WEAK
 void
 operator delete(void* ptr, std::align_val_t) _NOEXCEPT
 {
-    if (ptr)
 #if defined(_LIBCPP_MSVCRT_LIKE)
-        ::_aligned_free(ptr);
 #else
-        ::free(ptr);
 #endif
 }
 
 _LIBCPP_WEAK
 void
 operator delete(void* ptr, std::align_val_t alignment, const std::nothrow_t&) _NOEXCEPT
 {
     ::operator delete(ptr, alignment);