[RFC] Fix abort in malloc extent coalescing.
ClosedPublic
Actions

Authored by dgr_semihalf.com on Oct 30 2020, 6:58 PM.

Details

Reviewers

mw
kib
emaste
jasone

Commits

rS368756: Fix abort in jemalloc extent coalescing.

Summary

Fix error in extent_try_coalesce_impl(), which could cause abort
to happen when trying to coalesce extents backwards. The error could
happen because of how extent_before_get() function works. This function
gets address of previous extent, by subtracting page size from current
extent address. If current extent is located at PAGE_SIZE offset, this
address resolved to 0x0000. An assertion in rtree_leaf_elm_lookup
then caused the running program to abort.

This problem was discovered when trying to build world on 32-bit
machines with ASLR and PIE enabled. The problem was present
on armv7 and i386 machines, however other 32-bit machines were
not tested, and the same problem is most likely seen there.
While this fixes one problem with buildworld on 32-bit platforms
with ASLR, the build still fails, however it happens much later
and due to lack of memory.

No such problem was observed on 64-bit platforms, however it seems
that it could also happen there, but the probability of such abort
happening is much lower due to significantly larger address space.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

dgr_semihalf.com requested review of this revision.Oct 30 2020, 6:58 PM

dgr_semihalf.com created this revision.

I think this change is fine as is.

But isn't there a further problem then ? If sysctl security.bsd.map_at_zero is true, then in principle mmap could create a mapping at address zero, on malloc request. Then wouldn't we get a similar issue ?

Does it make sense also report to the upstream? https://github.com/jemalloc/jemalloc

In D27025#603548, @lwhsu wrote:

Does it make sense also report to the upstream? https://github.com/jemalloc/jemalloc

I opened a Pull Request in upstream repository: https://github.com/jemalloc/jemalloc/pull/1973

In D27025#602992, @kib wrote:

I think this change is fine as is.

But isn't there a further problem then ? If sysctl security.bsd.map_at_zero is true, then in principle mmap could create a mapping at address zero, on malloc request. Then wouldn't we get a similar issue ?

Yes, in this case the assertion would fail anyway. However, allowing mapping at address zero is not a typical scenario and there are more NULL checks that will fail when mmap returns zero.

Hi,

In jemalloc upstream repo based on this patch another PR was merged, that applies on top of tree (https://github.com/jemalloc/jemalloc/pull/2003).

What we have in contrib/jemalloc is not aligned to jemalloc upstream and IMO we should merge this commit as-is. Any objections @kib, @lwhsu ?

Best regards,
Marcin

mw mentioned this in D27666: Enable ASLR by default for 64-bit executables..Dec 18 2020, 4:51 AM

I do not have objections.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 18 2020, 10:09 AM

Closed by commit rS368756: Fix abort in jemalloc extent coalescing. (authored by mw). · Explain Why

This revision was automatically updated to reflect the committed changes.

mw added a commit: rS368756: Fix abort in jemalloc extent coalescing..

Herald added a subscriber: imp. · View Herald TranscriptDec 18 2020, 10:09 AM