460990 - VirtualProtect call in NJ sometimes fails

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Sometimes the VirtualProtect call that NJ uses to mark a chunk of code as executable fails with ERROR_INVALID_ADDRESS. (Worse, we don't bother checking the result... we should at least check-and-assert as we do for the Unix case.)

Reason for failure is unclear, as the pointer value itself looks plausible. As of yet I haven't been able to make it occur in avmshell alone, only in FlashPlayer.

Comment 1

[Steven Johnson]

Why do we use GC memory for Page allocation anyway? seems like going straight to the platform allocator (eg VirtualAlloc) would make more sense for NJ.

Comment 2

[Edwin Smith]

we use GCHeap, which is just an abstraction for raw OS memory.  NJ needs a portability layer for this, but agreed, it doesn't have to be GCHeap.  In fact  Tracemonkey has to emulate the GCHeap layer, so a crisp portability layer makes more sense.

Comment 3

[Steven Johnson]

Adding a portability layer sounds like the right approach to me, but is out of scope for this bug IMHO.

Comment 4

[Steven Johnson]

Code in nMarkExecutable for Nativei386.cpp is wrong: it marks NJ_PAGE_SIZE bytes starting at the code field of the page, but that is actually past the end of the page by a single pointer. Bug occurs if the next page in memory happens to be decommitted (and thus can't be marked as executable). Fix coming up soon.

Comment 5

[Rick Reitmaier]

We also have a SetPageProtection() in GCHeap might want to duke it out to see which one wins.

Comment 6

[Steven Johnson]

That one is apparently never getting called in this context.

Comment 7

[Steven Johnson]

Attachment: Patch

redid nMarkExecute() to fix the bug. Now it always does exactly one page (rather than a count of pages) and to take explicit flags for write and exec (rather than just exec) to lay the groundwork for fixing https://bugzilla.mozilla.org/show_bug.cgi?id=460993

Also enabled nMarkExecute to work on mac, since AVMPLUS_UNIX isn't defined there, oddly enough.

Comment 8

[Steven Johnson]

pushed to redux as changeset:   1015:f1dc0ab0ae9e

Comment 9

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.