Closed
Bug 522088
Opened 15 years ago
Closed 15 years ago
Reserve address space for frame poisoning on OSX and 32-on-64 Linux
Categories
(Core :: Layout, defect, P2)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta3-fixed |
People
(Reporter: zwol, Assigned: zwol)
References
()
Details
(Whiteboard: [baking for 1.9.2])
Attachments
(1 file, 2 obsolete files)
25.09 KB,
patch
|
zwol
:
review+
|
Details | Diff | Splinter Review |
The CPU and OS do not provide a "totally inaccessible" address region when running a 32-bit process on OSX or a 64-bit Linux kernel. Under these conditions, the frame arena needs to reserve a few pages of address space at startup and mark them inaccessible with mprotect() or similar. The poison address that we're currently using is unmapped at process startup but could become mapped "by accident" -- e.g. it could become part of the malloc arena.
Flags: blocking1.9.2?
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Updated•15 years ago
|
Depends on: FramePoisoning
Comment 1•15 years ago
|
||
This doesn't need to be secret -- it's part of implementing this mitigation safely/correctly but is not a risk to current users.
Group: core-security
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
What's the plan for this 1.9.2 blocker?
Zack just needs to write the code. I imagine it shouldn't be hard.
Assignee | ||
Comment 4•15 years ago
|
||
Here's the patch, finally. I'd be happy closing bug 507924 with this patch as well, thanks to the obsessive unit test in here. I'm asking bsmedberg to review because, despite the location of the code, this is all about the low-level OS stuff.
Attachment #411727 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #411727 -
Flags: review? → review?(benjamin)
Assignee | ||
Comment 5•15 years ago
|
||
*sigh* Of course I meant bug 507294.
Assignee | ||
Comment 6•15 years ago
|
||
The candidate patch has now flunked on the try server twice (in slightly different variations) with this error: make[6]: *** No rule to make target `-lxpcom', needed by `TestPoisonArea'. Stop. (the text is slightly different for Windows but the error is the same) I can't get this to happen on my own machine, and I *thought* I had worked around it by clearing $(LIBS) after rules.mk, but evidently not. Can anyone advise on the proper way to write a C++ unit test that doesn't need or want to link XPCOM?
Assignee | ||
Comment 7•15 years ago
|
||
Another attempt at getting the unit test logic to work.
Attachment #411727 -
Attachment is obsolete: true
Attachment #411769 -
Flags: review?(benjamin)
Attachment #411727 -
Flags: review?(benjamin)
Is it an issue of being a libxul build? Have you read https://developer.mozilla.org/en/XPCOM_Glue ?
Assignee | ||
Comment 9•15 years ago
|
||
(In reply to comment #8) > Is it an issue of being a libxul build? > Have you read https://developer.mozilla.org/en/XPCOM_Glue ? Me not being able to reproduce the problem might be a libxul/not-libxul issue, but it's moot -- the program is written not to need XPCOM, I oughta be able to avoid the dependency. The latest patch does achieve that at the cost of some extra logic in layout/base/tests/Makefile.in. According to the try server, the current sticking point is that WinMo doesn't have FormatMessageA. I wonder how I can detect that at compile time.
Comment 10•15 years ago
|
||
WINCE doesn't have the A versions of almost any windows API. Use FormatMessageW and %S or WideCharToMultiByte.
Comment 11•15 years ago
|
||
Or just print the GetLastError code and move on, life is short.
Comment 12•15 years ago
|
||
Comment on attachment 411769 [details] [diff] [review] candidate patch v2 (revised test harness) The build goop and Windows API look fine. I'm a little concerned that there's a fair bit of code duplication between the test and the nsPresArena code: is it possible that the shared pieces could be pulled out into a file which is #included from both locations?
Attachment #411769 -
Flags: review?(benjamin) → review+
I think we should just land this.
Whiteboard: [needs landing]
Assignee | ||
Comment 14•15 years ago
|
||
(In reply to comment #13) > I think we should just land this. Not while it still breaks the Win(CE|Mo) build! Which I would have fixed by now if I hadn't gotten sick; but as is, we're talking Monday. Sorry.
What's the status here?
Assignee | ||
Comment 16•15 years ago
|
||
I'm going to land it on trunk as soon as trunk reopens. Will post final patch shortly.
Assignee | ||
Comment 17•15 years ago
|
||
This will be landed on mozilla-central as soon as the sheriff reopens it. The only change from v2 is to work around the lack of FormatMessageA on WinCE. I decided not to refactor the shared test/presarena code in the interest of getting this done quicker. I'll file a new bug for that. I think this should be safe to land on 1.9.2 with relatively little bake time on trunk, and I'm going to mark bug 507294 as a duplicate of this one, since that bug is covered by the test case that lands with this bug.
Attachment #411769 -
Attachment is obsolete: true
Attachment #412894 -
Flags: review+
Attachment #412894 -
Flags: approval1.9.2?
Assignee | ||
Comment 19•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3173494c8bdb
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs landing] → [baking for 1.9.2]
Assignee | ||
Updated•15 years ago
|
Attachment #412894 -
Flags: approval1.9.2?
Assignee | ||
Comment 20•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1882c0c8900f
status1.9.2:
--- → beta3-fixed
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•