[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