[gbinder] correct stability field wire format on Android 12 #133
+68
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
monich
reviewed
Sep 23, 2024
|
@mlehtima could you please double check that this doesn't break anything for you guys? |
peat-psuwit
force-pushed
the
for-upstream/libgbinder-stability-android-rs
branch
from
57afdf6
to
962d249
Compare
|
I'll try to test this tomorrow on an Android 13 based device. |
|
Ping the developers. |
peat-psuwit
force-pushed
the
for-upstream/libgbinder-stability-android-rs
branch
from
962d249
to
cd35bd9
Compare
peat-psuwit
changed the title
|
As it turns out, the stability field wire format change is reverted in Android 13, not 14. New commit is pushed to apply the wire format change in Android 12 only. Thanks to Nikita (@NotKit) for noticing this. |
On Android 12, the wire format of stability field is changed to also include so-called "Binder wire format version", which starts at 1 [1]. A 32-bit-sized struct is re-interpreted into a 32-bit integer, with a layout which makes it incompatible with the old version. Interestingly, they reverted this idea in Android 13 [2], which makes the wire format of the stability field the same as Android 11 again (as far as I know). Add a new RPC protocol variant 'aidl4' to account for this difference. Use this protocol on API level 31 through 32 and use 'aidl3' from API level 33 onwards. The only difference from 'aidl3' is `finish_flatten_ binder()` function. Interestingly, there is also a 16-bit-sized struct variant of the field too [3]. However, to the best of my knowledge, this version is not used in any of the released Android versions. [1]: LineageOS/android_frameworks_native@89ddfc5 [2]: LineageOS/android_frameworks_native@16a4106 [3]: LineageOS/android_frameworks_native@14e4cfa
Nikita (@NotKit) noticed that the change in commit f227ae4 ("[gbinder] All binder objects need stability field in Android 11. JB#58951") has made the aidl4 servicemanager variant redundant. In fact, using aidl4 variant will cause an extra stability field to be sent on the wire (luckily it has not caused any problem). I've tried using aidl3 variant on Volla Phone X23 which runs Halium 12 (API level 32), and service registration still work, which seems to validate this theory. Thus, stop using aidl4 servicemanager variant on any of the API level-based config, as it no longer correspond to any of Android versions. Note that this commit doesn't outright remove aidl4 variant, as doing so would break configurations which explicitly request its use. This commit doesn't doesn't alias the aidl4 variant to aidl3 variant either. Manually requesting a certain variant could mean some unusual setup; aliasing aidl4 to aidl3 could break such setup.
peat-psuwit
force-pushed
the
for-upstream/libgbinder-stability-android-rs
branch
from
cd35bd9
to
374526b
Compare
|
Ouch, what a mess on the Android side. Why does it keep going back and forth. Thank you for working on this, I'm sure it gets merged sooner or later. I'm still hoping that @mlehtima or someone at Jolla gives it more testing to make sure that it doesn't break anything (sorry Matti for pushing you) |
|
Sorry for the delay. I now tested this with an Android 13 based device and with this AIDL services work via libgbinder when using ApiLevel values 30 and 33 as they should and as expected with ApiLevel 31 those don't work so looks good to me. |
mlehtima
approved these changes
Nov 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On Android 12, the wire format of stability field is changed to also
include so-called "Binder wire format version", which starts at 1 [1].
A 32-bit-sized struct is re-interpreted into a 32-bit integer, with a
layout which makes it incompatible with the old version. Interestingly,
they reverted this idea in Android 13 [2], which makes the wire format
of the stability field the same as Android 11 again (as far as I know).
Add a new RPC protocol variant 'aidl4' to account for this difference.
Use this protocol on API level 31 through 32 and use 'aidl3' from API
level 33 onwards. The only difference from 'aidl3' is
finish_flatten_ binder()function.Interestingly, there is also a 16-bit-sized struct variant of the field
too [3]. However, to the best of my knowledge, this version is not used
in any of the released Android versions.
[1]: LineageOS/android_frameworks_native@89ddfc5
[2]: LineageOS/android_frameworks_native@16a4106
[3]: LineageOS/android_frameworks_native@14e4cfa