2011-05-19 12:57:42

by Dmitriy Paliy

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

Hi,

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

Br,
Dmitriy

2011-05-20 06:51:48

by Dmitriy Paliy

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

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

2011-05-19 23:34:01

by Johan Hedberg

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

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

2011-05-19 12:57:43

by Dmitriy Paliy

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

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

2011-05-19 12:57:44

by Dmitriy Paliy

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

---
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