758106 - PoisonPtr should never be a valid pointer

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

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.)

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: 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.

Comment 2

[Brian Hackett [Laid off!]]

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.

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: 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.

Comment 4

[Steve Fink [:sfink] [:s:]]

*this line

Comment 5

[Steve Fink [:sfink] [:s:]]

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.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Intentionally leak poisoned pointers

Comment 7

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8feb24b4989c

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8feb24b4989c