This is an archive of the discontinued LLVM Phabricator instance.

Remove redundant null pointer check in operator delete
ClosedPublic

Authored by MaskRay on Sep 22 2018, 10:17 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Sep 22 2018, 10:17 PM
MaskRay updated this revision to Diff 166625.Sep 22 2018, 10:21 PM
MaskRay edited the summary of this revision. (Show Details)

.

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 updated this revision to Diff 166629.Sep 23 2018, 12:45 AM
MaskRay edited the summary of this revision. (Show Details)

Also remove the check for _aligned_free

MaskRay updated this revision to Diff 166632.Sep 23 2018, 1:29 AM
MaskRay edited the summary of this revision. (Show Details)

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

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."

khng300 accepted this revision.Sep 24 2018, 7:45 PM
khng300 added a subscriber: khng300.

LGTM

This revision is now accepted and ready to land.Sep 24 2018, 7:45 PM
hotpxl accepted this revision.Sep 24 2018, 7:45 PM
hotpxl added a subscriber: hotpxl.

LGTM

This revision was automatically updated to reflect the committed changes.

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

lichray reopened this revision.Sep 28 2018, 9:49 PM
lichray added a subscriber: lichray.

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

This revision is now accepted and ready to land.Sep 28 2018, 9:49 PM

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).

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.

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 :)

__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.

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 accepted this revision.Oct 1 2018, 9:12 AM

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

MaskRay edited the summary of this revision. (Show Details)Oct 1 2018, 10:12 AM
This revision was automatically updated to reflect the committed changes.