Guilherme Gonçalves [:ggp] Assignee
	Description • 10 years ago

Bug 860254 separated mozjemalloc's opt_junk into opt_junk and opt_poison, making it possible to only poison freed memory but not uninitialized memory on optimized builds.

jemalloc3 is only capable of doing both at the same time: if opt.junk is enabled, uninitialized memory will be filled with 0xa5, and freed memory will be filled with 0x5a. Thus we may need to upstream this change.

Do we still want to poison freed memory only, or is jemalloc3's behavior sufficient? needinfo :dmajor since he introduced the change.

	Eric Rahm [:erahm]
	Comment 1 • 10 years ago

To clarify: jemalloc3's behavior is to disable all poisoning on release builds.

	(Away)
	Comment 2 • 10 years ago

We definitely want to keep poisoning freed memory. I don't think we want to do the junk on alloc though.

Can we not add the same separation of junk/poison to jemalloc3?

	Mike Hommey [:glandium]
	Comment 3 • 10 years ago

(In reply to dmajor (away/busy) from comment #2)
> We definitely want to keep poisoning freed memory. I don't think we want to
> do the junk on alloc though.
> 
> Can we not add the same separation of junk/poison to jemalloc3?

We can, the question was whether we needed to. I discussed this with dveditz tonight, and he told me it's useful in mitigation but could use a better value than 0x5a (which is too low)

	Daniel Veditz [:dveditz]
	Comment 4 • 10 years ago

Ideally we poison with a value that points to invalid memory, like we do in frame poisoning. If that's the case then the value probably doesn't matter that much: invalid is invalid (barring huge offsets). If it's not pointing at invalid memory then on a 32-bit build an attacker can fill the address space and might be able to abuse any poisoned value.

	Daniel Veditz [:dveditz]
	Comment 5 • 10 years ago

Which doesn't mean poisoning is worthless, it's still raising the bar on attackers and making exploits more fragile.

	Eric Rahm [:erahm]
	Comment 6 • 10 years ago

So just to recap for the original question: yes we want poisoning on free only, so this should be upstreamed to jemalloc3.

We can continue the discussion on what the value should be and whether or not it's already reserved/protected in a separate issue so as not to block progress on switching to upstream jemalloc3.

	Guilherme Gonçalves [:ggp] Assignee
	Comment 7 • 10 years ago

jemalloc3 pull request: https://github.com/jemalloc/jemalloc/pull/172

	Guilherme Gonçalves [:ggp] Assignee
	Comment 8 • 10 years ago

The PR has been merged. It should now be possible to specify the "junk:free" option to have junking on free only, while "junk:true" will work for both allocations and deallocations.

	Guilherme Gonçalves [:ggp] Assignee
	Comment 9 • 10 years ago

We can solve this now that bug 1094275 has landed. This patch is based on the one for opt.lg_dirty_mult over at bug 762448, though.

	Mike Hommey [:glandium]
	Comment 10 • 10 years ago

Comment on attachment 8539395 [details] [diff] [review]
Junk memory with jemalloc3.

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

::: memory/build/jemalloc_config.c
@@ +15,5 @@
>  #else
>  #define MOZ_MALLOC_PLATFORM_OPTIONS ",lg_dirty_mult:6"
>  #endif
>  
> +#ifdef MOZ_MEMORY_DEBUG

Considering how widely unused MOZ_MEMORY_DEBUG is, and considering it's essentially a synonym of DEBUG, please use DEBUG.

@@ +25,3 @@
>  #define MOZ_MALLOC_OPTIONS "narenas:1,lg_chunk:20,tcache:false"
>  MFBT_DATA const char * je_(malloc_conf) =
> +  MOZ_MALLOC_OPTIONS MOZ_MALLOC_PLATFORM_OPTIONS MOZ_MALLOC_BUILD_OPTIONS;

Come to think of it... why use intermediate macros at all?
MFBT_DATA const char * je_(malloc_conf) =
  "narenas:1,lg_chunk:20,tcache:false"
#ifdef MOZ_B2G
  ",lg_dirty_mult:8"
#else
  ",lg_dirty_mult:6"
#endif
#ifdef DEBUG
  ",junk:true"
#else
  ",junk:free"
#endif
;

Not sure it's better, though. Nathan, what do you think?

	Nathan Froyd [:froydnj]
	Comment 11 • 10 years ago

Comment on attachment 8539395 [details] [diff] [review]
Junk memory with jemalloc3.

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

::: memory/build/jemalloc_config.c
@@ +25,3 @@
>  #define MOZ_MALLOC_OPTIONS "narenas:1,lg_chunk:20,tcache:false"
>  MFBT_DATA const char * je_(malloc_conf) =
> +  MOZ_MALLOC_OPTIONS MOZ_MALLOC_PLATFORM_OPTIONS MOZ_MALLOC_BUILD_OPTIONS;

I have a +0 preference for intermediate macros: I think they make things a little more comprehensible.

I would like to see some explanation of why these particular values, if possible.  Perhaps it's more obvious if you go and look at the documentation for particular options, but having B2G with different values for no other reason than "override" is disconcerting.

Likewise for the |junk:| value.

	Guilherme Gonçalves [:ggp] Assignee
	Comment 12 • 10 years ago

Thanks :froydnj! As you probably noticed, this patch only really adds the junk options, which should hopefully be easy to understand from jemalloc's manpage. The other option that's currently not in the tree but is present in the context of this patch is lg_dirty_mult. Bug 762448 comment 24 explains how we chose 6, and bug 762448 comment 7 explains why we're using 8 on B2G. I also included an explanatory comment in the patch before it lands, as per :glandium's request. That bug also has context for tcache:false, which landed earlier.

Here's an updated patch using DEBUG instead of MOZ_MEMORY_DEBUG. I'm carrying over the r+.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 10 years ago

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