From a9dfd6e4537ab7bfc0d34be9561b044271de143f Mon Sep 17 00:00:00 2001 From: Ratchanan Srirattanamet Date: Sat, 5 Oct 2024 17:41:55 +0700 Subject: [PATCH] writer: don't write object offset for NULL binder object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing offset will trigger the kernel-side code to transform the flat binder object into the handle form, which (in my understanding) is not a valid operation for a NULL binder object. Meanwhile, the receiving side will create a corresponding Binder object from such handle, tripping the stability check as it will no longer accept UNDECLARED. OTOH, if the offset is not written, then the receiving side will receive the flat binder object as-is, with type BINDER and pointer NULL, which will be interpreted as NULL binder. This is also what Android's Parcel.cpp does [1][2]. IMO, this is sort of a hack. Binder kernel driver should handle the NULL binder internally, and not relying on the sender doing the correct thing. Meanwhile, the receiver should always reject a flat binder object of type BINDER. But that's how Android work, so... 🤷 [1]: https://github.com/LineageOS/android_frameworks_native/blob/lineage-19.1/libs/binder/Parcel.cpp#L1327-L1332 [2]: https://github.com/LineageOS/android_frameworks_native/blob/lineage-19.1/libs/binder/Parcel.cpp#L2023-L2029 Origin: vendor Forwarded: https://github.com/mer-hybris/libgbinder/pull/135 --- src/gbinder_writer.c | 14 +++++--- unit/unit_local_reply/unit_local_reply.c | 11 ++---- unit/unit_local_request/unit_local_request.c | 37 +++++++++++++------- unit/unit_writer/unit_writer.c | 11 ++---- 4 files changed, 39 insertions(+), 34 deletions(-) diff --git a/src/gbinder_writer.c b/src/gbinder_writer.c index d7f8425..1e93700 100644 --- a/src/gbinder_writer.c +++ b/src/gbinder_writer.c @@ -1176,8 +1176,11 @@ gbinder_writer_data_append_local_object( n = data->io->encode_local_object(buf->data + offset, obj, data->protocol); /* Fix the data size */ g_byte_array_set_size(buf, offset + n); - /* Record the offset */ - gbinder_writer_data_record_offset(data, offset); + + if (obj) { + /* Record the offset */ + gbinder_writer_data_record_offset(data, offset); + } } void @@ -1308,8 +1311,11 @@ gbinder_writer_data_append_remote_object( n = data->io->encode_remote_object(buf->data + offset, obj); /* Fix the data size */ g_byte_array_set_size(buf, offset + n); - /* Record the offset */ - gbinder_writer_data_record_offset(data, offset); + + if (obj) { + /* Record the offset */ + gbinder_writer_data_record_offset(data, offset); + } } static diff --git a/unit/unit_local_reply/unit_local_reply.c b/unit/unit_local_reply/unit_local_reply.c index 6a4abc4..5222e4c 100644 --- a/unit/unit_local_reply/unit_local_reply.c +++ b/unit/unit_local_reply/unit_local_reply.c @@ -454,10 +454,7 @@ test_local_object( reply = test_local_reply_new(); gbinder_local_reply_append_local_object(reply, NULL); data = gbinder_local_reply_data(reply); - offsets = gbinder_output_data_offsets(data); - g_assert(offsets); - g_assert_cmpuint(offsets->count, == ,1); - g_assert_cmpuint(offsets->data[0], == ,0); + g_assert(!gbinder_output_data_offsets(data)); g_assert_cmpuint(gbinder_output_data_buffers_size(data), == ,0); g_assert_cmpuint(data->bytes->len, == ,BINDER_OBJECT_SIZE_32); gbinder_local_reply_unref(reply); @@ -475,14 +472,10 @@ test_remote_object( { GBinderLocalReply* reply = test_local_reply_new(); GBinderOutputData* data; - GUtilIntArray* offsets; gbinder_local_reply_append_remote_object(reply, NULL); data = gbinder_local_reply_data(reply); - offsets = gbinder_output_data_offsets(data); - g_assert(offsets); - g_assert(offsets->count == 1); - g_assert(offsets->data[0] == 0); + g_assert(!gbinder_output_data_offsets(data)); g_assert(!gbinder_output_data_buffers_size(data)); g_assert(data->bytes->len == BINDER_OBJECT_SIZE_32); gbinder_local_reply_unref(reply); diff --git a/unit/unit_local_request/unit_local_request.c b/unit/unit_local_request/unit_local_request.c index 9bc6db7..5434cb5 100644 --- a/unit/unit_local_request/unit_local_request.c +++ b/unit/unit_local_request/unit_local_request.c @@ -33,6 +33,7 @@ #include "test_common.h" #include "test_binder.h" +#include "gbinder_local_object.h" #include "gbinder_local_request_p.h" #include "gbinder_output_data.h" #include "gbinder_rpc_protocol.h" @@ -40,6 +41,7 @@ #include "gbinder_driver.h" #include "gbinder_writer.h" #include "gbinder_io.h" +#include "gbinder_ipc.h" #include @@ -432,19 +434,36 @@ void test_local_object( void) { - GBinderLocalRequest* req = test_local_request_new(); + GBinderLocalRequest* req; GBinderOutputData* data; GUtilIntArray* offsets; + GBinderIpc* ipc = gbinder_ipc_new(NULL, NULL); + const char* const ifaces[] = { "android.hidl.base@1.0::IBase", NULL }; + GBinderLocalObject* obj = gbinder_local_object_new(ipc, ifaces, NULL, NULL); - gbinder_local_request_append_local_object(req, NULL); + /* Append a real object */ + req = test_local_request_new(); + gbinder_local_request_append_local_object(req, obj); data = gbinder_local_request_data(req); offsets = gbinder_output_data_offsets(data); g_assert(offsets); - g_assert(offsets->count == 1); - g_assert(offsets->data[0] == 0); + g_assert_cmpuint(offsets->count, == ,1); + g_assert_cmpuint(offsets->data[0], == ,0); + g_assert_cmpuint(gbinder_output_data_buffers_size(data), == ,0); + g_assert_cmpuint(data->bytes->len, == ,BINDER_OBJECT_SIZE_32); + gbinder_local_request_unref(req); + + /* Append NULL object */ + req = test_local_request_new(); + gbinder_local_request_append_local_object(req, NULL); + data = gbinder_local_request_data(req); + g_assert(!gbinder_output_data_offsets(data)); g_assert(!gbinder_output_data_buffers_size(data)); g_assert(data->bytes->len == BINDER_OBJECT_SIZE_32); gbinder_local_request_unref(req); + + gbinder_local_object_unref(obj); + gbinder_ipc_unref(ipc); } /*==========================================================================* @@ -458,14 +477,10 @@ test_remote_object( { GBinderLocalRequest* req = test_local_request_new(); GBinderOutputData* data; - GUtilIntArray* offsets; gbinder_local_request_append_remote_object(req, NULL); data = gbinder_local_request_data(req); - offsets = gbinder_output_data_offsets(data); - g_assert(offsets); - g_assert(offsets->count == 1); - g_assert(offsets->data[0] == 0); + g_assert(!gbinder_output_data_offsets(data)); g_assert(!gbinder_output_data_buffers_size(data)); g_assert(data->bytes->len == BINDER_OBJECT_SIZE_32); gbinder_local_request_unref(req); @@ -539,12 +554,10 @@ test_remote_request_obj_validate_data( const GByteArray* bytes = data->bytes; GUtilIntArray* offsets = gbinder_output_data_offsets(data); - offsets = gbinder_output_data_offsets(data); g_assert(offsets); - g_assert(offsets->count == 3); + g_assert(offsets->count == 2); g_assert(offsets->data[0] == 4); g_assert(offsets->data[1] == 4 + BUFFER_OBJECT_SIZE_64); - g_assert(offsets->data[2] == 4 + 2*BUFFER_OBJECT_SIZE_64); g_assert(bytes->len == 4 + 2*BUFFER_OBJECT_SIZE_64 + BINDER_OBJECT_SIZE_64); /* GBinderHidlString + the contents (2 bytes) aligned at 8-byte boundary */ g_assert(gbinder_output_data_buffers_size(data) == diff --git a/unit/unit_writer/unit_writer.c b/unit/unit_writer/unit_writer.c index 55565d2..a24a1c1 100644 --- a/unit/unit_writer/unit_writer.c +++ b/unit/unit_writer/unit_writer.c @@ -1359,10 +1359,7 @@ test_local_object( gbinder_local_request_init_writer(req, &writer); gbinder_writer_append_local_object(&writer, NULL); data = gbinder_local_request_data(req); - offsets = gbinder_output_data_offsets(data); - g_assert(offsets); - g_assert_cmpuint(offsets->count, == ,1); - g_assert_cmpuint(offsets->data[0], == ,0); + g_assert(!gbinder_output_data_offsets(data)); g_assert_cmpuint(gbinder_output_data_buffers_size(data), == ,0); g_assert_cmpuint(data->bytes->len, == ,test->objsize); gbinder_local_request_unref(req); @@ -1380,7 +1377,6 @@ test_remote_object( { GBinderLocalRequest* req = test_local_request_new_64(); GBinderOutputData* data; - GUtilIntArray* offsets; GBinderWriter writer; TestContext test; @@ -1388,10 +1384,7 @@ test_remote_object( gbinder_local_request_init_writer(req, &writer); gbinder_writer_append_remote_object(&writer, NULL); data = gbinder_local_request_data(req); - offsets = gbinder_output_data_offsets(data); - g_assert(offsets); - g_assert(offsets->count == 1); - g_assert(offsets->data[0] == 0); + g_assert(!gbinder_output_data_offsets(data)); g_assert(!gbinder_output_data_buffers_size(data)); g_assert(data->bytes->len == BINDER_OBJECT_SIZE_64); gbinder_local_request_unref(req);