Closed Bug 1463596 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- verified
firefox62 --- fixed

People

(Reporter: dmajor, Assigned: aklotz)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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)
Component: General → XPCOM
Sure, I'll take it.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Flags: needinfo?(aklotz)
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+
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
https://hg.mozilla.org/mozilla-central/rev/3518510139bb
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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+
Attached patch Rebased for betaSplinter Review
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.