903420 - jemalloc's stats_chunks lacks proper locking

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

STR:
- from tip 140824:05d3797276d3, apply patch attachment 787002 [details] [diff] [review]
- ./mach build toolkit
- ./mach xpcshell-test toolkit/components/telemetry/unit/test_TelemetryPing.js

Comment 1

[Justin Lebar (not reading bugmail)]

I just tested and as expected, this reproduces even without the patch from comment 0.  You're off the hook, Yoric; this is all me.  :)

Comment 2

[Justin Lebar (not reading bugmail)]

Interestingly this does not reproduce in a debugger.

Comment 3

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Huh, this was broken before we changed it, too.  Thanks for finding this, Yoric!

Comment 4

[Justin Lebar (not reading bugmail)]

Actually, I am not so sure this is right...

Comment 5

[Justin Lebar (not reading bugmail)]

Comment on attachment 788369 [details] [diff] [review]
Patch, v1

Yeah, I was mislead by the comments.

There are comments which say that stats_chunks.curchunks is for large allocs only, but those are wrong afaict.

Comment 6

[Justin Lebar (not reading bugmail)]

There are actually two race conditions here; one in jemalloc_stats, and one outside it.

chunk_alloc doesn't take any locks, and it reads/writes stats_chunks.curchunks.

chunk_alloc is called by both huge_alloc, which holds the huge lock, and arena_run_alloc, which holds the arena lock.

So that's busted.

But then also, it's possible for allocations to happen during a call to jemalloc_stats, and we won't handle that correctly, either.

We need to fix the race in huge_alloc, but we may end up just taking out this assertion in jemalloc_stats; that one doesn't seem very problematic.  And who knows whether these races are what's causing the test failure here; it happens so consistently, I have difficulty believing it's the case, but who knows.

Comment 7

[Justin Lebar (not reading bugmail)]

Whoa, and if we have MALLOC_VALIDATE defined, we touch an rbtree without holding the right locks.

Comment 8

[Justin Lebar (not reading bugmail)]

Wow, this keeps getting worse and worse.

MALLOC_VALIDATE is defined, but even worse, we don't hold the huge mutex while running chunk_alloc, which means that this rbtree is only synchronized when two allocs from the same arena race.  (We only have one arena, so this happens when two non-huge allocs race.)

Comment 9

[Justin Lebar (not reading bugmail)]

I don't see any reason why this isn't sg:crit, save that it might be hard to pull off because you're relying on threading effects.

Comment 10

[Daniel Veditz [:dveditz]]

so let's call it sec-high

Comment 11

[Justin Lebar (not reading bugmail)]

Renaming this bug to cover the sec-sensitive race condition I found.  I'll see if it makes sense to fix the stats race while I'm at it.  Fixing the stats race may or may not fix the assertion that this bug was originally filed about.

One option we have here is to disable MALLOC_VALIDATE.  I'll need to look into what the consequences of doing that will be if we pass in an invalid pointer.  It may be the easiest, safest thing we can do; fixing this properly will require shuffling around and potentially acquiring more locks, which may have an effect on jemalloc's performance.

Comment 12

[Justin Lebar (not reading bugmail)]

Oh, thank goodnesss, I'm wrong about the s-s race here.

malloc_rtree_set, which is the function called from chunk_alloc, does in fact acquire a lock; it's just hiding.

The stats are still racy afaict, but that isn't security-sensitive.

Comment 13

[Justin Lebar (not reading bugmail)]

Sorry for the false alarm, everyone.  I'm pretty confident we can safely unprotect this.

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v2

This patch makes two fixes to jemalloc's memory reporters.

1) We were counting "dirty" pages in "waste", when we shouldn't have
   been.  This was causing the assertion at the end of jemalloc_stats
   which checks that mapped memory is greater than committed memory to
   fail.

2) Previously jemalloc_stats used stats_chunks.curchunks to measure the
   number of mapped pages.  This was problematic for two reasons.

   a) stats_chunks.curchunks was not locked when it was modified in
      chunk_{de}alloc(), so it could be garbage.

   b) Even if it had been locked properly, it would have been possible
      for an allocation to occur during a call to jemalloc_stats which
      would cause the measured amount of allocated memory to exceed the
      measured amount of mapped memory.

   We fixed these issues by deleting stats_chunks entirely, and by
   introducing huge_mapped, which measures the amount of memory mapped
   by huge allocations (and is properly protected by huge_mtx).

   We now measure the amount of mapped memory by adding huge_mapped and
   each arena's mapped memory, and we do this in such a way that even if
   an allocation occurs during our call to jemalloc_stats, we'll still
   get a consistent result (where mapped >= committed).

Comment 15

[Justin Lebar (not reading bugmail)]

If for some reason you want to keep stats_chunks, it is possible to touch it from inside the function which modifies the chunk rbtree (which already acquires its own locks).  So we could keep it without any additional overhead, so long as MALLOC_VALIDATE is defined.  But I think the approach here is cleaner.

Comment 16

[Justin Lebar (not reading bugmail)]

Note also that currently stats_chunks.nchunks isn't even written to except on Windows.

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 790453 [details] [diff] [review]
Patch, v2

Review of attachment 790453 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 18

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1152b6380d

Comment 19

[Justin Lebar (not reading bugmail)]

Note that this will affect explicit (I think it should decrease a bit).

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b1152b6380d