[PATCH] hfp_hf: Fix modifying hash table while iterating

[PATCH] hfp_hf: Fix modifying hash table while iterating

[Mikel Astiz]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

Calling ofono_remove_modem() while iterating the hash table is not safe
given that it can modify the table in hfp_remove().

A simple way to reproduce the problem is to pair some Bluetooth phones
and remove the Bluetooth adapter, triggering a GLib-CRITICAL assertion.

This approach proposes a two-step removal: first, the hash table is
iterated and all modems to be removed are marked as pending (thus
removed from the hash table and put in a temporary list), and afterwards
all pending modems are actually removed.
---
 plugins/hfp_hf.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/plugins/hfp_hf.c b/plugins/hfp_hf.c
index 48a734a..0028e21 100644
--- a/plugins/hfp_hf.c
+++ b/plugins/hfp_hf.c
@@ -60,6 +60,7 @@
 
 static DBusConnection *connection;
 static GHashTable *modem_hash = NULL;
+static GSList *pending_removals = NULL;
 
 struct hfp_data {
 	struct hfp_slc_info info;
@@ -265,11 +266,16 @@ static gboolean hfp_remove_modem(gpointer key, gpointer value,
 	if (prefix && g_str_has_prefix(device, prefix) == FALSE)
 		return FALSE;
 
-	ofono_modem_remove(modem);
+	pending_removals = g_slist_append(pending_removals, modem);
 
 	return TRUE;
 }
 
+static void hfp_remove_pending_modem(gpointer data)
+{
+	ofono_modem_remove(data);
+}
+
 static void hfp_hf_remove(const char *prefix)
 {
 	DBG("%s", prefix);
@@ -279,6 +285,9 @@ static void hfp_hf_remove(const char *prefix)
 
 	g_hash_table_foreach_remove(modem_hash, hfp_remove_modem,
 							(gpointer) prefix);
+
+	g_slist_free_full(pending_removals, hfp_remove_pending_modem);
+	pending_removals = NULL;
 }
 
 static void hfp_hf_set_alias(const char *device, const char *alias)
-- 
1.7.7.6

Re: [PATCH] hfp_hf: Fix modifying hash table while iterating

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

Hi Mikel,

On 04/18/2012 10:19 AM, Mikel Astiz wrote:
> Calling ofono_remove_modem() while iterating the hash table is not safe
> given that it can modify the table in hfp_remove().
> 
> A simple way to reproduce the problem is to pair some Bluetooth phones
> and remove the Bluetooth adapter, triggering a GLib-CRITICAL assertion.
> 
> This approach proposes a two-step removal: first, the hash table is
> iterated and all modems to be removed are marked as pending (thus
> removed from the hash table and put in a temporary list), and afterwards
> all pending modems are actually removed.
> ---
>  plugins/hfp_hf.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 

The patch looks absolutely fine, however can you check whether removing
g_hashtable_remove from hfp_remove would do the trick as well?  We don't
seem to be doing this in the sap driver, and I no longer recall whether
this was done in hfp_hf to keep valgrind happy or is simply a mistake.

Regards,
-Denis

Re: [PATCH] hfp_hf: Fix modifying hash table while iterating

[Mikel Astiz]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

Hi Denis,

On Wed, Apr 18, 2012 at 5:55 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Mikel,
>
> On 04/18/2012 10:19 AM, Mikel Astiz wrote:
>> Calling ofono_remove_modem() while iterating the hash table is not safe
>> given that it can modify the table in hfp_remove().
>>
>> A simple way to reproduce the problem is to pair some Bluetooth phones
>> and remove the Bluetooth adapter, triggering a GLib-CRITICAL assertion.
>>
>> This approach proposes a two-step removal: first, the hash table is
>> iterated and all modems to be removed are marked as pending (thus
>> removed from the hash table and put in a temporary list), and afterwards
>> all pending modems are actually removed.
>> ---
>>  plugins/hfp_hf.c |   11 ++++++++++-
>>  1 files changed, 10 insertions(+), 1 deletions(-)
>>
>
> The patch looks absolutely fine, however can you check whether removing
> g_hashtable_remove from hfp_remove would do the trick as well?  We don't
> seem to be doing this in the sap driver, and I no longer recall whether
> this was done in hfp_hf to keep valgrind happy or is simply a mistake.

I was reluctant to make such a change because I wasn't entirely sure about
which assumptions are made by the core about modem removals.

If we know that all calls to ofono_modem_remove() will be originated in
hfp_hf.c, then your proposal would just work. I will send the corresponding
patch soon.

Cheers,
Mikel

Re: [PATCH] hfp_hf: Fix modifying hash table while iterating

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

Hi Mikel,

>> The patch looks absolutely fine, however can you check whether removing
>> g_hashtable_remove from hfp_remove would do the trick as well?  We don't
>> seem to be doing this in the sap driver, and I no longer recall whether
>> this was done in hfp_hf to keep valgrind happy or is simply a mistake.
> 
> I was reluctant to make such a change because I wasn't entirely sure about
> which assumptions are made by the core about modem removals.
> 
> If we know that all calls to ofono_modem_remove() will be originated in
> hfp_hf.c, then your proposal would just work. I will send the corresponding
> patch soon.

The core does not call ofono_modem_remove on its own, this is triggered
by the plugins (e.g. udev, bluetooth).  So Under normal circumstances
all calls to ofono_modem_remove would be triggered by BlueZ reporting
something, e.g. adapter removed, device removed, etc.

The only time this might be a problem is during shutdown, hence my
comment about keeping valgrind happy.

Regards,
-Denis

Re: [PATCH] hfp_hf: Fix modifying hash table while iterating

[Mikel Astiz]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

Hi Denis,

On Thu, Apr 19, 2012 at 2:55 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Mikel,
>
>>> The patch looks absolutely fine, however can you check whether removing
>>> g_hashtable_remove from hfp_remove would do the trick as well?  We don't
>>> seem to be doing this in the sap driver, and I no longer recall whether
>>> this was done in hfp_hf to keep valgrind happy or is simply a mistake.
>>
>> I was reluctant to make such a change because I wasn't entirely sure about
>> which assumptions are made by the core about modem removals.
>>
>> If we know that all calls to ofono_modem_remove() will be originated in
>> hfp_hf.c, then your proposal would just work. I will send the corresponding
>> patch soon.
>
> The core does not call ofono_modem_remove on its own, this is triggered
> by the plugins (e.g. udev, bluetooth).  So Under normal circumstances
> all calls to ofono_modem_remove would be triggered by BlueZ reporting
> something, e.g. adapter removed, device removed, etc.

In that case the second version that I proposed should work.

> The only time this might be a problem is during shutdown, hence my
> comment about keeping valgrind happy.

During shutdown the table is freed inside hfp_exit, so that shouldn't
be a problem.

Cheers,
Mikel