Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

fixed crash in replaceAsm function #3

Closed
wants to merge 2 commits into from
Closed

Conversation

likunkun
Copy link
Contributor

when the patched function address is the tail of memory page, the
patched code touches two pages. so we must make the two pages writable.

when the patched function address is the tail of memory page, the
patched code touches two pages. so we must make the two pages writable.
@rovo89
Copy link
Owner

rovo89 commented Nov 23, 2013

Thanks, that sounds logical. Same change is necessary for xposedtest.cpp as well, but I will take care of this while merging. Just one question: Is size_t better for address calculations than int?

@rovo89
Copy link
Owner

rovo89 commented Nov 23, 2013

I have looked through the native Android code a bit.. int and size_t seem to be wildly mixed here. I also found uintptr_t, which seems to match best. What do you think?

Also, isn't the check off by one? For simplicitly, let's assume PAGESIZE=10. If function=7 and len=3, the replacement code would go into bytes 7,8 and 9. However, the check would be:
if (0+10 <= 7+3)
which is true, so it incorrectly uses the next memory page. Then again, I'm wondering if I shouldn't just mprotect two pages in any case. The method is called only three times per boot. The overhead of (un)protecting the additional page should be very, very low and it would keep the function more simple.

@likunkun
Copy link
Contributor Author

  1. size_t is the C++ habit, uintptr_t is C. It depends your code style.:D.
  2. It is the bug in the patch. Thank you. it should be used '<‘ instead。

@likunkun
Copy link
Contributor Author

I afraid that if the next page is a writable data segment, we mprotect both of them with out PRO_WRITE at the end. it will crash later, when program write data to the address which is in the next page.

@likunkun
Copy link
Contributor Author

as the code style, I think the uint_ptr is better :D

For simplicitly, let's assume PAGESIZE=10. If function=7 and len=3, the
replacement code would go into bytes 7,8 and 9. However, the check
would be:
if (0+10 <= 7+3)
@rovo89 rovo89 closed this in 538a630 Nov 24, 2013
@rovo89
Copy link
Owner

rovo89 commented Nov 24, 2013

I have used the opportunity to change a few more things. Instead of casting back and forth all the time, uintptr_t is now used for arithmetics and only for the actual memory operations it's casted to void*. Also change the size parameters to size_t.

@likunkun
Copy link
Contributor Author

Great!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants