Message ID | 1444256218.10883.3.camel@redhat.com |
---|---|
State | Changes Requested |
Headers | show |
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?
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
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 --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);
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(-)