[PATCH v3 0/2] Fix change to numbers GSlist in new missed calls

[PATCH v3 0/2] Fix change to numbers GSlist in new missed calls

[Dmitriy Paliy]

Hi,

Commit message is updated in this version to describe problem more correctly.

Br,
Dmitriy

[PATCH v3 1/2] Fix change to numbers GSlist in new missed calls

[Dmitriy Paliy]

It is possible that phonebook_pull_read is invoked several times
submitting multiple pull requests without closing PBAP object. E.g., when
history is large enough and maxlistcount>VCARDS_PART_COUNT.

The result is possibility of different data structures (GString and
contact_data) to be mixed in a single GSlist that may lead to undefined
behaviour.
---
 plugins/phonebook-tracker.c |   57 ++++++++++++++++++++++++-------------------
 1 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index eec1e5d..95908ba 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -465,6 +465,7 @@ struct phonebook_data {
 	gboolean vcardentry;
 	const struct apparam_field *params;
 	GSList *contacts;
+	GSList *numbers;
 	phonebook_cache_ready_cb ready_cb;
 	phonebook_entry_cb entry_cb;
 	int newmissedcalls;
@@ -1422,26 +1423,6 @@ done:
 	return path;
 }
 
-void phonebook_req_finalize(void *request)
-{
-	struct phonebook_data *data = request;
-
-	DBG("");
-
-	if (!data)
-		return;
-
-	/* canceling asynchronous operation on tracker if any is active */
-	if (data->query_canc) {
-		g_cancellable_cancel(data->query_canc);
-		g_object_unref(data->query_canc);
-	}
-
-	free_data_contacts(data);
-	g_free(data->req_name);
-	g_free(data);
-}
-
 static gboolean find_checked_number(GSList *numbers, const char *number)
 {
 	GSList *l;
@@ -1460,6 +1441,13 @@ static void gstring_free_helper(gpointer data, gpointer user_data)
 	g_string_free(data, TRUE);
 }
 
+static void free_data_numbers(struct phonebook_data *data)
+{
+	g_slist_foreach(data->numbers, gstring_free_helper, NULL);
+	g_slist_free(data->numbers);
+	data->numbers = NULL;
+}
+
 static int pull_newmissedcalls(const char **reply, int num_fields,
 							void *user_data)
 {
@@ -1471,12 +1459,12 @@ static int pull_newmissedcalls(const char **reply, int num_fields,
 	if (num_fields < 0 || reply == NULL)
 		goto done;
 
-	if (!find_checked_number(data->contacts, reply[1])) {
+	if (!find_checked_number(data->numbers, reply[1])) {
 		if (g_strcmp0(reply[2], "false") == 0)
 			data->newmissedcalls++;
 		else {
 			GString *number = g_string_new(reply[1]);
-			data->contacts = g_slist_append(data->contacts,
+			data->numbers = g_slist_append(data->numbers,
 								number);
 		}
 	}
@@ -1484,9 +1472,7 @@ static int pull_newmissedcalls(const char **reply, int num_fields,
 
 done:
 	DBG("newmissedcalls %d", data->newmissedcalls);
-	g_slist_foreach(data->contacts, gstring_free_helper, NULL);
-	g_slist_free(data->contacts);
-	data->contacts = NULL;
+	free_data_numbers(data);
 
 	if (num_fields < 0) {
 		data->cb(NULL, 0, num_fields, 0, TRUE, data->user_data);
@@ -1513,6 +1499,27 @@ done:
 	return 0;
 }
 
+void phonebook_req_finalize(void *request)
+{
+	struct phonebook_data *data = request;
+
+	DBG("");
+
+	if (!data)
+		return;
+
+	/* canceling asynchronous operation on tracker if any is active */
+	if (data->query_canc) {
+		g_cancellable_cancel(data->query_canc);
+		g_object_unref(data->query_canc);
+	}
+
+	free_data_numbers(data);
+	free_data_contacts(data);
+	g_free(data->req_name);
+	g_free(data);
+}
+
 void *phonebook_pull(const char *name, const struct apparam_field *params,
 				phonebook_cb cb, void *user_data, int *err)
 {
-- 
1.7.4.1

[PATCH v3 2/2] Change append to prepend in pull_newmissedcalls

[Dmitriy Paliy]

---
 plugins/phonebook-tracker.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 95908ba..0e77ce8 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -1464,7 +1464,7 @@ static int pull_newmissedcalls(const char **reply, int num_fields,
 			data->newmissedcalls++;
 		else {
 			GString *number = g_string_new(reply[1]);
-			data->numbers = g_slist_append(data->numbers,
+			data->numbers = g_slist_prepend(data->numbers,
 								number);
 		}
 	}
-- 
1.7.4.1

Re: [PATCH v3 1/2] Fix change to numbers GSlist in new missed calls

[Johan Hedberg]

Hi Dmitriy,

On Thu, May 19, 2011, Dmitriy Paliy wrote:
> It is possible that phonebook_pull_read is invoked several times
> submitting multiple pull requests without closing PBAP object. E.g.,
> when history is large enough and maxlistcount>VCARDS_PART_COUNT.
> 
> The result is possibility of different data structures (GString and
> contact_data) to be mixed in a single GSlist that may lead to
> undefined behaviour.
> ---
>  plugins/phonebook-tracker.c |   57 ++++++++++++++++++++++++-------------------
>  1 files changed, 32 insertions(+), 25 deletions(-)

Both patches have been pushed upstream. Could you still explain what
data->numbers is supposed to be useful for? It's not used anywhere in
the code (except where creating and freeing the list).

Johan

Re: [PATCH v3 1/2] Fix change to numbers GSlist in new missed calls

[Dmitriy Paliy]

Hi Johan,

> Both patches have been pushed upstream. Could you still explain what
> data->numbers is supposed to be useful for? It's not used anywhere in
> the code (except where creating and freeing the list).

Thanks.

data->numbers is used in pull_newmissedcalls to store missed calls
numbers that were checked by user. It is updated when starts reading
response, and cleared when completed. Therefore,
free_data_numbers(data); is done also in _finalize function also for the
sake of safety if PBAP object is destroyed before the list if freed.

BR,
Dmitriy