diff mbox

[2/2] dbus: expose interface globals via D-Bus properties

Message ID 1444256218.10883.3.camel@redhat.com
State Changes Requested
Headers show

Commit Message

Dan Williams Oct. 7, 2015, 10:16 p.m. UTC
All interface globals are now exposed as D-Bus properties of type
string, and parsed via the normal interface global parsing functions.

Signed-off-by: Dan Williams <dcbw@redhat.com>
---
 wpa_supplicant/config.c                   | 16 ++++++
 wpa_supplicant/config.h                   |  3 ++
 wpa_supplicant/dbus/dbus_new.c            | 85 ++++++++++++++++++++++++++++++-
 wpa_supplicant/dbus/dbus_new_handlers.c   | 72 ++++++++++++++++++++++++++
 wpa_supplicant/dbus/dbus_new_handlers.h   |  2 +
 wpa_supplicant/dbus/dbus_new_helpers.h    |  2 +
 wpa_supplicant/dbus/dbus_new_introspect.c |  2 +-
 7 files changed, 179 insertions(+), 3 deletions(-)

Comments

Jouni Malinen Oct. 12, 2015, 4:05 p.m. UTC | #1
On Wed, Oct 07, 2015 at 05:16:58PM -0500, Dan Williams wrote:
> All interface globals are now exposed as D-Bus properties of type
> string, and parsed via the normal interface global parsing functions.

This did not compile with number of gcc warnings enabled and once those
issues were fixed, wpabuf_alloc() overflow in introspection kills the
process.. That fixed, dbus_introspect test case passed, but there were
so many memory leaks that I did not really want to spend any more time
on this..

The current snapshot of the patches from my work branch is here:
http://w1.fi/p/dbus-globals/

Would it be possible for you to run these through the mac80211_hwsim
test cases and resubmit a version that does not fail any of the D-Bus
test cases ("./vm-run-sh -f dbus" in tests/hwsim/vm) or leak memory
while going through those tests?
Dan Williams Oct. 12, 2015, 5:08 p.m. UTC | #2
On Mon, 2015-10-12 at 19:05 +0300, Jouni Malinen wrote:
> On Wed, Oct 07, 2015 at 05:16:58PM -0500, Dan Williams wrote:
> > All interface globals are now exposed as D-Bus properties of type
> > string, and parsed via the normal interface global parsing functions.
> 
> This did not compile with number of gcc warnings enabled and once those

Which warnings do you have enabled?  I should enable those too.  I have
gcc 4.9.2 and default CFLAGS, so that means " -MMD -O2 -Wall -g" per
Makefile.  Perhaps you're using gcc 5.x and it has enabled some new
warning flags by default?

> issues were fixed, wpabuf_alloc() overflow in introspection kills the
> process.. That fixed, dbus_introspect test case passed, but there were

I increased the buffer allocation there at least once already for
introspection, but I suppose it dependsa on how much you've compiled in
with CONFIG_xxx=y.  We should fix that better there in the future,
instead of just bumping the value.  Also, we should just allocate that
once since it's never going to change.

> so many memory leaks that I did not really want to spend any more time
> on this..

Which ones?  The only memory allocated in this 2/2 patch is
wpas_dbus_all_interface_properties which is allocated when the first
wifi interface is added and then kept in-memory after that since it is
constant and needed whenever another interface is added.  There's no
point in allocating it per-interface since that would just waste memory.
Inside that, we need to convert the wpa-supplicant property name to a
D-Bus style one, so that requires another allocation but again this is
constant and long-lived.

I'm now converting this global array to one that is allocated on dbus
interface setup and freed when the global dbus interface is de-inited.
That ensures that no memory is left allocated at exit, with the downside
that the supplicant will now always allocate this memory, regardless of
whether or not any interface is ever added.

> The current snapshot of the patches from my work branch is here:
> http://w1.fi/p/dbus-globals/
> 
> Would it be possible for you to run these through the mac80211_hwsim
> test cases and resubmit a version that does not fail any of the D-Bus
> test cases ("./vm-run-sh -f dbus" in tests/hwsim/vm) or leak memory
> while going through those tests?

I'll do that, can you let me know which gcc warnings you're using so I
can ensure the v2 patch fixes them?

Dan
Jouni Malinen Oct. 12, 2015, 9:07 p.m. UTC | #3
On Mon, Oct 12, 2015 at 12:08:25PM -0500, Dan Williams wrote:
> Which warnings do you have enabled?  I should enable those too.  I have
> gcc 4.9.2 and default CFLAGS, so that means " -MMD -O2 -Wall -g" per
> Makefile.  Perhaps you're using gcc 5.x and it has enabled some new
> warning flags by default?

These are from gcc 4.8.4 and I have a number of -W* arguments in
CFLAGS.. I'm not sure which ones of these actually trigger the warnings,
but based on the warnings themselves, -Wmissing-field-initializers and
-Wsign-compare seem to be the ones. I guess this one in
wpa_supplicant/.config ended up enabling these:
CFLAGS += -Wall -Wextra -Wno-unused-parameter

> I increased the buffer allocation there at least once already for
> introspection, but I suppose it dependsa on how much you've compiled in
> with CONFIG_xxx=y.  We should fix that better there in the future,
> instead of just bumping the value.  Also, we should just allocate that
> once since it's never going to change.

Agreed, it would be nice to determine this automatically or alternative,
reallocate the buffer if needed. It is not sufficient to update just the
per-interface wpabuf_alloc(); also the encapsulating buffer needs to be
larger. And yes, I do have all build options that can affect this size
enabled.

> > so many memory leaks that I did not really want to spend any more time
> > on this..
> 
> Which ones?  The only memory allocated in this 2/2 patch is
> wpas_dbus_all_interface_properties which is allocated when the first
> wifi interface is added and then kept in-memory after that since it is
> constant and needed whenever another interface is added.  There's no
> point in allocating it per-interface since that would just waste memory.
> Inside that, we need to convert the wpa-supplicant property name to a
> D-Bus style one, so that requires another allocation but again this is
> constant and long-lived.

It looks like there is a large number of leaks from here:

1444683046.251012: [2]: /home/jm/Git/hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant(wpas_dbus_register_interface+0x225) [0x58bd45]
1444683046.251120:      uscore_to_dbus() dbus/dbus_new.c:3409
1444683046.251327:      wpas_dbus_register_interface() dbus/dbus_new.c:3499

(os_zalloc() in uscore_to_dbus(); by the way, there is no code to verify
that this allocation succeeded, i.e., this can segfault..)

and one of this:

1444683046.802881: [2]: /home/jm/Git/hostap/tests/hwsim/../../wpa_supplicant/wpa_supplicant(wpas_dbus_register_interface+0x12d) [0x58bc4d]
1444683046.802940:      wpas_dbus_register_interface() dbus/dbus_new.c:3474


1444683046.803362: MEMLEAK: total 8142 bytes


> I'm now converting this global array to one that is allocated on dbus
> interface setup and freed when the global dbus interface is de-inited.
> That ensures that no memory is left allocated at exit, with the downside
> that the supplicant will now always allocate this memory, regardless of
> whether or not any interface is ever added.

I guess that's fine, but anyway, I will require all memory to be freed
when the process exits to make memory leak analysis a reasonable task.
diff mbox

Patch

diff --git a/wpa_supplicant/config.c b/wpa_supplicant/config.c
index b1adab7..5d085a3 100644
--- a/wpa_supplicant/config.c
+++ b/wpa_supplicant/config.c
@@ -4304,6 +4304,22 @@  int wpa_config_get_value(const char *name, struct wpa_config *config,
 }
 
 
+int wpa_config_get_num_global_field_names(void)
+{
+	return NUM_GLOBAL_FIELDS;
+}
+
+const char * wpa_config_get_global_field_name(int i, int *no_var)
+{
+	if (i < 0 || i >= NUM_GLOBAL_FIELDS)
+		return NULL;
+
+	if (no_var && !global_fields[i].param1)
+		*no_var = 1;
+	return global_fields[i].name;
+}
+
+
 int wpa_config_process_global(struct wpa_config *config, char *pos, int line)
 {
 	size_t i;
diff --git a/wpa_supplicant/config.h b/wpa_supplicant/config.h
index 68d64fa..cca602e 100644
--- a/wpa_supplicant/config.h
+++ b/wpa_supplicant/config.h
@@ -1310,6 +1310,9 @@  void wpa_config_debug_dump_networks(struct wpa_config *config);
 /* Prototypes for common functions from config.c */
 int wpa_config_process_global(struct wpa_config *config, char *pos, int line);
 
+int wpa_config_get_num_global_field_names(void);
+
+const char * wpa_config_get_global_field_name(int i, int *no_var);
 
 /* Prototypes for backend specific functions from the selected config_*.c */
 
diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
index 7d99ea3..7c8f7dc 100644
--- a/wpa_supplicant/dbus/dbus_new.c
+++ b/wpa_supplicant/dbus/dbus_new.c
@@ -2999,6 +2999,8 @@  static const struct wpa_dbus_method_desc wpas_dbus_interface_methods[] = {
 	{ NULL, NULL, NULL, { END_ARGS } }
 };
 
+static struct wpa_dbus_property_desc *wpas_dbus_all_interface_properties = NULL;
+
 static const struct wpa_dbus_property_desc wpas_dbus_interface_properties[] = {
 	{ "Capabilities", WPAS_DBUS_NEW_IFACE_INTERFACE, "a{sv}",
 	  wpas_dbus_getter_capabilities,
@@ -3398,6 +3400,28 @@  static const struct wpa_dbus_signal_desc wpas_dbus_interface_signals[] = {
 };
 
 
+static char *
+uscore_to_dbus (const char *uscore)
+{
+	const char *p = uscore;
+	char *str, *s;
+	dbus_bool_t last_was_uscore = TRUE;
+
+	s = str = os_zalloc(strlen (uscore) + 1);
+	while (p && *p) {
+		if (*p == '_')
+			last_was_uscore = TRUE;
+		else {
+			*s++ = last_was_uscore ? toupper (*p) : *p;
+			last_was_uscore = FALSE;
+		}
+		++p;
+	}
+
+	return str;
+}
+
+
 /**
  * wpas_dbus_register_interface - Register an interface with D-Bus
  * @wpa_s: wpa_supplicant interface structure
@@ -3405,7 +3429,6 @@  static const struct wpa_dbus_signal_desc wpas_dbus_interface_signals[] = {
  */
 int wpas_dbus_register_interface(struct wpa_supplicant *wpa_s)
 {
-
 	struct wpa_dbus_object_desc *obj_desc = NULL;
 	struct wpas_dbus_priv *ctrl_iface = wpa_s->global->dbus;
 	int next;
@@ -3430,8 +3453,66 @@  int wpas_dbus_register_interface(struct wpa_supplicant *wpa_s)
 		goto err;
 	}
 
+	if (wpas_dbus_all_interface_properties == NULL) {
+		size_t all_size;
+		int i, j, count, num_const, num_globals;
+		const char *global_name;
+		static const char *ignored_globals[] = {
+			"bss_expiration_age", "bss_expiration_scan_count",
+			"ap_scan", "country", "fast_reauth",
+			"pkcs11_engine_path", "pkcs11_module_path"
+		};
+
+		/* wpas_dbus_interface_properties terminates with a NULL element */
+		num_const = ARRAY_SIZE(wpas_dbus_interface_properties) - 1;
+
+		num_globals = wpa_config_get_num_global_field_names();
+
+		/* allocate enough for all properties + terminating NULL element */
+		all_size = (num_globals + num_const + 1) * 
+				sizeof (wpas_dbus_interface_properties[0]);
+		wpas_dbus_all_interface_properties = os_zalloc(all_size);
+		if (!wpas_dbus_all_interface_properties) {
+			wpa_printf(MSG_ERROR,
+				   "Not enough memory for interface properties");
+			goto err;
+		}
+		os_memcpy(wpas_dbus_all_interface_properties,
+			  wpas_dbus_interface_properties,
+			  sizeof(wpas_dbus_interface_properties));
+
+		for (i = 0, count = num_const; i < num_globals; i++) {
+			int no_var = 0;
+
+			/* ignore globals that are actually just methods */
+			global_name = wpa_config_get_global_field_name(i, &no_var);
+			if (no_var)
+				continue;
+			/* Ignore fields already exposed explicitly */
+			for (j = 0; j < ARRAY_SIZE(ignored_globals); j++) {
+				if (os_strcmp(global_name, ignored_globals[j]) == 0)
+					break;
+			}
+			if (j < ARRAY_SIZE(ignored_globals))
+				continue;
+
+			wpas_dbus_all_interface_properties[count].dbus_property =
+				uscore_to_dbus(global_name);
+			wpas_dbus_all_interface_properties[count].dbus_interface =
+				WPAS_DBUS_NEW_IFACE_INTERFACE;
+			wpas_dbus_all_interface_properties[count].type = "s";
+			wpas_dbus_all_interface_properties[count].getter =
+				wpas_dbus_getter_iface_global;
+			wpas_dbus_all_interface_properties[count].setter =
+				wpas_dbus_setter_iface_global;
+			wpas_dbus_all_interface_properties[count].data =
+				global_name;
+			count++;
+		}
+	}
+
 	wpas_dbus_register(obj_desc, wpa_s, NULL, wpas_dbus_interface_methods,
-			   wpas_dbus_interface_properties,
+			   wpas_dbus_all_interface_properties,
 			   wpas_dbus_interface_signals);
 
 	wpa_printf(MSG_DEBUG, "dbus: Register interface object '%s'",
diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
index ac75a5b..dc2bc43 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers.c
+++ b/wpa_supplicant/dbus/dbus_new_handlers.c
@@ -3480,6 +3480,78 @@  dbus_bool_t wpas_dbus_getter_blobs(const struct wpa_dbus_property_desc *property
 }
 
 
+dbus_bool_t wpas_dbus_getter_iface_global(const struct wpa_dbus_property_desc *property_desc,
+					  DBusMessageIter *iter,
+					  DBusError *error, void *user_data)
+{
+	struct wpa_supplicant *wpa_s = user_data;
+	int ret;
+	char buf[250];
+	char *p = buf;
+
+	if (!property_desc->data) {
+		dbus_set_error(error, DBUS_ERROR_INVALID_ARGS,
+			       "Unhandled interface property %s",
+			       property_desc->dbus_property);
+		return FALSE;
+	}
+
+	ret = wpa_config_get_value(property_desc->data, wpa_s->conf, buf,
+			sizeof (buf));
+	if (ret < 0)
+		*p = '\0';
+
+	return wpas_dbus_simple_property_getter(iter, DBUS_TYPE_STRING, &p,
+						error);
+}
+
+
+dbus_bool_t wpas_dbus_setter_iface_global(const struct wpa_dbus_property_desc *property_desc,
+					  DBusMessageIter *iter,
+					  DBusError *error, void *user_data)
+{
+	struct wpa_supplicant *wpa_s = user_data;
+	const char *new_value = NULL;
+	char buf[250];
+	size_t combined_len;
+	int ret;
+
+	if (!wpas_dbus_simple_property_setter(iter, error, DBUS_TYPE_STRING,
+					      &new_value))
+		return FALSE;
+
+	combined_len = strlen (property_desc->data) + strlen (new_value) + 3;
+	if (combined_len >= sizeof (buf)) {
+		dbus_set_error(error, DBUS_ERROR_INVALID_ARGS,
+			       "Interface property %s value too large",
+			       property_desc->dbus_property);
+		return FALSE;
+	}
+
+	if (!new_value[0])
+		new_value = "NULL";
+
+	ret = os_snprintf(buf, combined_len, "%s=%s", property_desc->data,
+			  new_value);
+	if (os_snprintf_error(combined_len, ret)) {
+		dbus_set_error(error,  WPAS_DBUS_ERROR_UNKNOWN_ERROR,
+			       "Failed to construct new interface property %s",
+			       property_desc->dbus_property);
+		return FALSE;
+	}
+
+	if (wpa_config_process_global(wpa_s->conf, buf, -1)) {
+		dbus_set_error(error, DBUS_ERROR_INVALID_ARGS,
+			       "Failed to set interface property %s",
+			       property_desc->dbus_property);
+		return FALSE;
+	}
+
+	wpa_supplicant_update_config(wpa_s);
+	return TRUE;
+}
+
+
 static struct wpa_bss * get_bss_helper(struct bss_handler_args *args,
 				       DBusError *error, const char *func_name)
 {
diff --git a/wpa_supplicant/dbus/dbus_new_handlers.h b/wpa_supplicant/dbus/dbus_new_handlers.h
index 8af99e0..5f2054d 100644
--- a/wpa_supplicant/dbus/dbus_new_handlers.h
+++ b/wpa_supplicant/dbus/dbus_new_handlers.h
@@ -61,6 +61,8 @@  DECLARE_ACCESSOR(wpas_dbus_setter_debug_show_keys)
 DECLARE_ACCESSOR(wpas_dbus_getter_interfaces)
 DECLARE_ACCESSOR(wpas_dbus_getter_eap_methods)
 DECLARE_ACCESSOR(wpas_dbus_getter_global_capabilities)
+DECLARE_ACCESSOR(wpas_dbus_getter_iface_global)
+DECLARE_ACCESSOR(wpas_dbus_setter_iface_global)
 
 DBusMessage * wpas_dbus_handler_scan(DBusMessage *message,
 				     struct wpa_supplicant *wpa_s);
diff --git a/wpa_supplicant/dbus/dbus_new_helpers.h b/wpa_supplicant/dbus/dbus_new_helpers.h
index 44ace3b..c3ae080 100644
--- a/wpa_supplicant/dbus/dbus_new_helpers.h
+++ b/wpa_supplicant/dbus/dbus_new_helpers.h
@@ -94,6 +94,8 @@  struct wpa_dbus_property_desc {
 	WPADBusPropertyAccessor getter;
 	/* property setter function */
 	WPADBusPropertyAccessor setter;
+	/* other data */
+	const char *data;
 };
 
 
diff --git a/wpa_supplicant/dbus/dbus_new_introspect.c b/wpa_supplicant/dbus/dbus_new_introspect.c
index fba57e6..fac352a 100644
--- a/wpa_supplicant/dbus/dbus_new_introspect.c
+++ b/wpa_supplicant/dbus/dbus_new_introspect.c
@@ -38,7 +38,7 @@  static struct interfaces * add_interface(struct dl_list *list,
 	if (!iface)
 		return NULL;
 	iface->dbus_interface = os_strdup(dbus_interface);
-	iface->xml = wpabuf_alloc(6000);
+	iface->xml = wpabuf_alloc(10000);
 	if (iface->dbus_interface == NULL || iface->xml == NULL) {
 		os_free(iface->dbus_interface);
 		wpabuf_free(iface->xml);