[PATCH 1/2] protocol: Add explicit nullable types
Kristian Høgsberg
hoegsberg at gmail.com
Mon Jul 2 11:35:53 PDT 2012
On Mon, Jul 02, 2012 at 08:03:30PM +1000, Christopher James Halse Rogers wrote:
> 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.
Very nice, thanks. I committed a follow-on change to add two missing
allow-null annotations.
Kristian
> Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>
> ---
> src/connection.c | 81 ++++++++++++++++++++++++++++++++++++++-----
> src/scanner.c | 32 +++++++++++++++++
> tests/connection-test.c | 87 ++++++++++++++++++++++++++++++++++++++++++++---
> 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,
> --
> 1.7.10.4
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list