Closed Bug 1108045 Opened 7 years ago Closed 7 years ago

Separate opt.junk into opt.junk and opt.poison if necessary

Categories

(Core :: Memory Allocator, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: ggp, Assigned: ggp)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Flags: needinfo?(dmajor)
To clarify: jemalloc3's behavior is to disable all poisoning on release builds.
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?
Flags: needinfo?(dmajor)
(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)
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.
Which doesn't mean poisoning is worthless, it's still raising the bar on attackers and making exploits more fragile.
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.
Blocks: 1108693
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.
Blocks: 1094275
No longer blocks: 1094275
Depends on: 1094275
Attached patch Junk memory with jemalloc3. (obsolete) — Splinter Review
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.
Attachment #8539395 - Flags: review?(mh+mozilla)
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?
Attachment #8539395 - Flags: review?(mh+mozilla)
Attachment #8539395 - Flags: review+
Attachment #8539395 - Flags: feedback?(nfroyd)
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.
Attachment #8539395 - Flags: feedback?(nfroyd) → feedback+
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+.
Attachment #8539395 - Attachment is obsolete: true
Attachment #8540744 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a0315fb0c4f9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.