1378658 - Allocating a huge buffer can return a previously freed huge buffer with its contents unaltered

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Mike Hommey [:glandium]]

Consider the following source:

  #include <stdio.h>
  #include <stdlib.h>

  static const size_t size = 1024 * 1024;

  int main() {
    char* ptr = (char*) malloc(size);
    for (size_t i = 0; i < size; i++)
      ptr[i] = (char)i;
    free(ptr);
    char *ptr2 = (char*) malloc(size);
    if (ptr != ptr2) {
      fprintf(stderr, "didn't get the same pointer.\n");
      return 1;
    }
    for (size_t i = 0; i < size; i++)
      if (ptr[i] != (char)i)
        return 0;
    return 1;
  }

If you run this with mozjemalloc, it turns out that what you get from the second malloc is the same pointer (which is expected), but the contents that were stored first are still there. This only affects OSX (more specifically, non-Linux POSIX OSes, so BSDs are affected too, but most disable mozjemalloc afaik). On Linux and Windows, the returned buffer is zeroed out.

For sizes smaller than 1MiB - 8KiB, the second alloc gets poisoned data (with 0xe5).

Note: because of how mozjemalloc is hooked up on OSX, you can just compile the source above normally, and run the resulting executable with DYLD_INSERT_LIBRARIES=/Applications/Firefox.app/Contents/MacOS/libmozglue.dylib set (assuming you have a Firefox install in /Applications), and you'll have it use mozjemalloc.

What's happening is this:
- Allocating "huge" buffers calls chunk_alloc()
- The first time we allocate a "huge" buffer, there is no recycled chunk, so we get a fresh chunk.
- The test program fills the buffer.
- When the buffer is freed, the chunk is "purged" and placed in a recycle queue. On Linux, "purged" means madvise(MADV_DONTNEED), which actively zeroes the memory. On Windows, it means it's decommitted (which makes accesses to it a hard error). On OSX, it means madvise(MADV_FREE), which keeps the content of the memory, but drops it in case of memory pressure.
- When we allocate a new "huge" buffer, we get a chunk from the recycle queue. Because the test case is simple, we just get the one that was just freed. On Linux, we get an already zeroed buffer. On Windows, mozjemalloc recommits the memory, which zeroes it out, so we also get a zeroed buffer. On OSX, we're just handed out the buffer straight out of the recycle queue, so it still contains the data.

FWIW, this recycling feature comes from bug 1073662.

Several notes:
- If the chunk had been used for arena data before being recycled, we'd get a valid arena chunk header (arena_chunk_t), with empty runs all filled with either zeroes or poison bytes.
- The more the recycle queue is filled up, the less likely it is to get the most recently released chunks. Essentially, those with smallest addresses are going to be given back first (as long as there are enough contiguous chunks for the requested amount of memory, if it's more than one chunk worth). AFAICT, OSX tends to give mappings with increasing addresses, so in practice we get the oldest chunks out of the recycle queue.
- Technically, the recycle queue is not a queue, but a R-B tree, "indexed" by size, and adjacent chunks are folded together.
- An attack is probably hard to pull off because of the above, combined with the fact that I don't think there are JS controlled types that will allow to read such a buffer. (and e.g. ArrayBuffers are not allocated through jemalloc)
- I'm not sure it's worth keeping hidden, but better safe than sorry.

Comment 1

[Mike Hommey [:glandium]]

Forgot to add: I found this while working on a fix for bug 1360772, which will now depend on this bug being fixed.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Only recycle arena chunks when page_purges doesn't zero pages

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8883838 [details] [diff] [review]
Only recycle arena chunks when page_purges doesn't zero pages

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

Thanks for the detailed description.

::: memory/mozjemalloc/mozjemalloc.cpp
@@ +796,5 @@
>  };
>  
> +enum ChunkType {
> +  UNKNOWN_CHUNK,
> +  BASE_CHUNK,

BASE_CHUNK is unused.

@@ +800,5 @@
> +  BASE_CHUNK,
> +  ARENA_CHUNK,
> +  HUGE_CHUNK,
> +  RECYCLED_CHUNK,
> +};

Add some brief comments about the different kinds of chunks?

@@ +2127,5 @@
> +	/* If purge doesn't zero the chunk, only keep arena chunks and
> +	 * previously recycled chunks for recycling. */
> +	if (unzeroed && type != ARENA_CHUNK && type != RECYCLED_CHUNK) {
> +		return;
> +	}

I'm having a hard time convincing myself this condition is correct. Does returning early mean we don't recycle the chunk?

I think it can be rewritten like this:

> if (unzeroed && (type == UNKNOWN_CHUNK || type == HUGE_CHUNK)) {
>   return;
> }	

which makes it seems like RECYCLED and ARENA chunks *will* get recycled when they haven't been zeroed. Which would be backwards? I'm confused.

I'm also confused about the commit message. Don't we want to recycle chunks when the pages *have* been zeroed?

Comment 4

[Mike Hommey [:glandium]]

Yeah, re-reading the commit message, I can see how it's confusing. The idea is that, when the pages have not been zeroed, we only want to recycle the arena chunks (or chunks that were already previously recycled).

Comment 5

[Mike Hommey [:glandium]]

Attachment: When page_purges doesn't zero pages, only record arena chunks for future recycling

Is it clearer?

Comment 6

[Daniel Veditz [:dveditz]]

Sounds pretty "internal", but calling it sec-moderate because it could be combined with some other bug that leaks/uses uninitialized memory to turn into something worse.

Comment 7

[Mike Hommey [:glandium]]

Do you think this should be backported to beta and/or esr?

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8883854 [details] [diff] [review]
When page_purges doesn't zero pages, only record arena chunks for future recycling

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

Thank you for the clarifications. r=me with the comments addressed.

::: memory/mozjemalloc/mozjemalloc.cpp
@@ +796,5 @@
>  };
>  
> +enum ChunkType {
> +  UNKNOWN_CHUNK,
> +  BASE_CHUNK,     // used for the base allocator (e.g. base_alloc)

BASE_CHUNK still isn't used, AFAICT.

@@ +2124,5 @@
>  
>  	unzeroed = pages_purge(chunk, size);
>  
> +	/* If purge doesn't zero the chunk, only record arena chunks or
> +	 * previously recycled chunks. */

The commit message doesn't mention recycled chunks. It probably should to match this comment.

Comment 9

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b84a4439260322c260fc272e3b8a3537b616d3f1

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b84a44392603

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Mike Hommey [:glandium] from comment #7)
> Do you think this should be backported to beta and/or esr?

Without a web content proof-of-concept I'd rather err on the side of caution with a change to something as fundamental as our allocator. Maybe if we still had aurora we could have uplifted to that, but I don't think this is appropriate for a beta uplift (we've already seen one regression).

Comment 12

[Nicholas Nethercote [inactive]]

> Without a web content proof-of-concept I'd rather err on the side of caution
> with a change to something as fundamental as our allocator. Maybe if we
> still had aurora we could have uplifted to that, but I don't think this is
> appropriate for a beta uplift (we've already seen one regression).

Seems reasonable to me given that this behaviour has been present (AFAIK) for a long time.