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

login
register
mail settings
Submitter Jouni Malinen
Date April 14, 2012, 6:22 p.m.
Message ID <20120414182202.GB7833@w1.fi>
Download mbox | patch
Permalink /patch/152543/
State Accepted
Commit c9b72c257aa07e1f7bbb4b4897943d969e739055
Headers show

Comments

Jouni Malinen - April 14, 2012, 6:22 p.m.
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).
Adrien Bustany - April 17, 2012, 9:19 a.m.
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
Jouni Malinen - April 21, 2012, 3:31 p.m.
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.

Patch

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);
 }