Re: [PATCH v3 3/9] supplicant: Do not wait for D-Bus reply if it's not used
On 21.01.2015 06:32, Patrik Flykt wrote: ConnMan constantly crashes with signal 11 at startup after applying this patch... Yes, there was a bone-headed mistake that resulted in freeing method_call always. Will iterate this once more. BR, H. ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH v4 3/9] supplicant: Do not wait for D-Bus reply if it's not used
Don't wait for a reply from supplicant for D-Bus calls which don't
have a callback function for processing the reply.
---
gsupplicant/dbus.c | 44
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/gsupplicant/dbus.c b/gsupplicant/dbus.c
index 2957979..2d052a5 100644
--- a/gsupplicant/dbus.c
+++ b/gsupplicant/dbus.c
@@ -487,9 +487,10 @@ int supplicant_dbus_method_call(const char *path,
gpointer caller)
{
struct method_call_data *method_call = NULL;
- DBusMessage *message;
+ DBusMessage *message = NULL;
DBusMessageIter iter;
DBusPendingCall *call;
+ int result = 0;
if (!connection)
return -EINVAL;
@@ -497,16 +498,10 @@ int supplicant_dbus_method_call(const char *path,
if (!path || !interface || !method)
return -EINVAL;
- method_call = g_try_new0(struct method_call_data, 1);
- if (!method_call)
- return -ENOMEM;
-
message = dbus_message_new_method_call(SUPPLICANT_SERVICE, path,
interface, method);
- if (!message) {
- g_free(method_call);
+ if (!message)
return -ENOMEM;
- }
dbus_message_set_auto_start(message, FALSE);
@@ -514,17 +509,28 @@ int supplicant_dbus_method_call(const char *path,
if (setup)
setup(iter, user_data);
+ /* No need to wait for reply if there's no reply function */
+ if (!function) {
+ if (!dbus_connection_send(connection, message, NULL))
+ result = -ENOMEM;
+ goto cleanup;
+ }
+
+ method_call = g_try_new0(struct method_call_data, 1);
+ if (!method_call) {
+ result = -ENOMEM;
+ goto cleanup;
+ }
+
if (!dbus_connection_send_with_reply(connection, message,
call, TIMEOUT)) {
- dbus_message_unref(message);
- g_free(method_call);
- return -EIO;
+ result = -ENOMEM;
+ goto cleanup;
}
if (!call) {
- dbus_message_unref(message);
- g_free(method_call);
- return -EIO;
+ result = -EIO;
+ goto cleanup;
}
method_call-caller = caller;
@@ -536,9 +542,15 @@ int supplicant_dbus_method_call(const char *path,
dbus_pending_call_set_notify(call, method_call_reply, method_call,
method_call_free);
- dbus_message_unref(message);
+ method_call = NULL;
- return 0;
+cleanup:
+ if (message)
+ dbus_message_unref(message);
+
+ g_free(method_call);
+
+ return result;
}
void supplicant_dbus_property_append_basic(DBusMessageIter *iter,
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 0/9] Small memory allocation fixes
Reworked patch 2 and 3 based on review comments; did not touch patch 6 as the channel variable is referenced in send_request(). Getting rid of that reference would require adding channel as a parameter and changing all the callers, which is probably more work than it's worth. As I had previously sent v2 for patch 4, sending this set as v3. Hannu Mallat (9): bluetooth: Clean up gdbus client on exit rfkill: Cleanup IO watch on exit supplicant: Do not wait for D-Bus reply if it's not used supplicant: Don't wait for interface removal reply if not needed technology: Clean up technology list on exit rtnl: Cleanup IO watch on exit peer: Clean up at exit dnsproxy: Clean up cache on exit main: Free wifi option parameter gsupplicant/dbus.c | 42 ++ gsupplicant/supplicant.c | 7 +-- plugins/bluetooth.c | 2 ++ src/dnsproxy.c | 15 +-- src/main.c | 1 + src/peer.c | 4 src/rfkill.c | 23 --- src/rtnl.c | 11 +-- src/technology.c | 6 ++ 9 files changed, 78 insertions(+), 33 deletions(-) -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 2/9] rfkill: Cleanup IO watch on exit
IO watch for rfkill events needs to be cleaned up on exit.
---
src/rfkill.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/rfkill.c b/src/rfkill.c
index 960cfea..c6f9674 100644
--- a/src/rfkill.c
+++ b/src/rfkill.c
@@ -153,7 +153,7 @@ static gboolean rfkill_event(GIOChannel *chan, GIOCondition
cond, gpointer data)
return TRUE;
}
-static GIOChannel *channel = NULL;
+static guint watch = 0;
int __connman_rfkill_block(enum connman_service_type type, bool block)
{
@@ -191,6 +191,7 @@ int __connman_rfkill_block(enum connman_service_type type,
bool block)
int __connman_rfkill_init(void)
{
+ GIOChannel *channel;
GIOFlags flags;
int fd;
@@ -215,21 +216,21 @@ int __connman_rfkill_init(void)
/* Process current RFKILL events sent on device open */
while (rfkill_process(channel) == G_IO_STATUS_NORMAL);
- g_io_add_watch(channel, G_IO_IN | G_IO_NVAL | G_IO_HUP | G_IO_ERR,
- rfkill_event, NULL);
+ watch = g_io_add_watch(channel,
+ G_IO_IN | G_IO_NVAL | G_IO_HUP | G_IO_ERR,
+ rfkill_event, NULL);
- return 0;
+ g_io_channel_unref(channel);
+
+ return watch ? 0 : -EIO;
}
void __connman_rfkill_cleanup(void)
{
DBG();
- if (!channel)
- return;
-
- g_io_channel_shutdown(channel, TRUE, NULL);
- g_io_channel_unref(channel);
-
- channel = NULL;
+ if (watch) {
+ g_source_remove(watch);
+ watch = 0;
+ }
}
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 1/9] bluetooth: Clean up gdbus client on exit
Gdbus client for BlueZ was created on plugin initialization, but not cleaned up on plugin exit. Fixed by adding a g_dbus_client_unref() to bluetooth_exit(). --- plugins/bluetooth.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/bluetooth.c b/plugins/bluetooth.c index c6f387e..533c6ce 100644 --- a/plugins/bluetooth.c +++ b/plugins/bluetooth.c @@ -976,6 +976,8 @@ static void bluetooth_exit(void) */ device_driver.disable = NULL; + g_dbus_client_unref(client); + connman_network_driver_unregister(network_driver); g_hash_table_destroy(networks); -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 3/9] supplicant: Do not wait for D-Bus reply if it's not used
Don't wait for a reply from supplicant for D-Bus calls which don't
have a callback function for processing the reply.
---
gsupplicant/dbus.c | 42 ++
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/gsupplicant/dbus.c b/gsupplicant/dbus.c
index 2957979..874be41 100644
--- a/gsupplicant/dbus.c
+++ b/gsupplicant/dbus.c
@@ -487,9 +487,10 @@ int supplicant_dbus_method_call(const char *path,
gpointer caller)
{
struct method_call_data *method_call = NULL;
- DBusMessage *message;
+ DBusMessage *message = NULL;
DBusMessageIter iter;
DBusPendingCall *call;
+ int result = 0;
if (!connection)
return -EINVAL;
@@ -497,16 +498,10 @@ int supplicant_dbus_method_call(const char *path,
if (!path || !interface || !method)
return -EINVAL;
- method_call = g_try_new0(struct method_call_data, 1);
- if (!method_call)
- return -ENOMEM;
-
message = dbus_message_new_method_call(SUPPLICANT_SERVICE, path,
interface, method);
- if (!message) {
- g_free(method_call);
+ if (!message)
return -ENOMEM;
- }
dbus_message_set_auto_start(message, FALSE);
@@ -514,17 +509,28 @@ int supplicant_dbus_method_call(const char *path,
if (setup)
setup(iter, user_data);
+ /* No need to wait for reply if there's no reply function */
+ if (!function) {
+ if (!dbus_connection_send(connection, message, NULL))
+ result = -ENOMEM;
+ goto cleanup;
+ }
+
+ method_call = g_try_new0(struct method_call_data, 1);
+ if (!method_call) {
+ result = -ENOMEM;
+ goto cleanup;
+ }
+
if (!dbus_connection_send_with_reply(connection, message,
call, TIMEOUT)) {
- dbus_message_unref(message);
- g_free(method_call);
- return -EIO;
+ result = -ENOMEM;
+ goto cleanup;
}
if (!call) {
- dbus_message_unref(message);
- g_free(method_call);
- return -EIO;
+ result = -EIO;
+ goto cleanup;
}
method_call-caller = caller;
@@ -536,9 +542,13 @@ int supplicant_dbus_method_call(const char *path,
dbus_pending_call_set_notify(call, method_call_reply, method_call,
method_call_free);
- dbus_message_unref(message);
+cleanup:
+ if (message)
+ dbus_message_unref(message);
- return 0;
+ g_free(method_call);
+
+ return result;
}
void supplicant_dbus_property_append_basic(DBusMessageIter *iter,
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 5/9] technology: Clean up technology list on exit
Clean up technology list when exiting. List may have been populated
e.g. by rfkill events.
---
src/technology.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/technology.c b/src/technology.c
index cd4bc6f..8b26af0 100644
--- a/src/technology.c
+++ b/src/technology.c
@@ -1811,6 +1811,12 @@ void __connman_technology_cleanup(void)
{
DBG();
+ while (technology_list) {
+ struct connman_technology *technology = technology_list-data;
+ technology_list = g_slist_remove(technology_list, technology);
+ technology_put(technology);
+ }
+
g_hash_table_destroy(rfkill_list);
dbus_connection_unref(connection);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 4/9] supplicant: Don't wait for interface removal reply if not needed
Don't wait for a reply from supplicant for interface removal if
there's not callback function for processing the reply.
---
gsupplicant/supplicant.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 060a4df..ef7239f 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -3733,9 +3733,12 @@ int g_supplicant_interface_remove(GSupplicantInterface
*interface,
SUPPLICANT_INTERFACE,
RemoveInterface,
interface_remove_params,
- interface_remove_result, data,
+ callback
+ ? interface_remove_result
+ : NULL,
+ data,
NULL);
- if (ret 0) {
+ if (ret 0 || !callback) {
g_free(data-path);
dbus_free(data);
}
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 6/9] rtnl: Cleanup IO watch on exit
Besides the IO channel for routing events, the associated watch also
needs to be cleaned up on exit.
---
src/rtnl.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/rtnl.c b/src/rtnl.c
index a46aa28..e03a390 100644
--- a/src/rtnl.c
+++ b/src/rtnl.c
@@ -1291,6 +1291,7 @@ static const char *type2string(uint16_t type)
}
static GIOChannel *channel = NULL;
+static guint channel_watch = 0;
struct rtnl_request {
struct nlmsghdr hdr;
@@ -1621,8 +1622,9 @@ int __connman_rtnl_init(void)
g_io_channel_set_encoding(channel, NULL, NULL);
g_io_channel_set_buffered(channel, FALSE);
- g_io_add_watch(channel, G_IO_IN | G_IO_NVAL | G_IO_HUP | G_IO_ERR,
- netlink_event, NULL);
+ channel_watch = g_io_add_watch(channel,
+ G_IO_IN | G_IO_NVAL | G_IO_HUP | G_IO_ERR,
+ netlink_event, NULL);
return 0;
}
@@ -1672,6 +1674,11 @@ void __connman_rtnl_cleanup(void)
g_slist_free(request_list);
request_list = NULL;
+ if (channel_watch) {
+ g_source_remove(channel_watch);
+ channel_watch = 0;
+ }
+
g_io_channel_shutdown(channel, TRUE, NULL);
g_io_channel_unref(channel);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 9/9] main: Free wifi option parameter
As option_wifi is allocated by the option parser, free it before exiting. --- src/main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main.c b/src/main.c index ba09eb6..1c17991 100644 --- a/src/main.c +++ b/src/main.c @@ -749,6 +749,7 @@ int main(int argc, char *argv[]) g_strfreev(connman_settings.tethering_technologies); g_free(option_debug); + g_free(option_wifi); return 0; } -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 8/9] dnsproxy: Clean up cache on exit
Clean up DNS cache and any associated timer on exit.
---
src/dnsproxy.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 7562067..9d7ba61 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -215,6 +215,7 @@ static GSList *request_list = NULL;
static GHashTable *listener_table = NULL;
static time_t next_refresh;
static GHashTable *partial_tcp_req_table;
+static guint cache_timer = 0;
static guint16 get_id(void)
{
@@ -768,6 +769,8 @@ static void cache_element_destroy(gpointer value)
static gboolean try_remove_cache(gpointer user_data)
{
+ cache_timer = 0;
+
if (__sync_fetch_and_sub(cache_refcount, 1) == 1) {
DBG(No cache users, removing it.);
@@ -2208,8 +2211,8 @@ static void destroy_server(struct server_data *server)
* without any good reason. The small delay allows the new RDNSS to
* create a new DNS server instance and the refcount does not go to 0.
*/
- if (cache)
- g_timeout_add_seconds(3, try_remove_cache, NULL);
+ if (cache !cache_timer)
+ cache_timer = g_timeout_add_seconds(3, try_remove_cache, NULL);
g_free(server);
}
@@ -3864,6 +3867,14 @@ void __connman_dnsproxy_cleanup(void)
{
DBG();
+ if (cache_timer) {
+ g_source_remove(cache_timer);
+ cache_timer = 0;
+ }
+
+ g_hash_table_destroy(cache);
+ cache = NULL;
+
connman_notifier_unregister(dnsproxy_notifier);
g_hash_table_foreach(listener_table, remove_listener, NULL);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH v3 7/9] peer: Clean up at exit
Clean up all structures allocated at initialization.
---
src/peer.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/peer.c b/src/peer.c
index 5e9006f..b36b28f 100644
--- a/src/peer.c
+++ b/src/peer.c
@@ -1177,6 +1177,10 @@ void __connman_peer_cleanup(void)
{
DBG();
+ g_hash_table_destroy(peers_notify-remove);
+ g_hash_table_destroy(peers_notify-add);
+ g_free(peers_notify);
+
g_hash_table_destroy(peers_table);
peers_table = NULL;
dbus_connection_unref(connection);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH 4/9] supplicant: Don't wait for interface removal reply if not needed
On 08.01.2015 11:26, Hannu Mallat wrote: Don't wait for a reply from supplicant for interface removal if there's not callback function for processing the reply. There is a problem with this patch as it omits the object path parameter to the method call if there's no callback function. Will send v2 of the patch which operates correctly and is actually shorter. BR, H. ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH v2 4/9] supplicant: Don't wait for interface removal reply if not needed
Don't wait for a reply from supplicant for interface removal if
there's not callback function for processing the reply.
---
gsupplicant/supplicant.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 909a617..131dcec 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -3726,9 +3726,12 @@ int g_supplicant_interface_remove(GSupplicantInterface
*interface,
SUPPLICANT_INTERFACE,
RemoveInterface,
interface_remove_params,
- interface_remove_result, data,
+ callback
+ ? interface_remove_result
+ : NULL,
+ data,
NULL);
- if (ret 0) {
+ if (ret 0 || !callback) {
g_free(data-path);
dbus_free(data);
}
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 7/9] peer: Clean up at exit
Clean up all structures allocated at initialization.
---
src/peer.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/peer.c b/src/peer.c
index caff70c..9386808 100644
--- a/src/peer.c
+++ b/src/peer.c
@@ -1151,6 +1151,10 @@ void __connman_peer_cleanup(void)
{
DBG();
+ g_hash_table_destroy(peers_notify-remove);
+ g_hash_table_destroy(peers_notify-add);
+ g_free(peers_notify);
+
g_hash_table_destroy(peers_table);
peers_table = NULL;
dbus_connection_unref(connection);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 4/9] supplicant: Don't wait for interface removal reply if not needed
Don't wait for a reply from supplicant for interface removal if
there's not callback function for processing the reply.
---
gsupplicant/supplicant.c | 28 +---
1 file changed, 17 insertions(+), 11 deletions(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 909a617..d5779e0 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -3693,7 +3693,8 @@ static void interface_remove_params(DBusMessageIter
*iter, void *user_data)
{
struct interface_data *data = user_data;
- dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH,
+ if (data)
+ dbus_message_iter_append_basic(iter, DBUS_TYPE_OBJECT_PATH,
data-interface-path);
}
@@ -3702,7 +3703,7 @@ int g_supplicant_interface_remove(GSupplicantInterface
*interface,
GSupplicantInterfaceCallback callback,
void *user_data)
{
- struct interface_data *data;
+ struct interface_data *data = NULL;
int ret;
if (!interface)
@@ -3713,22 +3714,27 @@ int g_supplicant_interface_remove(GSupplicantInterface
*interface,
g_supplicant_interface_cancel(interface);
- data = dbus_malloc0(sizeof(*data));
- if (!data)
- return -ENOMEM;
+ if (callback) {
+ data = dbus_malloc0(sizeof(*data));
+ if (!data)
+ return -ENOMEM;
- data-interface = interface;
- data-path = g_strdup(interface-path);
- data-callback = callback;
- data-user_data = user_data;
+ data-interface = interface;
+ data-path = g_strdup(interface-path);
+ data-callback = callback;
+ data-user_data = user_data;
+ }
ret = supplicant_dbus_method_call(SUPPLICANT_PATH,
SUPPLICANT_INTERFACE,
RemoveInterface,
interface_remove_params,
- interface_remove_result, data,
+ data
+ ? interface_remove_result
+ : NULL,
+ data,
NULL);
- if (ret 0) {
+ if (ret 0 data) {
g_free(data-path);
dbus_free(data);
}
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 5/9] technology: Clean up technology list on exit
Clean up technology list when exiting. List may have been populated
e.g. by rfkill events.
---
src/technology.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/src/technology.c b/src/technology.c
index cd4bc6f..8b26af0 100644
--- a/src/technology.c
+++ b/src/technology.c
@@ -1811,6 +1811,12 @@ void __connman_technology_cleanup(void)
{
DBG();
+ while (technology_list) {
+ struct connman_technology *technology = technology_list-data;
+ technology_list = g_slist_remove(technology_list, technology);
+ technology_put(technology);
+ }
+
g_hash_table_destroy(rfkill_list);
dbus_connection_unref(connection);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 8/9] dnsproxy: Clean up cache on exit
Clean up DNS cache and any associated timer on exit.
---
src/dnsproxy.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 7562067..9d7ba61 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -215,6 +215,7 @@ static GSList *request_list = NULL;
static GHashTable *listener_table = NULL;
static time_t next_refresh;
static GHashTable *partial_tcp_req_table;
+static guint cache_timer = 0;
static guint16 get_id(void)
{
@@ -768,6 +769,8 @@ static void cache_element_destroy(gpointer value)
static gboolean try_remove_cache(gpointer user_data)
{
+ cache_timer = 0;
+
if (__sync_fetch_and_sub(cache_refcount, 1) == 1) {
DBG(No cache users, removing it.);
@@ -2208,8 +2211,8 @@ static void destroy_server(struct server_data *server)
* without any good reason. The small delay allows the new RDNSS to
* create a new DNS server instance and the refcount does not go to 0.
*/
- if (cache)
- g_timeout_add_seconds(3, try_remove_cache, NULL);
+ if (cache !cache_timer)
+ cache_timer = g_timeout_add_seconds(3, try_remove_cache, NULL);
g_free(server);
}
@@ -3864,6 +3867,14 @@ void __connman_dnsproxy_cleanup(void)
{
DBG();
+ if (cache_timer) {
+ g_source_remove(cache_timer);
+ cache_timer = 0;
+ }
+
+ g_hash_table_destroy(cache);
+ cache = NULL;
+
connman_notifier_unregister(dnsproxy_notifier);
g_hash_table_foreach(listener_table, remove_listener, NULL);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 9/9] main: Free wifi option parameter
As option_wifi is allocated by the option parser, free it before exiting. --- src/main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main.c b/src/main.c index ba09eb6..1c17991 100644 --- a/src/main.c +++ b/src/main.c @@ -749,6 +749,7 @@ int main(int argc, char *argv[]) g_strfreev(connman_settings.tethering_technologies); g_free(option_debug); + g_free(option_wifi); return 0; } -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 0/9] Small memory allocation fixes
The following patches are mostly to clean up valgrind reporting of unreleased but still reachable memory and are related to releasing all allocated resources within the various cleanup functions. The two supplicant patches are for not allocating a pending call and related structures when there's no callback function for handing a D-Bus reply. Hannu Mallat (9): bluetooth: Clean up gdbus client on exit rfkill: Cleanup IO watch on exit supplicant: Do not wait for D-Bus reply if it's not used supplicant: Don't wait for interface removal reply if not needed technology: Clean up technology list on exit rtnl: Cleanup IO watch on exit peer: Clean up at exit dnsproxy: Clean up cache on exit main: Free wifi option parameter gsupplicant/dbus.c | 23 --- gsupplicant/supplicant.c | 28 +--- plugins/bluetooth.c | 2 ++ src/dnsproxy.c | 15 +-- src/main.c | 1 + src/peer.c | 4 src/rfkill.c | 11 +-- src/rtnl.c | 11 +-- src/technology.c | 6 ++ 9 files changed, 77 insertions(+), 24 deletions(-) -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 6/9] rtnl: Cleanup IO watch on exit
Besides the IO channel for routing events, the associated watch also
needs to be cleaned up on exit.
---
src/rtnl.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/rtnl.c b/src/rtnl.c
index a46aa28..e03a390 100644
--- a/src/rtnl.c
+++ b/src/rtnl.c
@@ -1291,6 +1291,7 @@ static const char *type2string(uint16_t type)
}
static GIOChannel *channel = NULL;
+static guint channel_watch = 0;
struct rtnl_request {
struct nlmsghdr hdr;
@@ -1621,8 +1622,9 @@ int __connman_rtnl_init(void)
g_io_channel_set_encoding(channel, NULL, NULL);
g_io_channel_set_buffered(channel, FALSE);
- g_io_add_watch(channel, G_IO_IN | G_IO_NVAL | G_IO_HUP | G_IO_ERR,
- netlink_event, NULL);
+ channel_watch = g_io_add_watch(channel,
+ G_IO_IN | G_IO_NVAL | G_IO_HUP | G_IO_ERR,
+ netlink_event, NULL);
return 0;
}
@@ -1672,6 +1674,11 @@ void __connman_rtnl_cleanup(void)
g_slist_free(request_list);
request_list = NULL;
+ if (channel_watch) {
+ g_source_remove(channel_watch);
+ channel_watch = 0;
+ }
+
g_io_channel_shutdown(channel, TRUE, NULL);
g_io_channel_unref(channel);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 2/9] rfkill: Cleanup IO watch on exit
Besides the IO channel for rfkill events, the associated watch also
needs to be cleaned up on exit.
---
src/rfkill.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/src/rfkill.c b/src/rfkill.c
index 960cfea..b248c26 100644
--- a/src/rfkill.c
+++ b/src/rfkill.c
@@ -154,6 +154,7 @@ static gboolean rfkill_event(GIOChannel *chan, GIOCondition
cond, gpointer data)
}
static GIOChannel *channel = NULL;
+static guint watch = 0;
int __connman_rfkill_block(enum connman_service_type type, bool block)
{
@@ -215,8 +216,9 @@ int __connman_rfkill_init(void)
/* Process current RFKILL events sent on device open */
while (rfkill_process(channel) == G_IO_STATUS_NORMAL);
- g_io_add_watch(channel, G_IO_IN | G_IO_NVAL | G_IO_HUP | G_IO_ERR,
- rfkill_event, NULL);
+ watch = g_io_add_watch(channel,
+ G_IO_IN | G_IO_NVAL | G_IO_HUP | G_IO_ERR,
+ rfkill_event, NULL);
return 0;
}
@@ -228,6 +230,11 @@ void __connman_rfkill_cleanup(void)
if (!channel)
return;
+ if (watch) {
+ g_source_remove(watch);
+ watch = 0;
+ }
+
g_io_channel_shutdown(channel, TRUE, NULL);
g_io_channel_unref(channel);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] network: protect service structure access with reference
As __connman_connection_gateway_remove() may unref service, wrap network.c:set_disconnected() with connman_service_ref/unref to make sure the struct stays around during the function call. --- src/network.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/network.c b/src/network.c index b388995..de1464c 100644 --- a/src/network.c +++ b/src/network.c @@ -703,6 +703,8 @@ static void set_disconnected(struct connman_network *network) struct connman_service *service; service = connman_service_lookup_from_network(network); + if (service) + connman_service_ref(service); ipconfig_ipv4 = __connman_service_get_ip4config(service); ipconfig_ipv6 = __connman_service_get_ip6config(service); @@ -801,6 +803,9 @@ static void set_disconnected(struct connman_network *network) network-connected = false; connman_network_set_associating(network, false); + + if (service) + connman_service_unref(service); } -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: crash while cancelling an in-progress connection
On 20.11.2014 23:12, Thomas Green wrote: issue is that when I attempt this, that after I make the dbus call to disconnect, and before the disconnect call returns I get a segmentation fault. The stack is hammered enough that the only traceback information is that it happens in libdbus-1.so.3. This only happens when I try to cancel a connection on an open AP. Cancelling all other attempts have worked to this point. Is there something that I need to set a guard around so this doesn't happen? Could you compile connman with debugging enabled and optimization disabled, and debug the issue with valgrind? Once the issue is reproducible that will help a lot in understanding what goes wrong. BR, H. ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 2/2] [connman] inotify: refcount struct connman_inotify
When processing inotify events, callbacks may unregister which can
result in freeing the related connman_inotify struct. However that
struct is referenced in the loop which processes the events read in.
Implement reference counting to make sure the structure is available
while events are being processed.
---
src/inotify.c | 24 +++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/inotify.c b/src/inotify.c
index 12fcb7d..485b7ed 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -35,6 +35,8 @@
#include connman.h
struct connman_inotify {
+ unsigned int refcount;
+
GIOChannel *channel;
uint watch;
int wd;
@@ -42,6 +44,21 @@ struct connman_inotify {
GSList *list;
};
+static void cleanup_inotify(gpointer user_data);
+
+static void connman_inotify_ref(struct connman_inotify *i)
+{
+ __sync_fetch_and_add(i-refcount, 1);
+}
+
+static void connman_inotify_unref(gpointer data)
+{
+ struct connman_inotify *i = data;
+ if (__sync_fetch_and_sub(i-refcount, 1) != 1)
+ return;
+ cleanup_inotify(data);
+}
+
static GHashTable *inotify_hash;
static gboolean inotify_data(GIOChannel *channel, GIOCondition cond,
@@ -75,6 +92,8 @@ static gboolean inotify_data(GIOChannel *channel,
GIOCondition cond,
next_event = buffer;
+ connman_inotify_ref(inotify);
+
while (bytes_read 0) {
struct inotify_event *event;
gchar *ident;
@@ -102,6 +121,8 @@ static gboolean inotify_data(GIOChannel *channel,
GIOCondition cond,
}
}
+ connman_inotify_unref(inotify);
+
return TRUE;
}
@@ -179,6 +200,7 @@ int connman_inotify_register(const char *path,
inotify_event_cb callback)
if (!inotify)
return -ENOMEM;
+ inotify-refcount = 1;
inotify-wd = -1;
err = create_watch(path, inotify);
@@ -225,7 +247,7 @@ int __connman_inotify_init(void)
DBG();
inotify_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
- g_free, cleanup_inotify);
+ g_free, connman_inotify_unref);
return 0;
}
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 1/2] [connman] inotify: fix buffer size
Fix buffer to which inotify events are read to be large enough,
according to inotify(7) manual page.
---
src/inotify.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/inotify.c b/src/inotify.c
index 1ab3807..12fcb7d 100644
--- a/src/inotify.c
+++ b/src/inotify.c
@@ -48,7 +48,7 @@ static gboolean inotify_data(GIOChannel *channel,
GIOCondition cond,
gpointer user_data)
{
struct connman_inotify *inotify = user_data;
- char buffer[256];
+ char buffer[sizeof(struct inotify_event) + NAME_MAX + 1];
char *next_event;
gsize bytes_read;
GIOStatus status;
@@ -60,7 +60,7 @@ static gboolean inotify_data(GIOChannel *channel,
GIOCondition cond,
}
status = g_io_channel_read_chars(channel, buffer,
- sizeof(buffer) -1, bytes_read, NULL);
+ sizeof(buffer), bytes_read, NULL);
switch (status) {
case G_IO_STATUS_NORMAL:
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] agent: Check for pointer validity before cancelling agent request
In the case where agent disappears from D-Bus while there are pending
requests, request-driver may become null. As it's useless to send a
Cancel to a nonexistent agent, not to mention accessing null pointer
causes a crash, check for the pointer value before creating a D-Bus
message.
---
src/agent.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/agent.c b/src/agent.c
index a340026..f67b30f 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -166,6 +166,9 @@ static int send_cancel_request(struct connman_agent *agent,
{
DBusMessage *message;
+ if (!request-driver)
+ return 0;
+
DBG(send cancel req to %s %s, agent-owner, agent-path);
message = dbus_message_new_method_call(agent-owner,
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] proxy: remove timeout on proxy cleanup
Make sure the timeout source is cleaned when proxy_lookup struct is
deallocated.
---
src/proxy.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/proxy.c b/src/proxy.c
index 0d1a25a..f331de9 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -44,6 +44,9 @@ struct proxy_lookup {
static void remove_lookup(struct proxy_lookup *lookup)
{
+ if (lookup-watch 0)
+ g_source_remove(lookup-watch);
+
lookup_list = g_slist_remove(lookup_list, lookup);
connman_service_unref(lookup-service);
@@ -150,11 +153,6 @@ void connman_proxy_lookup_cancel(unsigned int token)
}
if (lookup) {
- if (lookup-watch 0) {
- g_source_remove(lookup-watch);
- lookup-watch = 0;
- }
-
if (lookup-proxy
lookup-proxy-cancel_lookup)
lookup-proxy-cancel_lookup(lookup-service,
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH] agent: Check for pointer validity before cancelling agent request
On 22.10.2014 13:04, Patrik Flykt wrote: Mmmm, but the above can only happen in Jolla's version of ConnMan, there is/was commit 81195279ed07af08caf8c44fc390840b54b19ce2 in the Mer/Jolla tree that caused -driver to become NULL in some special circumstances. Ah, you're correct. Forgot about that when checking if I had something I hadn't upstreamed yet. BR, H. ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 0/3] Some memory issue fixes
Some fixes to memory issues I've encountered in testing, please review. These have all come up when testing connecting to and disconnecting from different networks while restarting wpa_supplicant and/or reloading wlan kernel module in the background, so that things can fail during pending operations for various reasons. The fix I made to wifi_tethering_info pointer to freed memory feels a bit clumsy to me, so feedback is appreciated. Hannu Mallat (3): gsupplicant: Fix D-Bus cancellation for interface creation wifi: set interface pointer to null on deallocation wifi: fix access to freed memory via wifi_tethering_info gsupplicant/supplicant.c | 2 +- plugins/wifi.c | 51 ++-- 2 files changed, 37 insertions(+), 16 deletions(-) -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 3/3] wifi: fix access to freed memory via wifi_tethering_info
If a wifi struct is deallocated during a pending D-Bus method call,
wifi_tethering_info may contain a pointer to the freed
memory. Reworked the code so that pointers to deallocated wifi structs
are cleared.
---
plugins/wifi.c | 47 +--
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/plugins/wifi.c b/plugins/wifi.c
index d18dc6c..ab23bb8 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -128,6 +128,15 @@ struct wifi_data {
int servicing;
};
+struct wifi_tethering_info {
+ struct wifi_data *wifi;
+ struct connman_technology *technology;
+ char *ifname;
+ GSupplicantSSID *ssid;
+};
+
+static GSList *tethering_info_list = NULL;
+
static GList *iface_list = NULL;
static GList *pending_wifi_device = NULL;
@@ -671,6 +680,7 @@ static void check_p2p_technology(void)
static void wifi_remove(struct connman_device *device)
{
struct wifi_data *wifi = connman_device_get_data(device);
+ GSList *t;
DBG(device %p wifi %p, device, wifi);
@@ -713,6 +723,12 @@ static void wifi_remove(struct connman_device *device)
g_free(wifi-autoscan);
g_free(wifi-identifier);
g_free(wifi);
+
+ for (t = tethering_info_list; t; t = t-next) {
+ struct wifi_tethering_info *info = t-data;
+ if (info-wifi == wifi)
+ info-wifi = NULL;
+ }
}
static bool is_duplicate(GSList *list, gchar *ssid, int ssid_len)
@@ -2678,13 +2694,6 @@ static void tech_remove(struct connman_technology
*technology)
wifi_technology = NULL;
}
-struct wifi_tethering_info {
- struct wifi_data *wifi;
- struct connman_technology *technology;
- char *ifname;
- GSupplicantSSID *ssid;
-};
-
static GSupplicantSSID *ssid_ap_init(const char *ssid, const char *passphrase)
{
GSupplicantSSID *ap;
@@ -2719,16 +2728,20 @@ static void ap_start_callback(int result,
GSupplicantInterface *interface,
struct wifi_tethering_info *info = user_data;
DBG(result %d index %d bridge %s,
- result, info-wifi-index, info-wifi-bridge);
+ result,
+ info-wifi ? info-wifi-index : -1,
+ info-wifi ? info-wifi-bridge : );
- if (result 0) {
- connman_inet_remove_from_bridge(info-wifi-index,
+ if (result 0 || info-wifi == NULL) {
+ if (info-wifi)
+ connman_inet_remove_from_bridge(info-wifi-index,
info-wifi-bridge);
connman_technology_tethering_notify(info-technology, false);
}
g_free(info-ifname);
g_free(info);
+ tethering_info_list = g_slist_remove(tethering_info_list, info);
}
static void ap_create_callback(int result,
@@ -2740,14 +2753,16 @@ static void ap_create_callback(int result,
DBG(result %d ifname %s, result,
g_supplicant_interface_get_ifname(interface));
- if (result 0) {
- connman_inet_remove_from_bridge(info-wifi-index,
+ if (result 0 || info-wifi == NULL) {
+ if (info-wifi)
+ connman_inet_remove_from_bridge(info-wifi-index,
info-wifi-bridge);
connman_technology_tethering_notify(info-technology, false);
g_free(info-ifname);
g_free(info-ssid);
g_free(info);
+ tethering_info_list = g_slist_remove(tethering_info_list, info);
return;
}
@@ -2770,12 +2785,14 @@ static void sta_remove_callback(int result,
DBG(ifname %s result %d , info-ifname, result);
- if (result 0) {
- info-wifi-tethering = true;
+ if (result 0 || info-wifi == NULL) {
+ if (info-wifi)
+ info-wifi-tethering = true;
g_free(info-ifname);
g_free(info-ssid);
g_free(info);
+ tethering_info_list = g_slist_remove(tethering_info_list, info);
return;
}
@@ -2857,6 +2874,8 @@ static int tech_set_tethering(struct connman_technology
*technology,
info-wifi-tethering = true;
+ tethering_info_list = g_slist_prepend(tethering_info_list,
info);
+
err = g_supplicant_interface_remove(interface,
sta_remove_callback,
info);
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 1/3] gsupplicant: Fix D-Bus cancellation for interface creation
Use null pointer as the caller argument for supplicant_dbus_property_get_all(), as the wifi plugin does not have the pointer to the interface struct until interface_create_callback() is called, and thus can't use it for D-Bus call cancellation until then. --- gsupplicant/supplicant.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c index b5e3930..93a9a6a 100644 --- a/gsupplicant/supplicant.c +++ b/gsupplicant/supplicant.c @@ -3393,7 +3393,7 @@ static void interface_create_result(const char *error, err = supplicant_dbus_property_get_all(path, SUPPLICANT_INTERFACE .Interface, interface_create_property, data, - data-interface); + NULL); if (err == 0) return; -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 2/3] wifi: set interface pointer to null on deallocation
As interface_removed() gets called when the interface struct is being deallocated, make sure the pointer is set to null regardless of the other fields' values in wifi struct. --- plugins/wifi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/plugins/wifi.c b/plugins/wifi.c index 45d88b8..d18dc6c 100644 --- a/plugins/wifi.c +++ b/plugins/wifi.c @@ -2284,6 +2284,9 @@ static void interface_removed(GSupplicantInterface *interface) wifi = g_supplicant_interface_get_data(interface); + if (wifi) + wifi-interface = NULL; + if (wifi wifi-tethering) return; @@ -2292,7 +2295,6 @@ static void interface_removed(GSupplicantInterface *interface) return; } - wifi-interface = NULL; connman_device_set_powered(wifi-device, false); check_p2p_technology(); -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
valgrind reports use of already freed memory for wifi-pending_network
Hi, came across the valgrind report below when trying to reproduce a connmand crash. Test was run against Jolla version of connmand (1.24 + some patches) so the line numbers are slightly off wrt upstream but the locations should be in the same ballpark. In the test I was periodically removing and reinstalling wlan kernel module, periodically restarting wpa supplicant (both simple sh loops) and semi-randomly connecting to and disconnecting from networks from the phone UI. Kernel module and wpa supplicant would be up barely enough to have time to scan and connect/disconnect a couple of times to some of the networks configured on the device, in an attempt to make connect/disconnect overlap with those special events. While the test is somewhat convoluted the root cause for the issue could perhaps be triggered by a simpler scenario too. It looks like wifi-pending_network has already been freed by the time the pointer is dereferenced. I get rid of this if I add reference counting to pending_network, but since reference counting it was previously removed (commit c8c5cd51) perhaps there's a more proper way to handle the issue. ==2725== Invalid read of size 4 ==2725==at 0x41520: connman_network_get_device (network.c:2161) ==2725==by 0x1FAF3: network_connect (wifi.c:1479) ==2725==by 0x217F3: disconnect_callback (wifi.c:1545) ==2725==by 0x24A5B: interface_disconnect_result (supplicant.c:4054) ==2725==by 0x29BBF: supplicant_dbus_method_call_cancel_all (dbus.c:451) ==2725==by 0x286F3: g_supplicant_interface_cancel (supplicant.c:2640) ==2725==by 0x2876B: signal_interface_removed (supplicant.c:2047) ==2725==by 0x2325F: g_supplicant_filter (supplicant.c:2630) ==2725==by 0x4976F47: dbus_connection_dispatch (dbus-connection.c:4631) ==2725==by 0x82653: message_dispatch (mainloop.c:72) ==2725==by 0x48A7A8B: g_idle_dispatch (gmain.c:5251) ==2725==by 0x48ABB1F: g_main_context_dispatch (gmain.c:3066) ==2725== Address 0x5730854 is 60 bytes inside a block of size 144 free'd ==2725==at 0x483752C: free (vg_replace_malloc.c:446) ==2725==by 0x48B36AB: g_free (gmem.c:197) ==2725==by 0x237B7: remove_network (supplicant.c:443) ==2725==by 0x48951E7: g_hash_table_remove_node (ghash.c:448) ==2725==by 0x48959D3: g_hash_table_remove_internal (ghash.c:1276) ==2725==by 0x25D8F: signal_bss_removed (supplicant.c:1803) ==2725==by 0x2325F: g_supplicant_filter (supplicant.c:2630) ==2725==by 0x4976F47: dbus_connection_dispatch (dbus-connection.c:4631) ==2725==by 0x82653: message_dispatch (mainloop.c:72) ==2725==by 0x48A7A8B: g_idle_dispatch (gmain.c:5251) ==2725==by 0x48ABB1F: g_main_context_dispatch (gmain.c:3066) ==2725==by 0x48ABE23: g_main_context_iterate.part.19 (gmain.c:3713) ==2725== BR, H. ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] dnsproxy: Check whether cache cleanup needs to be scheduled
server_create_socket() may fail for some reason before the cache hash table is created. In this case destroy_server() should not schedule a timeout for deallocating the cache; if it does, cache_refcount may become negative and an attempt to create the cache later on will not work properly, leading to a null pointer crash when trying to use the cache. --- src/dnsproxy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/dnsproxy.c b/src/dnsproxy.c index 741cd45..bdd7fd5 100644 --- a/src/dnsproxy.c +++ b/src/dnsproxy.c @@ -2204,7 +2204,8 @@ static void destroy_server(struct server_data *server) * without any good reason. The small delay allows the new RDNSS to * create a new DNS server instance and the refcount does not go to 0. */ - g_timeout_add_seconds(3, try_remove_cache, NULL); + if (cache) + g_timeout_add_seconds(3, try_remove_cache, NULL); g_free(server); } -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 2/2] unit: Add unit test for dnsproxy cache cleanup check
Unit test for checking that a nonexistent dnsproxy cache is not cleaned up.
---
Makefile.am | 9 ++-
unit/test-dnsproxy.c | 160 +++
2 files changed, 167 insertions(+), 2 deletions(-)
create mode 100644 unit/test-dnsproxy.c
diff --git a/Makefile.am b/Makefile.am
index a7f3ed3..b651d92 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -254,7 +254,8 @@ client_connmanctl_LDADD = gdbus/libgdbus-internal.la
@DBUS_LIBS@ @GLIB_LIBS@ \
-lreadline -ldl
endif
-noinst_PROGRAMS += unit/test-pbkdf2-sha1 unit/test-prf-sha1 unit/test-ippool
+noinst_PROGRAMS += unit/test-pbkdf2-sha1 unit/test-prf-sha1 unit/test-ippool \
+ unit/test-dnsproxy
unit_test_pbkdf2_sha1_SOURCES = unit/test-pbkdf2-sha1.c \
src/shared/sha1.h src/shared/sha1.c
@@ -269,7 +270,11 @@ unit_test_ippool_SOURCES = src/log.c src/dbus.c
src/error.c \
unit_test_ippool_LDADD = gdbus/libgdbus-internal.la \
@GLIB_LIBS@ @DBUS_LIBS@ -ldl
-TESTS = unit/test-pbkdf2-sha1 unit/test-prf-sha1 unit/test-ippool
+unit_test_dnsproxy_SOURCES = unit/test-dnsproxy.c src/log.c
+unit_test_dnsproxy_LDADD = @GLIB_LIBS@ -ldl
+
+TESTS = unit/test-pbkdf2-sha1 unit/test-prf-sha1 unit/test-ippool \
+ unit/test-dnsproxy
if WISPR
noinst_PROGRAMS += tools/wispr
diff --git a/unit/test-dnsproxy.c b/unit/test-dnsproxy.c
new file mode 100644
index 000..6435c2b
--- /dev/null
+++ b/unit/test-dnsproxy.c
@@ -0,0 +1,160 @@
+/*
+ *
+ * Connection Manager
+ *
+ * Copyright (C) 2014 Jolla Ltd. All rights reserved.
+ * Contact: Hannu Mallat hannu.mal...@jollamobile.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+/* Include source file to access static variables easily */
+#include src/dnsproxy.c
+
+static GMainLoop *main_loop = NULL;
+
+/* Stub getaddrinfo() to return test data */
+int getaddrinfo(const char *node, const char *service,
+ const struct addrinfo *hints,
+ struct addrinfo **res)
+{
+ struct addrinfo *ai = g_new0(struct addrinfo, 1);
+ ai-ai_socktype = hints-ai_socktype;
+ ai-ai_protocol = hints-ai_protocol;
+ if (hints-ai_family == AF_INET6) {
+ struct sockaddr_in6 *in6 = g_new0(struct sockaddr_in6, 1);
+ in6-sin6_family = AF_INET6;
+ in6-sin6_port = htons(53);
+ memcpy(in6-sin6_addr.s6_addr, 0123456789abcdef, 16);
+
+ ai-ai_family = AF_INET6;
+ ai-ai_addrlen = sizeof(struct sockaddr_in6);
+ ai-ai_addr = (struct sockaddr *)in6;
+ } else {
+ struct sockaddr_in *in = g_new0(struct sockaddr_in, 1);
+ in-sin_family = AF_INET;
+ in-sin_port = htons(53);
+ in-sin_addr.s_addr = htonl(0x12345678);
+
+ ai-ai_family = AF_INET6;
+ ai-ai_addrlen = sizeof(struct sockaddr_in);
+ ai-ai_addr = (struct sockaddr *)in;
+ }
+ ai-ai_canonname = g_strdup(node);
+ ai-ai_next = NULL;
+ *res = ai;
+
+ return 0;
+}
+
+void freeaddrinfo(struct addrinfo *res)
+{
+ if (res) {
+ if (res-ai_addr) {
+ g_free(res-ai_addr);
+ }
+ if (res-ai_canonname) {
+ g_free(res-ai_canonname);
+ }
+ g_free(res);
+ }
+}
+
+/* Stub socket() that always fails */
+int socket(int domain, int type, int protocol)
+{
+ return -1;
+}
+
+int connman_inet_ifindex(const char *name)
+{
+ return -1;
+}
+
+int __connman_service_get_index(struct connman_service *service)
+{
+ return -1;
+}
+
+bool __connman_service_index_is_default(int index)
+{
+ return FALSE;
+}
+
+bool __connman_service_index_is_split_routing(int index)
+{
+ return FALSE;
+}
+
+int __connman_resolvfile_append(int index, const char *domain, const char
*server)
+{
+ return -1;
+}
+
+int __connman_resolvfile_remove(int index, const char *domain, const char
*server)
+{
+ return -1;
+}
+
+int connman_notifier_register(struct connman_notifier *notifier)
+{
+ return 0;
+}
+
+void connman_notifier_unregister(struct connman_notifier *notifier)
+{
+}
+
+static gboolean
[PATCH 1/2] dnsproxy: Check whether cache cleanup needs to be scheduled
server_create_socket() may fail for some reason before the cache hash table is created. In this case destroy_server() should not schedule a timeout for deallocating the cache; if it does, cache_refcount may become negative and an attempt to create the cache later on will not work properly, leading to a null pointer crash when trying to use the cache. --- src/dnsproxy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/dnsproxy.c b/src/dnsproxy.c index 741cd45..bdd7fd5 100644 --- a/src/dnsproxy.c +++ b/src/dnsproxy.c @@ -2204,7 +2204,8 @@ static void destroy_server(struct server_data *server) * without any good reason. The small delay allows the new RDNSS to * create a new DNS server instance and the refcount does not go to 0. */ - g_timeout_add_seconds(3, try_remove_cache, NULL); + if (cache) + g_timeout_add_seconds(3, try_remove_cache, NULL); g_free(server); } -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 0/2] dnsproxy: Check whether cache cleanup needs to be scheduled
Re-sending the previous patch, this time with a unit test that checks a cleanup timeout isn't scheduled for nonexistent caches. It takes a few seconds to run the test, as it waits for the cleanup timeout to complete, but I didn't want to change the source under test just to speed up the test execution. Hannu Mallat (2): dnsproxy: Check whether cache cleanup needs to be scheduled unit: Add unit test for dnsproxy cache cleanup check Makefile.am | 9 ++- src/dnsproxy.c | 3 +- unit/test-dnsproxy.c | 160 +++ 3 files changed, 169 insertions(+), 3 deletions(-) create mode 100644 unit/test-dnsproxy.c -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 2/2] unit: Add unit test for dnsproxy cache cleanup check
Unit test for checking that a nonexistent dnsproxy cache is not cleaned up.
---
Makefile.am | 9 ++-
unit/test-dnsproxy.c | 192 +++
2 files changed, 199 insertions(+), 2 deletions(-)
create mode 100644 unit/test-dnsproxy.c
diff --git a/Makefile.am b/Makefile.am
index a7f3ed3..e0b44b9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -254,7 +254,8 @@ client_connmanctl_LDADD = gdbus/libgdbus-internal.la
@DBUS_LIBS@ @GLIB_LIBS@ \
-lreadline -ldl
endif
-noinst_PROGRAMS += unit/test-pbkdf2-sha1 unit/test-prf-sha1 unit/test-ippool
+noinst_PROGRAMS += unit/test-pbkdf2-sha1 unit/test-prf-sha1 unit/test-ippool \
+ unit/test-dnsproxy
unit_test_pbkdf2_sha1_SOURCES = unit/test-pbkdf2-sha1.c \
src/shared/sha1.h src/shared/sha1.c
@@ -269,7 +270,11 @@ unit_test_ippool_SOURCES = src/log.c src/dbus.c
src/error.c \
unit_test_ippool_LDADD = gdbus/libgdbus-internal.la \
@GLIB_LIBS@ @DBUS_LIBS@ -ldl
-TESTS = unit/test-pbkdf2-sha1 unit/test-prf-sha1 unit/test-ippool
+unit_test_dnsproxy_SOURCES = unit/test-dnsproxy.c src/log.c
+unit_test_dnsproxy_LDADD = @GLIB_LIBS@ -lresolv -ldl
+
+TESTS = unit/test-pbkdf2-sha1 unit/test-prf-sha1 unit/test-ippool \
+ unit/test-dnsproxy
if WISPR
noinst_PROGRAMS += tools/wispr
diff --git a/unit/test-dnsproxy.c b/unit/test-dnsproxy.c
new file mode 100644
index 000..ebe9587
--- /dev/null
+++ b/unit/test-dnsproxy.c
@@ -0,0 +1,192 @@
+/*
+ *
+ * Connection Manager
+ *
+ * Copyright (C) 2014 Jolla Ltd. All rights reserved.
+ * Contact: Hannu Mallat hannu.mal...@jollamobile.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+/* Include source file to access static variables easily */
+#include src/dnsproxy.c
+
+static GMainLoop *main_loop = NULL;
+
+/* Stub getaddrinfo() to return test data */
+int getaddrinfo(const char *node, const char *service,
+ const struct addrinfo *hints,
+ struct addrinfo **res)
+{
+ struct addrinfo *ai = g_new0(struct addrinfo, 1);
+ ai-ai_socktype = hints-ai_socktype;
+ ai-ai_protocol = hints-ai_protocol;
+ if (hints-ai_family == AF_INET6) {
+ struct sockaddr_in6 *in6 = g_new0(struct sockaddr_in6, 1);
+ in6-sin6_family = AF_INET6;
+ in6-sin6_port = htons(53);
+ memcpy(in6-sin6_addr.s6_addr, 0123456789abcdef, 16);
+
+ ai-ai_family = AF_INET6;
+ ai-ai_addrlen = sizeof(struct sockaddr_in6);
+ ai-ai_addr = (struct sockaddr *)in6;
+ } else {
+ struct sockaddr_in *in = g_new0(struct sockaddr_in, 1);
+ in-sin_family = AF_INET;
+ in-sin_port = htons(53);
+ in-sin_addr.s_addr = htonl(0x12345678);
+
+ ai-ai_family = AF_INET6;
+ ai-ai_addrlen = sizeof(struct sockaddr_in);
+ ai-ai_addr = (struct sockaddr *)in;
+ }
+ ai-ai_canonname = g_strdup(node);
+ ai-ai_next = NULL;
+ *res = ai;
+
+ return 0;
+}
+
+void freeaddrinfo(struct addrinfo *res)
+{
+ if (res) {
+ if (res-ai_addr) {
+ g_free(res-ai_addr);
+ }
+ if (res-ai_canonname) {
+ g_free(res-ai_canonname);
+ }
+ g_free(res);
+ }
+}
+
+/* Stub socket() that always fails */
+int socket(int domain, int type, int protocol)
+{
+ return -1;
+}
+
+GResolv *g_resolv_new(int index)
+{
+ return NULL;
+}
+
+bool g_resolv_set_address_family(GResolv *resolv, int family)
+{
+ return FALSE;
+}
+
+bool g_resolv_add_nameserver(GResolv *resolv, const char *address,
+ uint16_t port, unsigned long flags)
+{
+ return FALSE;
+}
+
+guint g_resolv_lookup_hostname(GResolv *resolv, const char *hostname,
+ GResolvResultFunc func, gpointer user_data)
+{
+ return 0;
+}
+
+char *connman_inet_ifname(int index)
+{
+ return NULL;
+}
+
+int connman_inet_ifindex(const char *name)
+{
+ return -1;
+}
+
+int __connman_inet_get_interface_address(int index, int family, void *address)
+{
+ return -1;
+}
+
+int
[PATCH 1/2] dnsproxy: Check whether cache cleanup needs to be scheduled
server_create_socket() may fail for some reason before the cache hash table is created. In this case destroy_server() should not schedule a timeout for deallocating the cache; if it does, cache_refcount may become negative and an attempt to create the cache later on will not work properly, leading to a null pointer crash when trying to use the cache. --- src/dnsproxy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/dnsproxy.c b/src/dnsproxy.c index 741cd45..bdd7fd5 100644 --- a/src/dnsproxy.c +++ b/src/dnsproxy.c @@ -2204,7 +2204,8 @@ static void destroy_server(struct server_data *server) * without any good reason. The small delay allows the new RDNSS to * create a new DNS server instance and the refcount does not go to 0. */ - g_timeout_add_seconds(3, try_remove_cache, NULL); + if (cache) + g_timeout_add_seconds(3, try_remove_cache, NULL); g_free(server); } -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH 0/2] dnsproxy: Check whether cache cleanup needs to be scheduled
One more reworking: unit test needs a few more stubs when compiling without optimization. Hannu Mallat (2): dnsproxy: Check whether cache cleanup needs to be scheduled unit: Add unit test for dnsproxy cache cleanup check Makefile.am | 9 ++- src/dnsproxy.c | 3 +- unit/test-dnsproxy.c | 192 +++ 3 files changed, 201 insertions(+), 3 deletions(-) create mode 100644 unit/test-dnsproxy.c -- 1.9.1 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] ipconfig: Do not pass a NULL pointer to D-Bus
connman_inet_ifname() may return a NULL pointer e.g. if the interface
does not exist in the kernel anymore but connman isn't yet aware of
that. Passing a NULL pointer to D-Bus may result in an abort, so check
the return value.
---
src/ipconfig.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/ipconfig.c b/src/ipconfig.c
index b7e4f94..fa4c0d6 100644
--- a/src/ipconfig.c
+++ b/src/ipconfig.c
@@ -2166,9 +2166,11 @@ void __connman_ipconfig_append_ethernet(struct
connman_ipconfig *ipconfig,
if (ipconfig-index = 0) {
char *ifname = connman_inet_ifname(ipconfig-index);
- connman_dbus_dict_append_basic(iter, Interface,
- DBUS_TYPE_STRING, ifname);
- g_free(ifname);
+ if (ifname) {
+ connman_dbus_dict_append_basic(iter, Interface,
+ DBUS_TYPE_STRING, ifname);
+ g_free(ifname);
+ }
}
if (ipdevice-address)
--
1.9.1
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] gsupplicant: Reassign best_bss pointer when that bss is removed
When the bss to which best_bss points to is removed, the pointer also
needs to be reassigned, or reference to already deallocated memory may
happen later on.
---
gsupplicant/supplicant.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index c6342d5..534944b 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -1824,7 +1824,7 @@ static void update_signal(gpointer key, gpointer value,
static void update_network_signal(GSupplicantNetwork *network)
{
- if (g_hash_table_size(network-bss_table) = 1)
+ if (g_hash_table_size(network-bss_table) = 1 network-best_bss)
return;
g_hash_table_foreach(network-bss_table,
@@ -1837,6 +1837,7 @@ static void interface_bss_removed(DBusMessageIter *iter,
void *user_data)
{
GSupplicantInterface *interface = user_data;
GSupplicantNetwork *network;
+ struct g_supplicant_bss *bss = NULL;
const char *path = NULL;
dbus_message_iter_get_basic(iter, path);
@@ -1847,6 +1848,12 @@ static void interface_bss_removed(DBusMessageIter *iter,
void *user_data)
if (!network)
return;
+ bss = g_hash_table_lookup(network-bss_table, path);
+ if (network-best_bss == bss) {
+ network-best_bss = NULL;
+ network-signal = 0;
+ }
+
g_hash_table_remove(bss_mapping, path);
g_hash_table_remove(interface-bss_mapping, path);
--
1.8.5.3
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 1/3] supplicant: Tie P2P flush D-Bus call to the interface
As the callback for the call accesses GSupplicantInterface structure,
tie the pending call to the interface pointer so that it gets cancelled
if the interface is removed.
---
gsupplicant/supplicant.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 2674298..0b42ce8 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -1964,7 +1964,7 @@ static void interface_added(DBusMessageIter *iter, void
*user_data)
supplicant_dbus_method_call(path,
SUPPLICANT_INTERFACE .Interface.P2PDevice, Flush,
- NULL, interface_p2p_flush, interface, NULL);
+ NULL, interface_p2p_flush, interface, interface);
dbus_message_iter_next(iter);
if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_INVALID) {
--
1.8.5.3
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 3/3] wifi: cancel pending supplicant operations when removing
If there are pending D-Bus calls made inside supplicant code when
wifi_remove() frees the wifi struct, wifi plugin callback functions
may attempt to use already released memory when the calls complete.
Fixed by cancelling any outstanding supplicant calls in wifi_remove().
---
gsupplicant/gsupplicant.h | 2 ++
gsupplicant/supplicant.c | 15 +--
plugins/wifi.c| 2 ++
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/gsupplicant/gsupplicant.h b/gsupplicant/gsupplicant.h
index a5ec405..7a3c843 100644
--- a/gsupplicant/gsupplicant.h
+++ b/gsupplicant/gsupplicant.h
@@ -172,6 +172,8 @@ typedef void (*GSupplicantInterfaceCallback) (int result,
GSupplicantInterface *interface,
void *user_data);
+void g_supplicant_interface_cancel(GSupplicantInterface *interface);
+
int g_supplicant_interface_create(const char *ifname, const char *driver,
const char *bridge,
GSupplicantInterfaceCallback callback,
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 7b0f4d4..ea68433 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -1990,9 +1990,7 @@ static void interface_removed(DBusMessageIter *iter, void
*user_data)
return;
interface = g_hash_table_lookup(interface_table, path);
- SUPPLICANT_DBG(Cancelling any pending DBus calls);
- supplicant_dbus_method_call_cancel_all(interface);
- supplicant_dbus_property_call_cancel_all(interface);
+ g_supplicant_interface_cancel(interface);
g_hash_table_remove(interface_table, path);
}
@@ -2582,6 +2580,13 @@ static DBusHandlerResult
g_supplicant_filter(DBusConnection *conn,
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
+void g_supplicant_interface_cancel(GSupplicantInterface *interface)
+{
+ SUPPLICANT_DBG(Cancelling any pending DBus calls);
+ supplicant_dbus_method_call_cancel_all(interface);
+ supplicant_dbus_property_call_cancel_all(interface);
+}
+
struct supplicant_regdom {
GSupplicantCountryCallback callback;
const char *alpha2;
@@ -2980,9 +2985,7 @@ int g_supplicant_interface_remove(GSupplicantInterface
*interface,
if (!system_available)
return -EFAULT;
- SUPPLICANT_DBG(Cancelling any pending DBus calls);
- supplicant_dbus_method_call_cancel_all(interface);
- supplicant_dbus_property_call_cancel_all(interface);
+ g_supplicant_interface_cancel(interface);
data = dbus_malloc0(sizeof(*data));
if (!data)
diff --git a/plugins/wifi.c b/plugins/wifi.c
index ef4dd95..aaa5b00 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -313,6 +313,8 @@ static void wifi_remove(struct connman_device *device)
g_supplicant_interface_set_data(wifi-interface, NULL);
+ g_supplicant_interface_cancel(wifi-interface);
+
if (wifi-scan_params)
g_supplicant_free_scan_params(wifi-scan_params);
--
1.8.5.3
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 2/3] supplicant: rewrite pending DBus call handling for properties
Property-related D-Bus calls may be pending replies when the related
GSupplicantInterface structure is cleaned up. This results in an
attempt to access already freed memory when calls complete.
Fixed by keeping a list of pending property calls, and canceling the
calls related to an interface when the interface is removed. This is a
similar change to the one made earlier for pending method calls.
---
gsupplicant/dbus.c | 140 ++-
gsupplicant/dbus.h | 12 ++--
gsupplicant/supplicant.c | 33 ++-
3 files changed, 116 insertions(+), 69 deletions(-)
diff --git a/gsupplicant/dbus.c b/gsupplicant/dbus.c
index 5b15bcd..130306e 100644
--- a/gsupplicant/dbus.c
+++ b/gsupplicant/dbus.c
@@ -59,10 +59,35 @@ static int find_method_call_by_caller(gconstpointer a,
gconstpointer b)
return method_call-caller != caller;
}
+static GSList *property_calls;
+
+struct property_call_data {
+ gpointer caller;
+ DBusPendingCall *pending_call;
+ supplicant_dbus_property_function function;
+ void *user_data;
+};
+
+static void property_call_free(void *pointer)
+{
+ struct property_call_data *property_call = pointer;
+ property_calls = g_slist_remove(property_calls, property_call);
+ g_free(property_call);
+}
+
+static int find_property_call_by_caller(gconstpointer a, gconstpointer b)
+{
+ const struct property_call_data *property_call = a;
+ gconstpointer caller = b;
+
+ return property_call-caller != caller;
+}
+
void supplicant_dbus_setup(DBusConnection *conn)
{
connection = conn;
method_calls = NULL;
+ property_calls = NULL;
}
void supplicant_dbus_array_foreach(DBusMessageIter *iter,
@@ -124,14 +149,27 @@ void supplicant_dbus_property_foreach(DBusMessageIter
*iter,
}
}
-struct property_get_data {
- supplicant_dbus_property_function function;
- void *user_data;
-};
+void supplicant_dbus_property_call_cancel_all(gpointer caller)
+{
+ while (property_calls) {
+ struct property_call_data *property_call;
+ GSList *elem = g_slist_find_custom(property_calls, caller,
+ find_property_call_by_caller);
+ if (!elem)
+ break;
+
+ property_call = elem-data;
+ property_calls = g_slist_delete_link(property_calls, elem);
+
+ dbus_pending_call_cancel(property_call-pending_call);
+
+ dbus_pending_call_unref(property_call-pending_call);
+ }
+}
static void property_get_all_reply(DBusPendingCall *call, void *user_data)
{
- struct property_get_data *data = user_data;
+ struct property_call_data *property_call = user_data;
DBusMessage *reply;
DBusMessageIter iter;
@@ -143,11 +181,11 @@ static void property_get_all_reply(DBusPendingCall *call,
void *user_data)
if (!dbus_message_iter_init(reply, iter))
goto done;
- supplicant_dbus_property_foreach(iter, data-function,
- data-user_data);
+ supplicant_dbus_property_foreach(iter, property_call-function,
+ property_call-user_data);
- if (data-function)
- data-function(NULL, NULL, data-user_data);
+ if (property_call-function)
+ property_call-function(NULL, NULL, property_call-user_data);
done:
dbus_message_unref(reply);
@@ -157,9 +195,9 @@ done:
int supplicant_dbus_property_get_all(const char *path, const char *interface,
supplicant_dbus_property_function function,
- void *user_data)
+ void *user_data, gpointer caller)
{
- struct property_get_data *data;
+ struct property_call_data *property_call = NULL;
DBusMessage *message;
DBusPendingCall *call;
@@ -169,14 +207,14 @@ int supplicant_dbus_property_get_all(const char *path,
const char *interface,
if (!path || !interface)
return -EINVAL;
- data = dbus_malloc0(sizeof(*data));
- if (!data)
+ property_call = g_try_new0(struct property_call_data, 1);
+ if (!property_call)
return -ENOMEM;
message = dbus_message_new_method_call(SUPPLICANT_SERVICE, path,
DBUS_INTERFACE_PROPERTIES, GetAll);
if (!message) {
- dbus_free(data);
+ g_free(property_call);
return -ENOMEM;
}
@@ -187,21 +225,23 @@ int supplicant_dbus_property_get_all(const char *path,
const char *interface,
if (!dbus_connection_send_with_reply(connection, message,
call, TIMEOUT)) {
dbus_message_unref(message);
-
[PATCH 3/3] wifi: cancel pending supplicant operations when removing
If there are pending D-Bus calls made inside supplicant code when
wifi_remove() frees the wifi struct, wifi plugin callback functions
may attempt to use already released memory when the calls complete.
Fixed by cancelling any outstanding supplicant calls in wifi_remove().
---
gsupplicant/gsupplicant.h | 2 ++
gsupplicant/supplicant.c | 15 +--
plugins/wifi.c| 2 ++
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/gsupplicant/gsupplicant.h b/gsupplicant/gsupplicant.h
index a5ec405..7a3c843 100644
--- a/gsupplicant/gsupplicant.h
+++ b/gsupplicant/gsupplicant.h
@@ -172,6 +172,8 @@ typedef void (*GSupplicantInterfaceCallback) (int result,
GSupplicantInterface *interface,
void *user_data);
+void g_supplicant_interface_cancel(GSupplicantInterface *interface);
+
int g_supplicant_interface_create(const char *ifname, const char *driver,
const char *bridge,
GSupplicantInterfaceCallback callback,
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 7b0f4d4..ea68433 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -1990,9 +1990,7 @@ static void interface_removed(DBusMessageIter *iter, void
*user_data)
return;
interface = g_hash_table_lookup(interface_table, path);
- SUPPLICANT_DBG(Cancelling any pending DBus calls);
- supplicant_dbus_method_call_cancel_all(interface);
- supplicant_dbus_property_call_cancel_all(interface);
+ g_supplicant_interface_cancel(interface);
g_hash_table_remove(interface_table, path);
}
@@ -2582,6 +2580,13 @@ static DBusHandlerResult
g_supplicant_filter(DBusConnection *conn,
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
+void g_supplicant_interface_cancel(GSupplicantInterface *interface)
+{
+ SUPPLICANT_DBG(Cancelling any pending DBus calls);
+ supplicant_dbus_method_call_cancel_all(interface);
+ supplicant_dbus_property_call_cancel_all(interface);
+}
+
struct supplicant_regdom {
GSupplicantCountryCallback callback;
const char *alpha2;
@@ -2980,9 +2985,7 @@ int g_supplicant_interface_remove(GSupplicantInterface
*interface,
if (!system_available)
return -EFAULT;
- SUPPLICANT_DBG(Cancelling any pending DBus calls);
- supplicant_dbus_method_call_cancel_all(interface);
- supplicant_dbus_property_call_cancel_all(interface);
+ g_supplicant_interface_cancel(interface);
data = dbus_malloc0(sizeof(*data));
if (!data)
diff --git a/plugins/wifi.c b/plugins/wifi.c
index ef4dd95..aaa5b00 100644
--- a/plugins/wifi.c
+++ b/plugins/wifi.c
@@ -313,6 +313,8 @@ static void wifi_remove(struct connman_device *device)
g_supplicant_interface_set_data(wifi-interface, NULL);
+ g_supplicant_interface_cancel(wifi-interface);
+
if (wifi-scan_params)
g_supplicant_free_scan_params(wifi-scan_params);
--
1.8.5.3
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH 2/3] supplicant: rewrite pending DBus call handling for properties
Property-related D-Bus calls may be pending replies when the related
GSupplicantInterface structure is cleaned up. This results in an
attempt to access already freed memory when calls complete.
Fixed by keeping a list of pending property calls, and canceling the
calls related to an interface when the interface is removed. This is a
similar change to the one made earlier for pending method calls.
---
gsupplicant/dbus.c | 140 ++-
gsupplicant/dbus.h | 12 ++--
gsupplicant/supplicant.c | 33 ++-
3 files changed, 116 insertions(+), 69 deletions(-)
diff --git a/gsupplicant/dbus.c b/gsupplicant/dbus.c
index 5b15bcd..130306e 100644
--- a/gsupplicant/dbus.c
+++ b/gsupplicant/dbus.c
@@ -59,10 +59,35 @@ static int find_method_call_by_caller(gconstpointer a,
gconstpointer b)
return method_call-caller != caller;
}
+static GSList *property_calls;
+
+struct property_call_data {
+ gpointer caller;
+ DBusPendingCall *pending_call;
+ supplicant_dbus_property_function function;
+ void *user_data;
+};
+
+static void property_call_free(void *pointer)
+{
+ struct property_call_data *property_call = pointer;
+ property_calls = g_slist_remove(property_calls, property_call);
+ g_free(property_call);
+}
+
+static int find_property_call_by_caller(gconstpointer a, gconstpointer b)
+{
+ const struct property_call_data *property_call = a;
+ gconstpointer caller = b;
+
+ return property_call-caller != caller;
+}
+
void supplicant_dbus_setup(DBusConnection *conn)
{
connection = conn;
method_calls = NULL;
+ property_calls = NULL;
}
void supplicant_dbus_array_foreach(DBusMessageIter *iter,
@@ -124,14 +149,27 @@ void supplicant_dbus_property_foreach(DBusMessageIter
*iter,
}
}
-struct property_get_data {
- supplicant_dbus_property_function function;
- void *user_data;
-};
+void supplicant_dbus_property_call_cancel_all(gpointer caller)
+{
+ while (property_calls) {
+ struct property_call_data *property_call;
+ GSList *elem = g_slist_find_custom(property_calls, caller,
+ find_property_call_by_caller);
+ if (!elem)
+ break;
+
+ property_call = elem-data;
+ property_calls = g_slist_delete_link(property_calls, elem);
+
+ dbus_pending_call_cancel(property_call-pending_call);
+
+ dbus_pending_call_unref(property_call-pending_call);
+ }
+}
static void property_get_all_reply(DBusPendingCall *call, void *user_data)
{
- struct property_get_data *data = user_data;
+ struct property_call_data *property_call = user_data;
DBusMessage *reply;
DBusMessageIter iter;
@@ -143,11 +181,11 @@ static void property_get_all_reply(DBusPendingCall *call,
void *user_data)
if (!dbus_message_iter_init(reply, iter))
goto done;
- supplicant_dbus_property_foreach(iter, data-function,
- data-user_data);
+ supplicant_dbus_property_foreach(iter, property_call-function,
+ property_call-user_data);
- if (data-function)
- data-function(NULL, NULL, data-user_data);
+ if (property_call-function)
+ property_call-function(NULL, NULL, property_call-user_data);
done:
dbus_message_unref(reply);
@@ -157,9 +195,9 @@ done:
int supplicant_dbus_property_get_all(const char *path, const char *interface,
supplicant_dbus_property_function function,
- void *user_data)
+ void *user_data, gpointer caller)
{
- struct property_get_data *data;
+ struct property_call_data *property_call = NULL;
DBusMessage *message;
DBusPendingCall *call;
@@ -169,14 +207,14 @@ int supplicant_dbus_property_get_all(const char *path,
const char *interface,
if (!path || !interface)
return -EINVAL;
- data = dbus_malloc0(sizeof(*data));
- if (!data)
+ property_call = g_try_new0(struct property_call_data, 1);
+ if (!property_call)
return -ENOMEM;
message = dbus_message_new_method_call(SUPPLICANT_SERVICE, path,
DBUS_INTERFACE_PROPERTIES, GetAll);
if (!message) {
- dbus_free(data);
+ g_free(property_call);
return -ENOMEM;
}
@@ -187,21 +225,23 @@ int supplicant_dbus_property_get_all(const char *path,
const char *interface,
if (!dbus_connection_send_with_reply(connection, message,
call, TIMEOUT)) {
dbus_message_unref(message);
-
[PATCH 1/3] supplicant: Tie P2P flush D-Bus call to the interface
As the callback for the call accesses GSupplicantInterface structure,
tie the pending call to the interface pointer so that it gets cancelled
if the interface is removed.
---
gsupplicant/supplicant.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gsupplicant/supplicant.c b/gsupplicant/supplicant.c
index 2674298..0b42ce8 100644
--- a/gsupplicant/supplicant.c
+++ b/gsupplicant/supplicant.c
@@ -1964,7 +1964,7 @@ static void interface_added(DBusMessageIter *iter, void
*user_data)
supplicant_dbus_method_call(path,
SUPPLICANT_INTERFACE .Interface.P2PDevice, Flush,
- NULL, interface_p2p_flush, interface, NULL);
+ NULL, interface_p2p_flush, interface, interface);
dbus_message_iter_next(iter);
if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_INVALID) {
--
1.8.5.3
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] service: helper function for sorting services
Created a helper function to sort service list, to shorten the code
slightly and to ensure null pointer check is always done before using
the list pointer.
---
src/service.c | 41 ++---
1 file changed, 14 insertions(+), 27 deletions(-)
diff --git a/src/service.c b/src/service.c
index 6a70777..d586147 100644
--- a/src/service.c
+++ b/src/service.c
@@ -4729,6 +4729,14 @@ static gint service_compare(gconstpointer a,
gconstpointer b)
return g_strcmp0(service_a-name, service_b-name);
}
+static void service_list_sort(void)
+{
+ if (service_list service_list-next) {
+ service_list = g_list_sort(service_list, service_compare);
+ service_schedule_changed();
+ }
+}
+
/**
* connman_service_get_type:
* @service: service structure
@@ -4896,11 +4904,7 @@ int __connman_service_set_favorite_delayed(struct
connman_service *service,
if (!delay_ordering) {
- if (service_list-next) {
- service_list = g_list_sort(service_list,
- service_compare);
- service_schedule_changed();
- }
+ service_list_sort();
__connman_connection_update_gateway();
}
@@ -5449,10 +5453,7 @@ static int service_indicate_state(struct connman_service
*service)
} else
set_error(service, CONNMAN_SERVICE_ERROR_UNKNOWN);
- if (service_list-next) {
- service_list = g_list_sort(service_list, service_compare);
- service_schedule_changed();
- }
+ service_list_sort();
__connman_connection_update_gateway();
@@ -6159,11 +6160,7 @@ int __connman_service_provision_changed(const char
*ident)
if (services_dirty) {
services_dirty = false;
- if (service_list-next) {
- service_list = g_list_sort(service_list,
- service_compare);
- service_schedule_changed();
- }
+ service_list_sort();
__connman_connection_update_gateway();
}
@@ -6237,10 +6234,7 @@ static int service_register(struct connman_service
*service)
service_methods, service_signals,
NULL, service, NULL);
- if (service_list-next) {
- service_list = g_list_sort(service_list, service_compare);
- service_schedule_changed();
- }
+ service_list_sort();
__connman_connection_update_gateway();
@@ -6669,10 +6663,7 @@ static void update_from_network(struct connman_service
*service,
if (!service-network)
service-network = connman_network_ref(network);
- if (service_list-next) {
- service_list = g_list_sort(service_list, service_compare);
- service_schedule_changed();
- }
+ service_list_sort();
}
/**
@@ -6834,11 +6825,7 @@ roaming:
sorting:
if (need_sort) {
- if (service_list-next) {
- service_list = g_list_sort(service_list,
- service_compare);
- service_schedule_changed();
- }
+ service_list_sort();
}
}
--
1.8.5.3
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] wifi: cancel autoscan timeout when removing device
Usually autoscan is cancelled when the device is disabled, but when wpa_supplicant leaves D-Bus while device has an ongoing power request, __connman_device_disable() will return early and thus autoscan timeout isn't cleared. The timeout callback may then access released memory when it's activated. Add stop_autoscan() call to wifi_remove() to make sure there are no pending timeouts when the autoscan struct is released. --- plugins/wifi.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/wifi.c b/plugins/wifi.c index c1da04b..ef4dd95 100644 --- a/plugins/wifi.c +++ b/plugins/wifi.c @@ -293,6 +293,8 @@ static void wifi_remove(struct connman_device *device) if (!wifi) return; + stop_autoscan(device); + iface_list = g_list_remove(iface_list, wifi); check_p2p_technology(); -- 1.8.5.3 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
[PATCH] supplicant: rewrite pending DBus call handling
It is possible to overwrite the pending_call and pending_slot fields
in the GSupplicantInterface structure by making a new supplicant
related DBus call while an older call is still ongoing. This causes
problems with cleanup at interface removal time, as the older call is
not canceled and its reply processing may access already freed memory.
Fixed by having supplicant DBus code keep track of pending calls in a
list which is updated when call replies are processed. Supplicant code
can request cancellation of all pending calls for a particular
interface when needed.
---
gsupplicant/dbus.c | 98 +---
gsupplicant/dbus.h | 6 +--
gsupplicant/supplicant.c | 82 +---
3 files changed, 80 insertions(+), 106 deletions(-)
diff --git a/gsupplicant/dbus.c b/gsupplicant/dbus.c
index 27a132a..5b15bcd 100644
--- a/gsupplicant/dbus.c
+++ b/gsupplicant/dbus.c
@@ -35,9 +35,34 @@
static DBusConnection *connection;
+static GSList *method_calls;
+
+struct method_call_data {
+ gpointer caller;
+ DBusPendingCall *pending_call;
+ supplicant_dbus_result_function function;
+ void *user_data;
+};
+
+static void method_call_free(void *pointer)
+{
+ struct method_call_data *method_call = pointer;
+ method_calls = g_slist_remove(method_calls, method_call);
+ g_free(method_call);
+}
+
+static int find_method_call_by_caller(gconstpointer a, gconstpointer b)
+{
+ const struct method_call_data *method_call = a;
+ gconstpointer caller = b;
+
+ return method_call-caller != caller;
+}
+
void supplicant_dbus_setup(DBusConnection *conn)
{
connection = conn;
+ method_calls = NULL;
}
void supplicant_dbus_array_foreach(DBusMessageIter *iter,
@@ -362,27 +387,31 @@ int supplicant_dbus_property_set(const char *path, const
char *interface,
return 0;
}
-struct method_call_data {
- supplicant_dbus_result_function function;
- void *user_data;
-};
-
-void supplicant_dbus_call_callback(DBusPendingCall *call, dbus_int32_t slot)
+void supplicant_dbus_method_call_cancel_all(gpointer caller)
{
- struct method_call_data *data;
+ while (method_calls) {
+ struct method_call_data *method_call;
+ GSList *elem = g_slist_find_custom(method_calls, caller,
+ find_method_call_by_caller);
+ if (!elem)
+ break;
- data = dbus_pending_call_get_data(call, slot);
- if (data data-function)
- data-function(net.connman.Error.OperationAborted,
- NULL, data-user_data);
+ method_call = elem-data;
+ method_calls = g_slist_delete_link(method_calls, elem);
- dbus_pending_call_free_data_slot(slot);
- dbus_pending_call_unref(call);
+ dbus_pending_call_cancel(method_call-pending_call);
+
+ if (method_call-function)
+
method_call-function(net.connman.Error.OperationAborted,
+ NULL, method_call-user_data);
+
+ dbus_pending_call_unref(method_call-pending_call);
+ }
}
static void method_call_reply(DBusPendingCall *call, void *user_data)
{
- struct method_call_data *data;
+ struct method_call_data *method_call = user_data;
DBusMessage *reply;
DBusMessageIter iter;
const char *error;
@@ -396,9 +425,8 @@ static void method_call_reply(DBusPendingCall *call, void
*user_data)
dbus_message_iter_init(reply, iter);
- data = dbus_pending_call_get_data(call, GPOINTER_TO_INT(user_data));
- if (data data-function)
- data-function(error, iter, data-user_data);
+ if (method_call method_call-function)
+ method_call-function(error, iter, method_call-user_data);
dbus_message_unref(reply);
@@ -410,14 +438,12 @@ int supplicant_dbus_method_call(const char *path,
supplicant_dbus_setup_function setup,
supplicant_dbus_result_function function,
void *user_data,
- DBusPendingCall **pending_call,
- dbus_int32_t *data_slot)
+ gpointer caller)
{
- struct method_call_data *data;
+ struct method_call_data *method_call = NULL;
DBusMessage *message;
DBusMessageIter iter;
DBusPendingCall *call;
- dbus_int32_t slot = -1;
if (!connection)
return -EINVAL;
@@ -425,14 +451,14 @@ int supplicant_dbus_method_call(const char *path,
if (!path || !interface || !method)
return -EINVAL;
- data = dbus_malloc0(sizeof(*data));
- if (!data)
+ method_call = g_try_new0(struct method_call_data, 1);
+ if
[PATCH] agent: fix a double free in agent message handling
It could happen that cleaning up the pending message in
agent_receive_message() would as a side effect clear the last
reference to the service via driver context unref function pointer.
Service cleanup would then call connman_agent_cancel(), which would
try to clean up the pending message for the second time as the pending
pointer was still non-NULL.
Fixed by decoupling the pointer to the pending request from the agent
structure before calling agent_request_free().
---
src/agent.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/src/agent.c b/src/agent.c
index b9b8dd9..37cf524 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -112,6 +112,17 @@ static void agent_request_free(struct
connman_agent_request *request)
g_free(request);
}
+static void agent_finalize_pending(struct connman_agent *agent,
+ DBusMessage *reply)
+{
+ struct connman_agent_request *pending = agent-pending;
+ if (pending) {
+ agent-pending = NULL;
+ pending-callback(reply, pending-user_data);
+ agent_request_free(pending);
+ }
+}
+
static void agent_receive_message(DBusPendingCall *call, void *user_data);
static int agent_send_next_request(struct connman_agent *agent)
@@ -146,9 +157,7 @@ static int agent_send_next_request(struct connman_agent
*agent)
return 0;
fail:
- agent-pending-callback(NULL, agent-pending-user_data);
- agent_request_free(agent-pending);
- agent-pending = NULL;
+ agent_finalize_pending(agent, NULL);
return -ESRCH;
}
@@ -191,12 +200,9 @@ static void agent_receive_message(DBusPendingCall *call,
void *user_data)
send_cancel_request(agent, agent-pending);
}
- agent-pending-callback(reply, agent-pending-user_data);
+ agent_finalize_pending(agent, reply);
dbus_message_unref(reply);
- agent_request_free(agent-pending);
- agent-pending = NULL;
-
err = agent_send_next_request(agent);
if (err 0 err != -EBUSY)
DBG(send next request failed (%s/%d), strerror(-err), -err);
@@ -456,9 +462,7 @@ static void cancel_all_requests(struct connman_agent *agent)
if (agent-pending-call)
send_cancel_request(agent, agent-pending);
- agent-pending-callback(NULL, agent-pending-user_data);
- agent_request_free(agent-pending);
- agent-pending = NULL;
+ agent_finalize_pending(agent, NULL);
}
for (list = agent-queue; list; list = list-next) {
@@ -521,12 +525,7 @@ void connman_agent_cancel(void *user_context)
if (agent-pending-call)
send_cancel_request(agent, agent-pending);
- agent-pending-callback(NULL,
- agent-pending-user_data);
-
- agent_request_free(agent-pending);
-
- agent-pending = NULL;
+ agent_finalize_pending(agent, NULL);
err = agent_send_next_request(agent);
if (err 0 err != -EBUSY)
--
1.8.5.3
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] agent: check for NULL parameter
Check for reply parameter being NULL before using it.
---
src/agent-connman.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/agent-connman.c b/src/agent-connman.c
index 203edf0..ab538f3 100644
--- a/src/agent-connman.c
+++ b/src/agent-connman.c
@@ -354,6 +354,9 @@ static void request_input_login_reply(DBusMessage *reply,
void *user_data)
char *key;
DBusMessageIter iter, dict;
+ if (!reply)
+ goto out;
+
if (dbus_message_get_type(reply) == DBUS_MESSAGE_TYPE_ERROR) {
error = dbus_message_get_error_name(reply);
goto done;
@@ -401,6 +404,8 @@ done:
username, password,
FALSE, NULL, error,
username_password_reply-user_data);
+
+out:
g_free(username_password_reply);
}
--
1.8.5.3
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
[PATCH] wispr: Handle wispr redirect properly
Return true if agent invocation is successful after receiving a valid wispr redirect message. Otherwise wispr_portal_web_result() will proceed based on HTTP status code. --- src/wispr.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/wispr.c b/src/wispr.c index 32cf75a..733debd 100644 --- a/src/wispr.c +++ b/src/wispr.c @@ -646,6 +646,8 @@ static bool wispr_manage_message(GWebResult *result, wispr_portal_request_wispr_login, wp_context) != -EINPROGRESS) wispr_portal_error(wp_context); + else + return true; break; case 120: /* Falling down */ -- 1.8.5.3 ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
