Closed Bug 1200951 Opened 4 years ago Closed 3 years ago

Change jemalloc4 poison address to something that is not a nop-slide


(Core :: Memory Allocator, defect)

Not set



Tracking Status
firefox53 --- fixed


(Reporter: njn, Assigned: RyanVM)



(Keywords: sec-moderate, Whiteboard: [adv-main53-] (stepping-stone for exploits))


(1 file, 1 obsolete file)

In bug 1044077 we did this for mozjemalloc. We should do it for jemalloc4 as well, if it gets enabled. And possibly upstream the change as well.
I opened to discuss upstream. I went with a public issue because it's not exactly an unknown problem.
Group: core-security → core-security-release
Looks like this was fixed upstream.

Given that we just recently updated our in-tree copy of jemalloc4 to version 4.3.1, we should be able to do this now by setting JEMALLOC_ALLOC_JUNK to 0xe4 and JEMALLOC_FREE_JUNK to 0xe5. Any objections, Mike?
Flags: needinfo?(mh+mozilla)
No objection.
Flags: needinfo?(mh+mozilla)
Not sure how to verify that this is actually working as intended (possible to cause an intentional crash somewhere that'd show the poison pattern?). I'm a bit worried that internal/util.h seems to define these unconditionally and don't know how that'll interact with defining them on the command line as well.

Green on Try, for whatever that's worth, though.
Assignee: nobody → ryanvm
Attachment #8812648 - Flags: review?(mh+mozilla)
Comment on attachment 8812648 [details] [diff] [review]
Use the same poison patterns for jemalloc4 as mozjemalloc

Review of attachment 8812648 [details] [diff] [review]:

This won't work because of the unconditional #ifdefs in util.h. They'd need to be wrapped with #ifndefs, or for those to become something that can be set through a configure flag.
Attachment #8812648 - Flags: review?(mh+mozilla) → review-
Thanks for confirming. I filed an upstream issue for it.
Looks like we can proceed here once we update to version 4.4.0 after it's released.
Confirmed with dveditz that this doesn't need to be closed. It's a known issue and jemalloc4 isn't enabled by default anywhere anyway.
Group: core-security-release
As far as I can tell, this works correctly on top of the tip revision of the upstream rc-4.4.0 branch. When I used intentionally-bogus values, the builds all failed as expected. Everything builds successfully with this patch as-is.
Attachment #8812648 - Attachment is obsolete: true
Attachment #8815095 - Flags: review?(mh+mozilla)
Comment on attachment 8815095 [details] [diff] [review]
Use the same poison patterns for jemalloc4 as mozjemalloc

Review of attachment 8815095 [details] [diff] [review]:

::: memory/jemalloc/
@@ +68,5 @@
>  if CONFIG['GNU_CC']:
>      CFLAGS += ['-std=gnu99']
>  DEFINES['abort'] = 'moz_abort'
> +DEFINES['JEMALLOC_ALLOC_JUNK'] = '((uint8_t)0xe4)'

Kind of a bummer that one needs to put the case in the define... Something else to report upstream I guess...
Attachment #8815095 - Flags: review?(mh+mozilla) → review+
Depends on: 1322027
Pushed by
Use the same poison patterns for jemalloc4 as mozjemalloc. r=glandium
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Whiteboard: (stepping-stone for exploits) → [adv-main53-] (stepping-stone for exploits)
You need to log in before you can comment on or make changes to this bug.