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

[Adrien Bustany]

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