[2/2] P2P: Don't rely on dictionary ordering in wpas_dbus_handler_p2p_add_service

On Fri, Apr 13, 2012 at 10:33:33AM +0300, Adrien Bustany wrote:
> In most languages, DBus dictionaries are mapped to either sorted maps
> or hash tables, so you can't control the actual ordering of the
> generated a{sv}. Relying on ordering in this method is unnecessary and
> makes it use from DBus much harder.

In general, this sounds fine, but there are couple of issues with this
patch. Could you please check whether the one in the end of the message
is fine? It could also be worth considering getting this pushed into the
1.x release branch, but I'm not familiar with the D-Bus API to make that
call.

> diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> @@ -2078,23 +2078,30 @@ DBusMessage * wpas_dbus_handler_p2p_add_service(DBusMessage *message,
> +		} else if (!os_strcmp(entry.key, "service") &&
> +			     (entry.type == DBUS_TYPE_STRING)) {
> +			service = os_strdup(entry.str_value);
> +		} else if (!os_strcmp(entry.key, "query")) {
> +			if ((entry.type != DBUS_TYPE_ARRAY) ||
> +			    (entry.array_type != DBUS_TYPE_BYTE))
> +				goto error_clear;
> +			query = wpabuf_alloc_copy(
> +				entry.bytearray_value,
> +				entry.array_len);
> +		} else if (!os_strcmp(entry.key, "response")) {
> +			if ((entry.type != DBUS_TYPE_ARRAY) ||
> +			    (entry.array_type != DBUS_TYPE_BYTE))
> +				goto error_clear;
> +			resp = wpabuf_alloc_copy(entry.bytearray_value,
> +						 entry.array_len);

Moving these handlers into the shared iteration opened up some memory
leaks (I think the original code already had one on an error path, but
this added few additional cases).

> @@ -2107,20 +2114,6 @@ DBusMessage * wpas_dbus_handler_p2p_add_service(DBusMessage *message,
>  			if (!wpa_dbus_dict_get_entry(&iter_dict, &entry))
>  				goto error;
>  
> -			if (!os_strcmp(entry.key, "query")) {
...

Is there any point in leaving the duplicated iteration here with an
empty body? I would assume this was supposed to be removed (as is done
in the patch below).

Le 2012-04-14 21:22, Jouni Malinen a écrit :
> On Fri, Apr 13, 2012 at 10:33:33AM +0300, Adrien Bustany wrote:
>> In most languages, DBus dictionaries are mapped to either sorted 
>> maps
>> or hash tables, so you can't control the actual ordering of the
>> generated a{sv}. Relying on ordering in this method is unnecessary 
>> and
>> makes it use from DBus much harder.
>
> In general, this sounds fine, but there are couple of issues with 
> this
> patch. Could you please check whether the one in the end of the 
> message
> is fine? It could also be worth considering getting this pushed into 
> the
> 1.x release branch, but I'm not familiar with the D-Bus API to make 
> that
> call.
>
>> diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c 
>> b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
>> @@ -2078,23 +2078,30 @@ DBusMessage * 
>> wpas_dbus_handler_p2p_add_service(DBusMessage *message,
>> +		} else if (!os_strcmp(entry.key, "service") &&
>> +			     (entry.type == DBUS_TYPE_STRING)) {
>> +			service = os_strdup(entry.str_value);
>> +		} else if (!os_strcmp(entry.key, "query")) {
>> +			if ((entry.type != DBUS_TYPE_ARRAY) ||
>> +			    (entry.array_type != DBUS_TYPE_BYTE))
>> +				goto error_clear;
>> +			query = wpabuf_alloc_copy(
>> +				entry.bytearray_value,
>> +				entry.array_len);
>> +		} else if (!os_strcmp(entry.key, "response")) {
>> +			if ((entry.type != DBUS_TYPE_ARRAY) ||
>> +			    (entry.array_type != DBUS_TYPE_BYTE))
>> +				goto error_clear;
>> +			resp = wpabuf_alloc_copy(entry.bytearray_value,
>> +						 entry.array_len);
>
> Moving these handlers into the shared iteration opened up some memory
> leaks (I think the original code already had one on an error path, 
> but
> this added few additional cases).
>
>> @@ -2107,20 +2114,6 @@ DBusMessage * 
>> wpas_dbus_handler_p2p_add_service(DBusMessage *message,
>>  			if (!wpa_dbus_dict_get_entry(&iter_dict, &entry))
>>  				goto error;
>>
>> -			if (!os_strcmp(entry.key, "query")) {
> ...
>
> Is there any point in leaving the duplicated iteration here with an
> empty body? I would assume this was supposed to be removed (as is 
> done
> in the patch below).
>
>
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> index edfc734..45e8a69 100644
> --- a/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> +++ b/wpa_supplicant/dbus/dbus_new_handlers_p2p.c
> @@ -2073,7 +2073,7 @@ DBusMessage *
> wpas_dbus_handler_p2p_add_service(DBusMessage *message,
>  	if (!wpa_dbus_dict_open_read(&iter, &iter_dict, NULL))
>  		goto error;
>
> -	if (wpa_dbus_dict_has_dict_entry(&iter_dict)) {
> +	while (wpa_dbus_dict_has_dict_entry(&iter_dict)) {
>  		if (!wpa_dbus_dict_get_entry(&iter_dict, &entry))
>  			goto error;
>
> @@ -2085,23 +2085,30 @@ DBusMessage *
> wpas_dbus_handler_p2p_add_service(DBusMessage *message,
>  				bonjour = 1;
>  			else
>  				goto error_clear;
> -			wpa_dbus_dict_entry_clear(&entry);
> +		} else if (!os_strcmp(entry.key, "version") &&
> +		           entry.type == DBUS_TYPE_INT32) {
> +			version = entry.uint32_value;
> +		} else if (!os_strcmp(entry.key, "service") &&
> +			     (entry.type == DBUS_TYPE_STRING)) {
> +			service = os_strdup(entry.str_value);
> +		} else if (!os_strcmp(entry.key, "query")) {
> +			if ((entry.type != DBUS_TYPE_ARRAY) ||
> +			    (entry.array_type != DBUS_TYPE_BYTE))
> +				goto error_clear;
> +			query = wpabuf_alloc_copy(
> +				entry.bytearray_value,
> +				entry.array_len);
> +		} else if (!os_strcmp(entry.key, "response")) {
> +			if ((entry.type != DBUS_TYPE_ARRAY) ||
> +			    (entry.array_type != DBUS_TYPE_BYTE))
> +				goto error_clear;
> +			resp = wpabuf_alloc_copy(entry.bytearray_value,
> +						 entry.array_len);
>  		}
> +		wpa_dbus_dict_entry_clear(&entry);
>  	}
>
>  	if (upnp == 1) {
> -		while (wpa_dbus_dict_has_dict_entry(&iter_dict)) {
> -			if (!wpa_dbus_dict_get_entry(&iter_dict, &entry))
> -				goto error;
> -
> -			if (!os_strcmp(entry.key, "version") &&
> -			    entry.type == DBUS_TYPE_INT32)
> -				version = entry.uint32_value;
> -			else if (!os_strcmp(entry.key, "service") &&
> -				 entry.type == DBUS_TYPE_STRING)
> -				service = os_strdup(entry.str_value);
> -			wpa_dbus_dict_entry_clear(&entry);
> -		}
>  		if (version <= 0 || service == NULL)
>  			goto error;
>
> @@ -2109,37 +2116,15 @@ DBusMessage *
> wpas_dbus_handler_p2p_add_service(DBusMessage *message,
>  			goto error;
>
>  		os_free(service);
> +		service = NULL;
>  	} else if (bonjour == 1) {
> -		while (wpa_dbus_dict_has_dict_entry(&iter_dict)) {
> -			if (!wpa_dbus_dict_get_entry(&iter_dict, &entry))
> -				goto error;
> -
> -			if (!os_strcmp(entry.key, "query")) {
> -				if ((entry.type != DBUS_TYPE_ARRAY) ||
> -				    (entry.array_type != DBUS_TYPE_BYTE))
> -					goto error_clear;
> -				query = wpabuf_alloc_copy(
> -					entry.bytearray_value,
> -					entry.array_len);
> -			} else if (!os_strcmp(entry.key, "response")) {
> -				if ((entry.type != DBUS_TYPE_ARRAY) ||
> -				    (entry.array_type != DBUS_TYPE_BYTE))
> -					goto error_clear;
> -				resp = wpabuf_alloc_copy(entry.bytearray_value,
> -							 entry.array_len);
> -			}
> -
> -			wpa_dbus_dict_entry_clear(&entry);
> -		}
> -
>  		if (query == NULL || resp == NULL)
>  			goto error;
>
> -		if (wpas_p2p_service_add_bonjour(wpa_s, query, resp) < 0) {
> -			wpabuf_free(query);
> -			wpabuf_free(resp);
> +		if (wpas_p2p_service_add_bonjour(wpa_s, query, resp) < 0)
>  			goto error;
> -		}
> +		query = NULL;
> +		resp = NULL;
>  	} else
>  		goto error;
>
> @@ -2147,6 +2132,9 @@ DBusMessage *
> wpas_dbus_handler_p2p_add_service(DBusMessage *message,
>  error_clear:
>  	wpa_dbus_dict_entry_clear(&entry);
>  error:
> +	os_free(service);
> +	wpabuf_free(query);
> +	wpabuf_free(resp);
>  	return wpas_dbus_error_invalid_args(message, NULL);
>  }

It seems indeed that I cut/pasted a bit too fast for this patch... Your 
version seems better, will you apply it or do you want me to send a 
fixed patch?

Cheers

Adrien

On Tue, Apr 17, 2012 at 12:19:37PM +0300, Adrien Bustany wrote:
> It seems indeed that I cut/pasted a bit too fast for this patch... Your 
> version seems better, will you apply it or do you want me to send a 
> fixed patch?

Thanks, I applied this.