Prevent page fault when uninstalling jumper

[flatwhatson]

• page fault occurs when running DARKSOULS.exe in 64bit wine prefix
• something must reprotect that address after jumper is initialized
• calling unprotect() again before uninstall() does the job!

[KoKuToru]

looking at the code i would say unprotect is wrong..

auto address = (void *)((uintptr_t)src & ~(pagesize - 1));
mprotect(address, header.size(), PROT_READ | PROT_WRITE | PROT_EXEC);

the adress gets the nearest page, but the size isn't, it only works when pagesize is a power of 2 .. and src+header.size() doesn't cross a second page..

wouldn't be something like this more correct ?

auto address = (void *)((uintptr_t)src / pagesize * pagesize));
auto size = ((uintptr_t)src + header.size() + pagesize - 1) / pagesize * pagesize - address;
mprotect(address, size, PROT_READ | PROT_WRITE | PROT_EXEC);

.. i think we should put unprotect() into jumper::install and jumper::uninstall

[flatwhatson]

Thanks for your comments, you were right that unprotect() was fishy.

IMO assuming pagesize is a power of 2 is pretty safe, it's the only sane way to implement virtual memory mapping. I've added an assert to document/check this assumption.

The length calculation was incorrect as you pointed out, it was only unprotecting pagestart+N instead of src+N. If src+N crossed a page boundary, we would get a page fault. I tested this fix on its own, but still got the page fault (reprotection is still needed to fix that).

Agreed that always unprotecting before install/uninstall is the best approach, this way it's bullet proof. I've deduplicated the install/uninstall routines as swap_header(), inlined unprotect there, and added a sanity check on the ordering of install/uninstall just in case.

[flatwhatson]

Oh, I also tested that mprotect really requires the address to be aligned to page start. The documentation suggests this might not be needed, but testing confirms that it is (at least on my system). Without this alignment I was instantly getting page faults on apps which normally work fine.

Aligning the length to page end as in your example shouldn't be necessary, the documentation is pretty clear that all pages on the range will be modified.