PoisonPtr should never be a valid pointer

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
On a 64-bit OS, IsPoisonedPtr can easily return a false positive if the 4th lowest byte happens to be an unlucky value (eg the pointer 0x7ffffda000000). I was getting a random set of 1-3 failures every time I ran a set of a few dozen tests in the js/src/tests suite. (Disabling ASLR removes the randomness.)
(Assignee)

Comment 1

5 years ago
Created attachment 626688 [details] [diff] [review]
PoisonPtr should never be a valid pointer

I am requesting review on this patch, but I am lying and really asking for feedback. I changed it to additionally screw up the alignment of the poisoned pointer, but I have no idea if that's going to mess up something else where a Value's type will be misidentified or something. At the very least, I should change the comment to whatever this ends up being.
Attachment #626688 - Flags: review?(bhackett1024)
Comment on attachment 626688 [details] [diff] [review]
PoisonPtr should never be a valid pointer

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

I originally tried this but it doesn't work well because the rooting analysis will give a lot of false positives.  e.g. a GC thing is placed on the stack, dies, and the next use of that word is for a byte sized variable or something.  Then if a GC happens the live byte variable plus the dead stack memory looks like a GC thing and the rooting analysis can poison the live value.  The current scheme of only poisoning the fourth byte is in principle vulnerable to similar behavior but in practice has not given any false positives.  It would be nice if we could ensure that the memory resulting from the poisoning is in fact not addressable due to some fancy syscalls or something but I haven't looked into that deeper.
Attachment #626688 - Flags: review?(bhackett1024)
(Assignee)

Comment 3

5 years ago
Created attachment 626903 [details]
memory address map of js

Here's an example of what's causing my problem. Specifically this long:

00007fffdaf00000 348160K rw---    [ anon ]

...so, lots and lots of pointers with 0xda in just the wrong place.

I suppose I could try to mmap & mprotect a 16MB chunk at 0xda000000 at startup, though I don't actually know how to do that on Windows. I guess I should look at the poisoning code that ted mentioned.
(Assignee)

Comment 4

5 years ago
*this line
Whiteboard: [js:t]
(Assignee)

Comment 5

5 years ago
I'm still running into this, and took a stab at preallocating regions, but it's not as straightforward as I thought. For one, we'd really need to preallocate 0x10000 bytes at 0x??????????da0000 for every value of ??????????. In practice, there are few such values used, so we might want to do it lazily. Except that allocating a poison region that overlaps a pre-existing region will blow away whatever was there before, so you have to know everything that is already in use to avoid them (shared libraries, jemalloc mmaps, etc.)

For these reasons, I've fallen back to just repeatedly allocating until a non-poison pointer is returned. We really ought to decommit the poison pointers, but that must be done at a page granularity, so for now I don't want to bother.
(Assignee)

Comment 6

5 years ago
Created attachment 640318 [details] [diff] [review]
Intentionally leak poisoned pointers
Attachment #626688 - Attachment is obsolete: true
Attachment #640318 - Flags: review?(bhackett1024)
Attachment #640318 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8feb24b4989c
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/8feb24b4989c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.