protocol: Add explicit nullable types

Most of the time it does not make sense to pass a NULL object, string, or array
to a protocol request. This commit adds an explicit “allow-null” attribute
to mark the request arguments where NULL makes sense.

Passing a NULL object, string, or array to a protocol request which is not
marked as allow-null is now an error. An implementation will never receive
a NULL value for these arguments from a client.

Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>

3 files changed, 188 insertions, 12 deletions

diff --git a/src/connection.c b/src/connection.c
index 72ca908..5946556 100644
--- a/src/connection.c
+++ b/src/connection.c
@@ -404,6 +404,36 @@ wl_connection_put_fd(struct wl_connection *connection, int32_t fd)
         return 0;
 }
 
+struct argument_details {
+        char type;
+        int nullable;
+};
+
+static const char *
+get_next_argument(const char *signature, struct argument_details *details)
+{
+        if (*signature == '?') {
+                details->nullable = 1;
+                signature++;
+        } else
+                details->nullable = 0;
+
+        details->type = *signature;
+        return signature + 1;
+}
+
+static int
+arg_count_for_signature(const char *signature)
+{
+        int count = 0;
+        while (*signature) {
+                if (*signature != '?')
+                        count++;
+                signature++;
+        }
+        return count;
+}
+
 struct wl_closure *
 wl_closure_vmarshal(struct wl_object *sender,
                     uint32_t opcode, va_list ap,
@@ -415,6 +445,8 @@ wl_closure_vmarshal(struct wl_object *sender,
         int dup_fd;
         struct wl_array **arrayp, *array;
         const char **sp, *s;
+        const char *signature = message->signature;
+        struct argument_details arg;
         char *extra;
         int i, count, fd, extra_size, *fd_ptr;
 
@@ -424,7 +456,7 @@ wl_closure_vmarshal(struct wl_object *sender,
                 return NULL;
 
         extra_size = wl_message_size_extra(message);
-        count = strlen(message->signature) + 2;
+        count = arg_count_for_signature(signature) + 2;
         extra = (char *) closure->buffer;
         start = &closure->buffer[DIV_ROUNDUP(extra_size, sizeof *p)];
         end = &closure->buffer[256];
@@ -434,7 +466,9 @@ wl_closure_vmarshal(struct wl_object *sender,
         closure->types[1] = &ffi_type_pointer;
 
         for (i = 2; i < count; i++) {
-                switch (message->signature[i - 2]) {
+                signature = get_next_argument(signature, &arg);
+
+                switch (arg.type) {
                 case 'f':
                         closure->types[i] = &ffi_type_sint32;
                         closure->args[i] = p;
@@ -463,6 +497,10 @@ wl_closure_vmarshal(struct wl_object *sender,
                         extra += sizeof *sp;
 
                         s = va_arg(ap, const char *);
+
+                        if (!arg.nullable && s == NULL)
+                                goto err_null;
+
                         length = s ? strlen(s) + 1: 0;
                         if (p + DIV_ROUNDUP(length, sizeof *p) + 1 > end)
                                 goto err;
@@ -483,6 +521,10 @@ wl_closure_vmarshal(struct wl_object *sender,
                         extra += sizeof *objectp;
 
                         object = va_arg(ap, struct wl_object *);
+
+                        if (!arg.nullable && object == NULL)
+                                goto err_null;
+
                         *objectp = object;
                         if (end - p < 1)
                                 goto err;
@@ -508,6 +550,10 @@ wl_closure_vmarshal(struct wl_object *sender,
                         extra += sizeof **arrayp;
 
                         array = va_arg(ap, struct wl_array *);
+
+                        if (!arg.nullable && array == NULL)
+                                goto err_null;
+
                         if (array == NULL || array->size == 0) {
                                 if (end - p < 1)
                                         goto err;
@@ -542,7 +588,7 @@ wl_closure_vmarshal(struct wl_object *sender,
                         break;
                 default:
                         fprintf(stderr, "unhandled format code: '%c'\n",
-                                message->signature[i - 2]);
+                                arg.type);
                         assert(0);
                         break;
                 }
@@ -567,6 +613,15 @@ err:
         errno = ENOMEM;
 
         return NULL;
+
+err_null:
+        free(closure);
+        wl_log("error marshalling arguments for %s:%i.%s (signature %s): "
+               "null value passed for arg %i\n",
+               sender->interface->name, sender->id, message->name,
+               message->signature, i);
+        errno = EINVAL;
+        return NULL;
 }
 
 struct wl_closure *
@@ -579,11 +634,13 @@ wl_connection_demarshal(struct wl_connection *connection,
         int *fd;
         char *extra, **s;
         unsigned int i, count, extra_space;
+        const char *signature = message->signature;
+        struct argument_details arg;
         struct wl_object **object;
         struct wl_array **array;
         struct wl_closure *closure;
 
-        count = strlen(message->signature) + 2;
+        count = arg_count_for_signature(signature) + 2;
         if (count > ARRAY_LENGTH(closure->types)) {
                 printf("too many args (%d)\n", count);
                 errno = EINVAL;
@@ -606,6 +663,8 @@ wl_connection_demarshal(struct wl_connection *connection,
         end = (uint32_t *) ((char *) p + size);
         extra = (char *) end;
         for (i = 2; i < count; i++) {
+                signature = get_next_argument(signature, &arg);
+
                 if (p + 1 > end) {
                         printf("message too short, "
                                "object (%d), message %s(%s)\n",
@@ -614,7 +673,7 @@ wl_connection_demarshal(struct wl_connection *connection,
                         goto err;
                 }
 
-                switch (message->signature[i - 2]) {
+                switch (arg.type) {
                 case 'u':
                         closure->types[i] = &ffi_type_uint32;
                         closure->args[i] = p++;
@@ -784,11 +843,14 @@ copy_fds_to_connection(struct wl_closure *closure,
 {
         const struct wl_message *message = closure->message;
         uint32_t i, count;
+        struct argument_details arg;
+        const char *signature = message->signature;
         int *fd;
 
-        count = strlen(message->signature) + 2;
+        count = arg_count_for_signature(signature) + 2;
         for (i = 2; i < count; i++) {
-                if (message->signature[i - 2] != 'h')
+                signature = get_next_argument(signature, &arg);
+                if (arg.type != 'h')
                         continue;
 
                 fd = closure->args[i];
@@ -834,6 +896,8 @@ wl_closure_print(struct wl_closure *closure, struct wl_object *target, int send)
         union wl_value *value;
         int32_t si;
         int i;
+        struct argument_details arg;
+        const char *signature = closure->message->signature;
         struct timespec tp;
         unsigned int time;
 
@@ -847,11 +911,12 @@ wl_closure_print(struct wl_closure *closure, struct wl_object *target, int send)
                 closure->message->name);
 
         for (i = 2; i < closure->count; i++) {
+                signature = get_next_argument(signature, &arg);
                 if (i > 2)
                         fprintf(stderr, ", ");
 
                 value = closure->args[i];
-                switch (closure->message->signature[i - 2]) {
+                switch (arg.type) {
                 case 'u':
                         fprintf(stderr, "%u", value->uint32);
                         break;
diff --git a/src/scanner.c b/src/scanner.c
index be8d3d6..4d4537c 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -93,6 +93,7 @@ enum arg_type {
 struct arg {
         char *name;
         enum arg_type type;
+        int nullable;
         char *interface_name;
         struct wl_list link;
         char *summary;
@@ -220,6 +221,20 @@ fail(struct parse_context *ctx, const char *msg)
         exit(EXIT_FAILURE);
 }
 
+static int
+is_nullable_type(struct arg *arg)
+{
+        switch (arg->type) {
+        /* Strings, objects, and arrays are possibly nullable */
+        case STRING:
+        case OBJECT:
+        case ARRAY:
+                return 1;
+        default:
+                return 0;
+        }
+}
+
 static void
 start_element(void *data, const char *element_name, const char **atts)
 {
@@ -231,6 +246,7 @@ start_element(void *data, const char *element_name, const char **atts)
         struct entry *entry;
         struct description *description;
         const char *name, *type, *interface_name, *value, *summary, *since;
+        const char *allow_null;
         char *end;
         int i, version;
 
@@ -242,6 +258,7 @@ start_element(void *data, const char *element_name, const char **atts)
         summary = NULL;
         description = NULL;
         since = NULL;
+        allow_null = NULL;
         for (i = 0; atts[i]; i += 2) {
                 if (strcmp(atts[i], "name") == 0)
                         name = atts[i + 1];
@@ -257,6 +274,8 @@ start_element(void *data, const char *element_name, const char **atts)
                         summary = atts[i + 1];
                 if (strcmp(atts[i], "since") == 0)
                         since = atts[i + 1];
+                if (strcmp(atts[i], "allow-null") == 0)
+                        allow_null = atts[i + 1];
         }
 
         ctx->character_data_length = 0;
@@ -364,6 +383,16 @@ start_element(void *data, const char *element_name, const char **atts)
                         break;
                 }
 
+                if (allow_null == NULL || strcmp(allow_null, "false") == 0)
+                        arg->nullable = 0;
+                else if (strcmp(allow_null, "true") == 0)
+                        arg->nullable = 1;
+                else
+                        fail(ctx, "invalid value for allow-null attribute");
+
+                if (allow_null != NULL && !is_nullable_type(arg))
+                        fail(ctx, "allow-null is only valid for objects, strings, and arrays");
+
                 arg->summary = NULL;
                 if (summary)
                         arg->summary = strdup(summary);
@@ -969,6 +998,9 @@ emit_messages(struct wl_list *message_list,
         wl_list_for_each(m, message_list, link) {
                 printf("\t{ \"%s\", \"", m->name);
                 wl_list_for_each(a, &m->arg_list, link) {
+                        if (is_nullable_type(a) && a->nullable)
+                                printf("?");
+
                         switch (a->type) {
                         default:
                         case INT:
diff --git a/tests/connection-test.c b/tests/connection-test.c
index 066b4bc..a852c17 100644
--- a/tests/connection-test.c
+++ b/tests/connection-test.c
@@ -233,9 +233,6 @@ TEST(connection_marshal)
         marshal(&data, "o", 12, &object);
         assert(data.buffer[2] == object.id);
 
-        marshal(&data, "o", 12, NULL);
-        assert(data.buffer[2] == 0);
-
         marshal(&data, "n", 12, &object);
         assert(data.buffer[2] == object.id);
 
@@ -252,6 +249,68 @@ TEST(connection_marshal)
 }
 
 static void
+expected_fail_marshal(int expected_error, const char *format, ...)
+{
+        struct wl_closure *closure;
+        static const uint32_t opcode = 4444;
+        static const struct wl_interface test_interface = { 
+                .name = "test_object"
+        };
+        static struct wl_object sender = { 0 };
+        struct wl_message message = { "test", format, NULL };
+
+        sender.interface = &test_interface;
+        sender.id = 1234;
+        va_list ap;
+
+        va_start(ap, format);
+        closure = wl_closure_vmarshal(&sender, opcode, ap, &message);
+        va_end(ap);
+
+        assert(closure == NULL);
+        assert(errno == expected_error);
+}
+
+TEST(connection_marshal_nullables)
+{
+        struct marshal_data data;
+        struct wl_object object;
+        struct wl_array array;
+        const char text[] = "curry";
+
+        setup_marshal_data(&data);
+
+        expected_fail_marshal(EINVAL, "o", NULL);
+        expected_fail_marshal(EINVAL, "s", NULL);
+        expected_fail_marshal(EINVAL, "a", NULL);
+
+        marshal(&data, "?o", 12, NULL);
+        assert(data.buffer[2] == 0);
+
+        marshal(&data, "?a", 12, NULL);
+        assert(data.buffer[2] == 0);
+
+        marshal(&data, "?s", 12, NULL);
+        assert(data.buffer[2] == 0);
+
+        object.id = 55293;
+        marshal(&data, "?o", 12, &object);
+        assert(data.buffer[2] == object.id);
+
+        array.data = (void *) text;
+        array.size = sizeof text;
+        marshal(&data, "?a", 20, &array);
+        assert(data.buffer[2] == array.size);
+        assert(memcmp(&data.buffer[3], text, array.size) == 0);
+
+        marshal(&data, "?s", 20, text);
+        assert(data.buffer[2] == sizeof text);
+        assert(strcmp((char *) &data.buffer[3], text) == 0);
+
+        release_marshal_data(&data);
+}
+
+static void
 validate_demarshal_u(struct marshal_data *data,
                      struct wl_object *object, uint32_t u)
 {
@@ -269,7 +328,10 @@ static void
 validate_demarshal_s(struct marshal_data *data,
                      struct wl_object *object, const char *s)
 {
-        assert(strcmp(data->value.s, s) == 0);
+        if (data->value.s != NULL)
+                assert(strcmp(data->value.s, s) == 0);
+        else
+                assert(s == NULL);
 }
 
 static void
@@ -343,12 +405,25 @@ TEST(connection_demarshal)
         memcpy(&msg[3], data.value.s, msg[2]);
         demarshal(&data, "s", msg, (void *) validate_demarshal_s);
 
+        data.value.s = "superdude";
+        msg[0] = 400200;
+        msg[1] = 24;
+        msg[2] = 10;
+        memcpy(&msg[3], data.value.s, msg[2]);
+        demarshal(&data, "?s", msg, (void *) validate_demarshal_s);
+
         data.value.i = wl_fixed_from_double(-90000.2390);
         msg[0] = 400200;
         msg[1] = 12;
         msg[2] = data.value.i;
         demarshal(&data, "f", msg, (void *) validate_demarshal_f);
 
+        data.value.s = NULL;
+        msg[0] = 400200;
+        msg[1] = 12;
+        msg[2] = 0;
+        demarshal(&data, "?s", msg, (void *) validate_demarshal_s);     
+
         release_marshal_data(&data);
 }
 
@@ -408,6 +483,10 @@ TEST(connection_marshal_demarshal)
         marshal_demarshal(&data, (void *) validate_demarshal_s,
                           28, "s", data.value.s);
 
+        data.value.s = "cookie robots";
+        marshal_demarshal(&data, (void *) validate_demarshal_s,
+                          28, "?s", data.value.s);
+
         data.value.h = mkstemp(f);
         assert(data.value.h >= 0);
         marshal_demarshal(&data, (void *) validate_demarshal_h,