Guard against invalid allocation flag bits in mozjemalloc

NEW
Assigned to

Status

()

defect
P5
normal
3 years ago
3 months ago

People

(Reporter: ehoogeveen, Assigned: ehoogeveen)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 affected)

Details

Attachments

(1 attachment)

I suspect the crashes in bug 1124397 are caused by corruption in mozjemalloc's allocation metadata. If so, this likely affects more than just the JITs, so it would be nice to know how widespread the problem is.

While actually guarding the allocation metadata against heap corruption probably won't be easy, we can at least check that the flag bits are valid whenever we check the allocation size.
I'm pretty sure the only valid flags for allocations are CHUNK_MAP_ALLOCATED and CHUNK_MAP_LARGE, and this seems to work locally.
Attachment #8833570 - Flags: review?(mh+mozilla)
Comment on attachment 8833570 [details] [diff] [review]
Ensure that only valid flag bits are set for allocations.

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

::: memory/mozjemalloc/jemalloc.c
@@ +4482,5 @@
>  	pageind = (((uintptr_t)ptr - (uintptr_t)chunk) >> pagesize_2pow);
>  	mapbits = chunk->map[pageind].bits;
>  	RELEASE_ASSERT((mapbits & CHUNK_MAP_ALLOCATED) != 0);
> +	RELEASE_ASSERT((mapbits &
> +	    (pagesize_mask & ~CHUNK_MAP_LARGE & ~CHUNK_MAP_ALLOCATED)) == 0);

This expression is hard to reason about ~(CHUNK_MAP_LARGE | CHUNK_MAP_ALLOCATED) would be a little better. But while it's essentially harmless, it's also not very useful in itself. That is, you /might/ hit this (I seriously doubt it, see below), but that won't tell you much. This would be more useful if the associated crash would also contain the raw value in an identifiable way (sometimes you can find that in registers, depending on the code, but most of the time, not). Tangentally, considering the number of times we've needed something like that, maybe it's time to augment MOZ_CRASH or have another manual crash API that conveniently puts data on the stack, to make these things more easily.

Anyways, the hypothesis you want to test is that something might be decrementing the mapbits, such that the size is 1 page less than it should, and that would leave dirty bits in the flag bits. This doesn't really sound likely:
- Spot checking all adjustments of those mapbits in mozjemalloc itself, there is no arithmetic operation performed on it. It's either bitwise operations on the existing value, or plain assignment.
- If something else than mozjemalloc is modifying some value in the chunk headers, we would likely have much more visible problems than a suble 1 page-size difference in some subtle cases. How would anything get a pointer there in the first place? Chunk headers are always at the same position in a chunk, and always has the same size.  You can't get a pointer there from hypothetical leftovers from a previous allocation, except maybe if you have a pointer inside a huge allocation that was later freed.
- Whatever decrement hypothetically happened, it would need to be keeping the two lowest bits at the right value, otherwise the CHUNK_MAP_ALLOCATED assert above would hit, and the allocation would be considered a small one if CHUNK_MAP_LARGE was unset. The fact that the realloc still copies a large size indicates that's not the case, so you would need a decrement of a value that doesn't have the two lowest bits set.

Now, taking a step back, from the crashes caused by bug 1329499, it looks like mozjemalloc and the js stuff don't agree on the old size. What the craches caused by bug 1329499 don't tell is what the js code thinks the old size is, and what mozjemalloc thinks the old size is (malloc_usable_size). Even better would be if it could also tell what the size was that it last reallocated to, and what the pointer address was before that (I feel like I already asked for this). The crashes tell the realloc is not in-place, but was the previous realloc in-place?

Furthermore, now that you've placed my attention on this part of the code, I can see there's a possible race condition (both here when called from iralloc and in arena_dalloc). Under perfect circumstances, it wouldn't matter. But in the light of misbehaving callers doing operations on the same pointer at the same time, could have unintended consequences: mapbits is read before the lock is held. What this means is that one thread could read it while another thread has the lock held and is preparing to update it. Fun ensues. Add to that functions being inlined, and once the thread gets hold of the lock, it might even use the old/wrong value it already read to further its work.

From a cursory look, it seems we should be able to move the locking to before mapbits/mapelm is read without significant performance concerns.

I also suspect it might be possible to make the problem more reproducible locally by making the Vectors reallocate by rounding the the next page instead of ramping up exponentially. Combined with rr chaos mode.
Attachment #8833570 - Flags: review?(mh+mozilla) → review-
Also, if you can reproduce the problem on Windows, you can try logalloc (see memory/replace/logalloc/README) to get a log of all malloc/realloc/free calls, or IntelliTrace.

Mmmmm I wonder if it's reproducible under Wine on Linux, and whether rr works with wine.
Sorry about the delayed response, I was very busy last week and needed some time to recover.

> Tangentally, considering the number of
> times we've needed something like that, maybe it's time to augment MOZ_CRASH
> or have another manual crash API that conveniently puts data on the stack,
> to make these things more easily.

As you've probably seen, I opened bug 1338574 about this. Should be useful if we decide we need more information here.

> - If something else than mozjemalloc is modifying some value in the chunk
> headers, we would likely have much more visible problems than a suble 1
> page-size difference in some subtle cases. How would anything get a pointer
> there in the first place? Chunk headers are always at the same position in a
> chunk, and always has the same size.  You can't get a pointer there from
> hypothetical leftovers from a previous allocation, except maybe if you have
> a pointer inside a huge allocation that was later freed.

That's the sort of thing I was thinking of, but I admit that it's unlikely.

> - Whatever decrement hypothetically happened, it would need to be keeping
> the two lowest bits at the right value, otherwise the CHUNK_MAP_ALLOCATED
> assert above would hit, and the allocation would be considered a small one
> if CHUNK_MAP_LARGE was unset. The fact that the realloc still copies a large
> size indicates that's not the case, so you would need a decrement of a value
> that doesn't have the two lowest bits set.

To be fair, that isn't hard - any multiple of 4 will do (though presumably something less than the page size)

> Now, taking a step back, from the crashes caused by bug 1329499, it looks
> like mozjemalloc and the js stuff don't agree on the old size. What the
> craches caused by bug 1329499 don't tell is what the js code thinks the old
> size is, and what mozjemalloc thinks the old size is (malloc_usable_size).

True, but from the crashes we've analyzed, the problem area always seems to be the last page of a power-of-two sized buffer. Since we start out with a 256-byte buffer, mozilla::Vector always doubles, and the buffer size stored in the crash reports is always a power of two, I think it's unlikely that the vector's capacity is wrong. I'm also not sure how to make malloc_usable_size accessible from SM - I got linker errors when I tried. But we can confirm this if the race condition thing falls through.

> Furthermore, now that you've placed my attention on this part of the code, I
> can see there's a possible race condition (both here when called from
> iralloc and in arena_dalloc). Under perfect circumstances, it wouldn't
> matter. But in the light of misbehaving callers doing operations on the same
> pointer at the same time, could have unintended consequences: mapbits is
> read before the lock is held. What this means is that one thread could read
> it while another thread has the lock held and is preparing to update it. Fun
> ensues. Add to that functions being inlined, and once the thread gets hold
> of the lock, it might even use the old/wrong value it already read to
> further its work.

Good find! I'll file a new bug about this. I think we can just add a lock in arena_salloc() and take the lock earlier in arena_dalloc().

> I also suspect it might be possible to make the problem more reproducible
> locally by making the Vectors reallocate by rounding the the next page
> instead of ramping up exponentially. Combined with rr chaos mode.

> Mmmmm I wonder if it's reproducible under Wine on Linux, and whether rr works with wine.

I'll hold off on investigating this until we've tried the above.
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #4)
> True, but from the crashes we've analyzed, the problem area always seems to
> be the last page of a power-of-two sized buffer. Since we start out with a
> 256-byte buffer, mozilla::Vector always doubles, and the buffer size stored
> in the crash reports is always a power of two, I think it's unlikely that
> the vector's capacity is wrong. I'm also not sure how to make
> malloc_usable_size accessible from SM - I got linker errors when I tried.
> But we can confirm this if the race condition thing falls through.

The race condition in realloc is unfortunately hard to plug, so I'm not too hopeful it will help, considering realloc is involved.

You should be able to use malloc_usable_size directly, although it might break building standalone js without jemalloc.
It's not necessarily available on all platforms header-wise, so you'd need to declare it manually:

  extern "C" size_t malloc_usable_size(const void *ptr);

One random thought I had while looking at the arena_ralloc_* code: we could change the poisoning in arena_ralloc_* to each use different values, which would tell us which case of poisoning is hit. 0xe6 and 0xe7 should be safe to use (they are x86 "out" instructions (0xe4 and 0xe5 are "in") ; 0xec through 0xef should be fine too). (may want to make them x86/x64-only, though, I haven't looked at what they are on arm). It would help detect whether the poisoned values comes from free, in-place realloc, or other, which would give further hints at what's going on.
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.