Last Comment Bug 758106 - PoisonPtr should never be a valid pointer
: PoisonPtr should never be a valid pointer
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Steve Fink [:sfink] [:s:]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 21:02 PDT by Steve Fink [:sfink] [:s:]
Modified: 2012-07-12 09:37 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
PoisonPtr should never be a valid pointer (1.11 KB, patch)
2012-05-23 21:02 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Splinter Review
memory address map of js (3.26 KB, text/plain)
2012-05-24 11:46 PDT, Steve Fink [:sfink] [:s:]
no flags Details
Intentionally leak poisoned pointers (1.34 KB, patch)
2012-07-09 12:10 PDT, Steve Fink [:sfink] [:s:]
bhackett1024: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] 2012-05-23 21:02:36 PDT
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:] 2012-05-23 21:02:45 PDT
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.
Comment 2 Brian Hackett (:bhackett) 2012-05-23 21:16:04 PDT
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:] 2012-05-24 11:46:00 PDT
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.
Comment 4 Steve Fink [:sfink] [:s:] 2012-05-24 11:46:14 PDT
*this line
Comment 5 Steve Fink [:sfink] [:s:] 2012-07-09 12:07:27 PDT
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:] 2012-07-09 12:10:57 PDT
Created attachment 640318 [details] [diff] [review]
Intentionally leak poisoned pointers
Comment 7 Steve Fink [:sfink] [:s:] 2012-07-11 16:59:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8feb24b4989c
Comment 8 Ed Morley [:emorley] 2012-07-12 09:37:48 PDT
https://hg.mozilla.org/mozilla-central/rev/8feb24b4989c

Note You need to log in before you can comment on or make changes to this bug.