838150 - Add ASan annotations to LifoAlloc

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Patch

The JS engine uses multiple internal memory allocators, one of them being LifoAlloc. In order to detect use-after-free in such an allocator, it would be necessary to explicitely poison/unpoison memory when it's deallocated/reallocated. The attached patch does that and has received a day of fuzzing without any related problems. This could however help to detect future use-after-free bugs in the JS engine.

Finally a question to the JS devs: What other allocators can/should we instrument like this?

Comment 1

[Brian Hackett [Laid off!]]

Comment on attachment 710177 [details] [diff] [review]
Patch

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

The allocators in the JS engine I'm aware of are LifoAlloc, malloc (and various wrappers), GC thing allocation, and ExecutableAllocator for JIT stuff.

::: js/src/ds/LifoAlloc.h
@@ +35,5 @@
> +  __asan_poison_memory_region((addr), (size))
> +#define ASAN_UNPOISON_MEMORY_REGION(addr, size) \
> +  __asan_unpoison_memory_region((addr), (size))
> +}
> +#endif

Can you sort out the build issue so that you can just #include asan_interface.h when #ifdef MOZ_ASAN and remove this block?  It would be unfortunate to have to repeat this for every allocator which you want poisoning for.

Comment 2

[Christian Holler (:decoder)]

I don't think it's trivial to solve the header problem on the build side, especially not for try, which we need to support. I copied this piece of code from bug 827150 but we could just put it into a separate header in mfbt/ and include it, how about that?

Comment 3

[Brian Hackett [Laid off!]]

That would be fine.

Comment 4

[Jesse Ruderman]

The header in mfbt/ should provide a macro that acts like ASAN_POISON_MEMORY_REGION, VALGRIND_MAKE_MEM_NOACCESS, or nothing, depending on the memory debugger.

Comment 5

[Christian Holler (:decoder)]

(In reply to Jesse Ruderman from comment #4)
> The header in mfbt/ should provide a macro that acts like
> ASAN_POISON_MEMORY_REGION, VALGRIND_MAKE_MEM_NOACCESS, or nothing, depending
> on the memory debugger.

I'm not sure if these should always be used interchangeably. Valgrind has more options and macros than ASan. In this particular case, the functions behave similarly although the MAKE_MEM_UNDEFINED one tells Valgrind more than the UNPOISON one tells ASan.

I'd leave this to a future bug to consolidate such an interface (we'd have to change this in numerous places I guess).

Comment 6

[Christian Holler (:decoder)]

Asking mats and sewardj for an opinion here. Can you comment on comment 4 and 5? I can file a followup bug for this and change the behavior across our codebase to integrate ASan everywhere where we have Valgrind annotations. I'd call these POISON/UNPOISON_MEMORY_REGION. Does that make sense?

Comment 7

[Julian Seward [:jseward]]

If I understand this correctly, I'd say that the Valgrind annotations
are a superset of the ASan ones, with the following mapping

   MAKE_MEM_NOACCESS      -->    poison_mem_region
   MAKE_MEM_UNDEFINED     -->    unpoison_mem_region
   MAKE_MEM_DEFINED       -->    unpoison_mem_region

If you decide to make a unified interface, I'd suggest you use
the V 3-state scheme rather than the Asan 2-state scheme, and do
the mapping within the macros -- I assume that is what the proposal
was.  And then use MAKE_MEM_NOACCESS and MAKE_MEM_UNDEFINED 
respectively, in this function.

> I'd call these POISON/UNPOISON_MEMORY_REGION. Does that make sense?

I find the V names make it clearer what will happen than the ASan
names.  "Poison" is sometimes used to mean "fill with junk values",
which is not what is going on here.

Comment 8

[Christian Holler (:decoder)]

Thanks Julian, filed bug 838557 to do this followup work :)

Comment 9

[Christian Holler (:decoder)]

Attachment: Patch v2

Patch with bhackett's modifications (separate header, cleanup for the code in nsPresArena, Valgrind support). r+ from bhackett on IRC.

Comment 10

[Christian Holler (:decoder)]

Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e871e6dba26

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e871e6dba26

Comment 12

[Jason Orendorff [:jorendorff]]

This could be a lot better: Add some padding between allocations so that there are some gaps, places for bugs to erroneously write where Valgrind will actually notice the mistake.

Comment 13

[Jesse Ruderman]

I like that idea. Split into bug 841498.