clang-cl fails TestDllInterceptor due to different page protection in nop space

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: dmajor, Assigned: aklotz)

Tracking

({regression})

unspecified
mozilla62
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 verified, firefox62 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

This is bug 1201205 all over again. (In that patch, I left a comment describing the problem. It looks like the comment survived the interceptor refactor but the behavior didn't, oops.)

0:000> !address rotatepayload-5
...
Protect:                00000002          PAGE_READONLY

0:000> !address rotatepayload
...
Protect:                00000020          PAGE_EXECUTE_READ

aklotz: I'd normally just fix this myself, but I've been finding the new memory policy code to be extremely hard to understand due to all the indirection. If this is an easy one-liner for you, do you mind putting together a patch?
Flags: needinfo?(aklotz)

Updated

11 months ago
Component: General → XPCOM
(Assignee)

Comment 1

11 months ago
Sure, I'll take it.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Flags: needinfo?(aklotz)
(Assignee)

Comment 4

11 months ago
I had to fix a bug in the move constructor.
Attachment #8981265 - Attachment is obsolete: true
Attachment #8981265 - Flags: review?(davidp99)
Attachment #8981267 - Flags: review?(davidp99)
Wow, this turned out to be more involved than I had figured. Thanks Aaron!
Attachment #8981267 - Flags: review?(davidp99) → review+
(Assignee)

Comment 7

11 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3518510139bbb70636739b271fb6fe602e5c644b
Bug 1463596: Ensure that WritableTargetFunction correctly handles changing of protection attributes across regions that straddle page boundaries and have different initial protection attributes; r=handyman

Comment 8

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3518510139bb
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(Assignee)

Comment 9

11 months ago
Comment on attachment 8981267 [details] [diff] [review]
Ensure protection changes work on regions that straddle page boundaries w/ differing initial protection attributes (r2)

Approval Request Comment
[Feature/Bug causing the regression]: bug 1432653
[User impact if declined]: Hooks might not be set under certain conditions, affecting various behaviors across our code base
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low
[Why is the change risky/not risky?]: It's a straightforward patch, it passes automated tests, and has been verified on Nightly.
[String changes made/needed]: None
Attachment #8981267 - Flags: approval-mozilla-beta?
Comment on attachment 8981267 [details] [diff] [review]
Ensure protection changes work on regions that straddle page boundaries w/ differing initial protection attributes (r2)

Not sure if SV was planning another round of TestDLLInterceptor testing during this Beta cycle or not, but they definitely should be now :).

Approved for 61.0b12.
Attachment #8981267 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 11

11 months ago
We did run another round of TestDLLInterceptor on Fx 61.0b12 and we found a new issue (bug 1467798) that is being addressed separately but did not encounter this issue here. Marking as verified on 61 based on our testing.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.