[gbinder] writer: don't write object offset for NULL binder object

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

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

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(!gbinder_output_data_offsets(data));
-    g_assert(offsets);
-    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_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(!gbinder_output_data_offsets(data));
-    g_assert(offsets);
-    g_assert(offsets->count == 1);
-    g_assert(offsets->data[0] == 0);
     g_assert(!gbinder_output_data_buffers_size(data));
     g_assert(data->bytes->len == BINDER_OBJECT_SIZE_32);
     gbinder_local_reply_unref(reply);

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 <gutil_intarray.h>
 
@@ -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_cmpuint(offsets->count, == ,1);
-    g_assert(offsets->data[0] == 0);
+    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(!gbinder_output_data_offsets(data));
-    g_assert(offsets);
-    g_assert(offsets->count == 1);
-    g_assert(offsets->data[0] == 0);
     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) ==

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(!gbinder_output_data_offsets(data));
-    g_assert(offsets);
-    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, == ,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(!gbinder_output_data_offsets(data));
-    g_assert(offsets);
-    g_assert(offsets->count == 1);
-    g_assert(offsets->data[0] == 0);
     g_assert(!gbinder_output_data_buffers_size(data));
     g_assert(data->bytes->len == BINDER_OBJECT_SIZE_64);
     gbinder_local_request_unref(req);