This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Handle malloc_destroy_zone() on Darwin
ClosedPublic

Authored by kubamracek on Nov 23 2016, 5:41 PM.

Details

Summary

We currently have a interceptor for malloc_create_zone, which returns a new zone that redirects all the zone requests to our sanitizer zone. However, calling malloc_destroy_zone on that zone will cause libmalloc to print out some warning messages, because the zone is not registered in the list of zones. This patch handles this and adds a testcase for that.

Secondly, in certain OS versions, it was possible that libmalloc replaced the sanitizer zone from being the default zone (i.e. being in malloc_zones[0]). This patch also introduces a failsafe that makes sure we always stay the default zone. No testcase for this, because this doesn't reproduce under normal circumstances.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 79179.Nov 23 2016, 5:41 PM
kubamracek retitled this revision from to [sanitizer] Handle malloc_destroy_zone() on Darwin.
kubamracek updated this object.
kubamracek set the repository for this revision to rL LLVM.
kubamracek added a project: Restricted Project.
kubamracek added a subscriber: llvm-commits.
filcab edited edge metadata.Nov 29 2016, 9:29 AM

Please split this in two commits: The malloc_zone_destroy part and the bugfix for malloc_zones[0]. They seem orthogonal.

I'd really like a test for the malloc_zones[0] problem, though. Is it not possible at all? Does it happen only on certain versions (as in: maybe it was a bug somewhere else)?
Otherwise LGTM, but you might want to check if anyone that's using ASan on the mac on more complex systems (Google Chrome?) have something to say.

lib/sanitizer_common/sanitizer_malloc_mac.inc
78

Why not this (seems more explicit to me. Not a big deal, though)?

malloc_zones[i] = malloc_zones[0];
malloc_zones[0] = &sanitizer_zone;
kubamracek edited edge metadata.
kubamracek removed rL LLVM as the repository for this revision.

Updating patch.

Please split this in two commits: The malloc_zone_destroy part and the bugfix for malloc_zones[0]. They seem orthogonal.

Ok, I'll commit this separately.

I'd really like a test for the malloc_zones[0] problem, though. Is it not possible at all? Does it happen only on certain versions (as in: maybe it was a bug somewhere else)?
you might want to check if anyone that's using ASan on the mac on more complex systems (Google Chrome?) have something to say.

I'm afraid I can't come up with a test for this. This was a "bug" (or rather an incompatible change) in some versions of libmalloc on macOS, where in certain cases this mprotect() interceptor was needed; but I believe the problem was fixed before ending in any public release. We have been using this patch internally for a long time now, so it should be safe.

zaks.anna added inline comments.Dec 1 2016, 3:54 PM
lib/sanitizer_common/sanitizer_malloc_mac.inc
68

I'd add a comment that explains the intent here (why are we doing this), ex:
// Ensure that the sanitizer_zone is registered as malloc_zones[0].

74

You could strengthen the condition above to malloc_num_zones > 1 && and start iteration from '1'. If there is a single zone:

  • if it is sanitizer_zone, we are good to go.
  • if it is not a sanitizer_zone, there is nothing to swap it with.
kubamracek updated this revision to Diff 80293.Dec 5 2016, 10:45 AM

Updating patch, addressing review comments.

zaks.anna added inline comments.Dec 5 2016, 10:54 AM
lib/sanitizer_common/sanitizer_malloc_mac.inc
75

Iteration should start with 1 since the condition will always be false for i==0. The current version is correct, but was a bit more complicated to reason about since the intention is never to malloc_zones[0] with malloc_zones[0].

kubamracek updated this revision to Diff 80296.Dec 5 2016, 10:56 AM
zaks.anna accepted this revision.Dec 6 2016, 3:59 PM
zaks.anna edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Dec 6 2016, 3:59 PM

Landed in r289375 and r289376.

This revision was automatically updated to reflect the committed changes.