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

[Jouni Malinen]

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);
 }
 
-- 
Jouni Malinen                                            PGP id EFC895FA