From patchwork Sat Apr 14 18:22:02 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jouni Malinen X-Patchwork-Id: 152543 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from maxx.maxx.shmoo.com (maxx.shmoo.com [205.134.188.171]) by ozlabs.org (Postfix) with ESMTP id DB185B7003 for ; Sun, 15 Apr 2012 04:22:18 +1000 (EST) Received: from localhost (localhost [127.0.0.1]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id 75BB79D1E1; Sat, 14 Apr 2012 14:22:16 -0400 (EDT) X-Virus-Scanned: amavisd-new at maxx.shmoo.com Received: from maxx.maxx.shmoo.com ([127.0.0.1]) by localhost (maxx.shmoo.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id LFWkuSkZJMQv; Sat, 14 Apr 2012 14:22:16 -0400 (EDT) Received: from maxx.shmoo.com (localhost [127.0.0.1]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id 10DB29C206; Sat, 14 Apr 2012 14:22:12 -0400 (EDT) X-Original-To: mailman-post+hostap@maxx.shmoo.com Delivered-To: mailman-post+hostap@maxx.shmoo.com Received: from localhost (localhost [127.0.0.1]) by maxx.maxx.shmoo.com (Postfix) with ESMTP id CE5DE9C206 for ; Sat, 14 Apr 2012 14:22:10 -0400 (EDT) X-Virus-Scanned: amavisd-new at maxx.shmoo.com Received: from maxx.maxx.shmoo.com ([127.0.0.1]) by localhost (maxx.shmoo.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 975B7zsxVPNn for ; Sat, 14 Apr 2012 14:22:06 -0400 (EDT) Received: from jmaline2.user.openhosting.com (kvm.w1.fi [128.177.28.162]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by maxx.maxx.shmoo.com (Postfix) with ESMTPS id 100309C201 for ; Sat, 14 Apr 2012 14:22:05 -0400 (EDT) Received: from jm (a91-155-81-182.elisa-laajakaista.fi [91.155.81.182]) (authenticated bits=0) by jmaline2.user.openhosting.com (8.13.8/8.13.8) with ESMTP id q3EID6eA007353 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Sat, 14 Apr 2012 14:13:07 -0400 Received: by jm (sSMTP sendmail emulation); Sat, 14 Apr 2012 21:22:02 +0300 Date: Sat, 14 Apr 2012 21:22:02 +0300 From: Jouni Malinen To: hostap@lists.shmoo.com Subject: Re: [PATCH 2/2] P2P: Don't rely on dictionary ordering in wpas_dbus_handler_p2p_add_service Message-ID: <20120414182202.GB7833@w1.fi> Mail-Followup-To: hostap@lists.shmoo.com References: <1334302413-22098-1-git-send-email-adrien@bustany.org> <1334302413-22098-3-git-send-email-adrien@bustany.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1334302413-22098-3-git-send-email-adrien@bustany.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: hostap@lists.shmoo.com X-Mailman-Version: 2.1.9 Precedence: list List-Id: HostAP Project List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: hostap-bounces@lists.shmoo.com Errors-To: hostap-bounces@lists.shmoo.com 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); }