Self-tests to validate the page permission settings employed by the JIT.

RESOLVED FIXED in Q4 11 - Anza

Status

Tamarin
Baseline JIT (CodegenLIR)
P3
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Rick Reitmaier, Assigned: Rick Reitmaier)

Tracking

(Blocks: 1 bug)

unspecified
Q4 11 - Anza
Dependency tree / graph
Bug Flags:
flashplayer-injection -
flashplayer-qrb +
flashplayer-bug -

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey)

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
We should augment the self-tests to include some form of validation that the page permission bits are set as expected before / after (possibly during?) code compilation.
(Assignee)

Updated

7 years ago
Blocks: 593517

Updated

7 years ago
Flags: flashplayer-qrb+

Comment 1

7 years ago
Tests should be checked in along with the fix for bug 602551.
Depends on: 602551
(Assignee)

Comment 2

7 years ago
Just an FYI, the self test framework doesn't (at first glance) appear to accommodate the style of testing needed to validate this property, so something more creative is going to be required.

Possibly, we might want to shrink (or very) the allocation size during the test (say to 4KB page), in order to gain more granularity on the page bit settings.

Additionally, I'm guessing we'll have to make AS function calls of code that exists in builtin.abc in order to tickle code generation.  Either that or roll-up an array of abc bytecodes and include them in the test; which is somewhat undesirable.
(Assignee)

Comment 3

7 years ago
Created attachment 482693 [details] [diff] [review]
v1

The ST infrastructure isn't set up for this type of validation, as the compiler needs to have most of the system up and running in order to run the tests.   This coupled with the fact that we wouldn't be able to throw arbitrary methods at it, lead me to the design contained within this patch.

The idea being that we perform the access checks during sanity_check(), which calls out to a new SPI method checkChunkMark() to perform the validation.  In this prototype the guts of the SPI are implemented in CodegenLIR.cpp.  I'm not sure that we want to keep it there, but moving it to VMPI_ implies that its available for all platforms, which may prove difficult as each OS has its own non-portable means of getting at the page permission bits.

The other big caveat is that this code is only enabled for _DEBUG builds, which I think is reasonable, considering that other validation checks are performed in a similar fashion.

Looking for feedback.
Assignee: nobody → rreitmai
Attachment #482693 - Flags: feedback?(edwsmith)

Comment 4

7 years ago
Comment on attachment 482693 [details] [diff] [review]
v1


Probable typo (extra not) and nit (capitalize and punctuate): 

    #ifdef DEBUG
    // note method is not called during debug builds

sticking #ifdef'd platform code to check bits could be the right
thing to do, as a sanity check (what if VMPI isn't implemented correctly),
however a comment should say so, if that is the intent.  If that is not
the intent, then a VMPI api should be used, even if not implemented on
all platforms.

And, perhaps, the best thing would be a series of VMPI self-tests, separate
from CodeAlloc.  I don't see how such a thing would require nanojit, or any
type system, to be up and running.

F+ anyway - this patch is still an improvement with above nits fixed.
Attachment #482693 - Flags: feedback?(edwsmith) → feedback+
(Assignee)

Comment 5

7 years ago
Created attachment 504050 [details] [diff] [review]
provider interface for page permission checking on debug builds

This change adds support within CodeAlloc to validate code chunk page permissions in debug builds.

CodeAlloc provides stubs for markCodeChunkExec/Write that must be implemented by any service providers.  Similarly an implementation for checkChunkMark() should be implemented such that it validates the state defined by prior exec/write calls.

If markCodeChunkExe/Write are stubbed out then likewise checkChunkMark() should simply return true.
Attachment #482693 - Attachment is obsolete: true
Attachment #504050 - Flags: review?(nnethercote)
(Assignee)

Comment 6

7 years ago
Created attachment 504051 [details] [diff] [review]
tamarin portion of the change

An implementation of checkChunkMark() that uses low level calls (on mac probably unsupported) to access the page permission bits.

The code was placed directly in CodegenLIR to mirror what is being done with the markCodeChunkXXX() routines.  Also platform specific ifdef are introduced in order to highlight the fact that these operations are not portable and highly specific , similar to CodeAlloc's use of flushICache().
Attachment #504051 - Flags: review?(edwsmith)
(Assignee)

Updated

7 years ago
Attachment #504050 - Attachment is patch: true
Attachment #504050 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

7 years ago
Attachment #504051 - Attachment is patch: true
Attachment #504051 - Attachment mime type: application/octet-stream → text/plain
Attachment #504050 - Flags: review?(nnethercote) → review+

Updated

7 years ago
Attachment #504051 - Flags: review?(edwsmith) → review+
(Assignee)

Comment 7

7 years ago
Created attachment 530180 [details] [diff] [review]
SM portion of change
(Assignee)

Comment 8

7 years ago
rreitmai http://hg.mozilla.org/projects/nanojit-central/rev/2c412e8f99bb
Whiteboard: fixed-in-nanojit
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 9

7 years ago
Created attachment 530211 [details] [diff] [review]
tamarin portion of the change (rebase)
Attachment #504051 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #530211 - Attachment is patch: true
(Assignee)

Comment 10

7 years ago
Created attachment 530694 [details] [diff] [review]
tamarin portion of the change (rebase)

fixes build error in non - osx / windows environments
Attachment #530211 - Attachment is obsolete: true

Updated

7 years ago
Flags: flashplayer-injection-
Flags: flashplayer-bug-
Priority: -- → P3
Target Milestone: --- → Q4 11 - Anza

Comment 11

7 years ago
changeset: 6283:e41656b83eb0
user:      Rick Reitmaier <rreitmai@adobe.com>
summary:   Bug 602264 - Self-tests to validate the page permission settings employed by the JIT. (r+nnethercote,edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/e41656b83eb0

Comment 12

7 years ago
changeset: 6287:e5e1b1185045
user:      Rick Reitmaier <rreitmai@adobe.com>
summary:   Bug 602264 - Self-tests to validate the page permission settings employed by the JIT, tamarin portion (r+edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/e5e1b1185045
(Assignee)

Updated

7 years ago
Whiteboard: fixed-in-nanojit → fixed-in-nanojit,fixed-in-tamarin
http://hg.mozilla.org/tracemonkey/rev/7328ad354d9a
Whiteboard: fixed-in-nanojit,fixed-in-tamarin → fixed-in-nanojit, fixed-in-tamarin, fixed-in-tracemonkey
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/7328ad354d9a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.