-
-
Notifications
You must be signed in to change notification settings - Fork 245
Fix jit crash caused by hooking intrinsic methods in bootclasspath on Android 15 and above #148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -28,6 +28,9 @@ class ArtMethod { | |||||
inline static auto GetMethodShorty_ = | ||||||
"_ZN3art15GetMethodShortyEP7_JNIEnvP10_jmethodID"_sym.as<const char *(JNIEnv *env, jmethodID mid)>; | ||||||
|
||||||
inline static auto SetNotIntrinsic_ = | ||||||
"_ZN3art9ArtMethod15SetNotIntrinsicEv"_sym.as<void (ArtMethod::*)()>; | ||||||
|
||||||
inline static auto ThrowInvocationTimeError_ = | ||||||
"_ZN3art9ArtMethod24ThrowInvocationTimeErrorEv"_sym.as<void(ArtMethod::*)()>; | ||||||
|
||||||
|
@@ -103,6 +106,12 @@ class ArtMethod { | |||||
SetAccessFlags(access_flags); | ||||||
} | ||||||
|
||||||
void SetNonIntrinsic() { | ||||||
auto access_flags = GetAccessFlags(); | ||||||
access_flags &= ~kAccIntrinsic; | ||||||
SetAccessFlags(access_flags); | ||||||
} | ||||||
|
||||||
bool IsPrivate() { return GetAccessFlags() & kAccPrivate; } | ||||||
bool IsProtected() { return GetAccessFlags() & kAccProtected; } | ||||||
bool IsPublic() { return GetAccessFlags() & kAccPublic; } | ||||||
|
@@ -294,6 +303,7 @@ class ArtMethod { | |||||
kAccPreCompiled = 0x00800000; | ||||||
} | ||||||
if (sdk_int < __ANDROID_API_Q__) kAccFastInterpreterToInterpreterInvoke = 0; | ||||||
if (sdk_int < __ANDROID_API_O__) kAccIntrinsic = 0; | ||||||
|
||||||
if (!handler(GetMethodShortyL_, true, GetMethodShorty_)) { | ||||||
LOGE("Failed to find GetMethodShorty"); | ||||||
|
@@ -302,6 +312,22 @@ class ArtMethod { | |||||
|
||||||
handler(PrettyMethod_, PrettyMethodStatic_, PrettyMethodMirror_); | ||||||
|
||||||
if (sdk_int >= __ANDROID_API_O__ && handler(SetNotIntrinsic_)) { | ||||||
auto dummy = first->Clone(); | ||||||
dummy->SetAccessFlags(kAccIntrinsic); | ||||||
SetNotIntrinsic_(dummy.get()); | ||||||
if (dummy->GetAccessFlags() == kAccIntrinsic) [[unlikely]] { | ||||||
for (auto shift = 16U; 32U > shift; ++shift) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] Consider documenting the rationale behind the magic numbers (16U through 32U) used in the bit-shifting logic or replacing them with named constants to clarify their purpose.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
auto acc = 1U << shift; | ||||||
dummy->SetAccessFlags(acc); | ||||||
SetNotIntrinsic_(dummy.get()); | ||||||
if (dummy->GetAccessFlags() == acc) continue; | ||||||
kAccIntrinsic = acc; | ||||||
break; | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
if (sdk_int <= __ANDROID_API_O__) [[unlikely]] { | ||||||
auto abstract_method_error = JNI_FindClass(env, "java/lang/AbstractMethodError"); | ||||||
if (!abstract_method_error) { | ||||||
|
@@ -367,6 +393,7 @@ class ArtMethod { | |||||
inline static uint32_t kAccPreCompiled = 0x00200000; | ||||||
inline static uint32_t kAccCompileDontBother = 0x02000000; | ||||||
inline static uint32_t kAccDefaultConflict = 0x01000000; | ||||||
inline static uint32_t kAccIntrinsic = 0x80000000; | ||||||
}; | ||||||
|
||||||
} // namespace lsplant::art |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -539,6 +539,8 @@ bool DoHook(ArtMethod *target, ArtMethod *hook, ArtMethod *backup) { | |||||||
} else { | ||||||||
LOGV("Generated trampoline %p", entrypoint); | ||||||||
|
||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nitpick] Consider adding a comment here to explain that removing the intrinsic flag is necessary to prevent a JIT crash on Android 15 and above.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||
target->SetNonIntrinsic(); | ||||||||
|
||||||||
hook->SetNonCompilable(); | ||||||||
|
||||||||
target->BackupTo(backup); | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need another symbol here? SetNonCompilable just use SetAccessFlags for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the value of
kAccIntrinsic
may be modified in subsequent updates of ART, I call this function first.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use
SetNotIntrinsic
symbol since switching between intrinsic and non-intrinsic may affect many flags (kAccIntrinsicBits).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already resolved 25 minutes ago
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean just unsetting the kAccIntrinsic flag is not enough, because intrinsic methods use some other flag bits to store their type.