2011-07-06 15:57:06

by Dmitriy Paliy

Subject: [PATCH obexd 1/2] Fix invalid memory read when pulling phone book

Data contacts are freed already in finalize function which is called
in query_result callback function. Calling free_data_contacts(data)
twice causes invalid memory read when dereferencing data->contacts.
---
plugins/phonebook-tracker.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index d396203..7a1a003 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -1154,7 +1154,6 @@ static void send_pull_part(struct phonebook_data *data,
data->cb(vcards->str, vcards->len, g_slist_length(data->contacts),
data->newmissedcalls, lastpart, data->user_data);

- free_data_contacts(data);
g_string_free(vcards, TRUE);
}

--
1.7.4.1

2011-07-06 16:39:37

by Radoslaw Jablonski

Subject: Re: [PATCH obexd 1/2] Fix invalid memory read when pulling phone book

Hi Dmitriy,

On Wed, Jul 6, 2011 at 5:57 PM, Dmitriy Paliy <[email protected]> wrote:
>
> Data contacts are freed already in finalize function which is called
> in query_result callback function. Calling free_data_contacts(data)
> twice causes invalid memory read when dereferencing data->contacts.
> ---
> ?plugins/phonebook-tracker.c | ? ?1 -
> ?1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
> index d396203..7a1a003 100644
> --- a/plugins/phonebook-tracker.c
> +++ b/plugins/phonebook-tracker.c
> @@ -1154,7 +1154,6 @@ static void send_pull_part(struct phonebook_data *data,
> ? ? ? ?data->cb(vcards->str, vcards->len, g_slist_length(data->contacts),
> ? ? ? ? ? ? ? ? ? ? ? ?data->newmissedcalls, lastpart, data->user_data);
>
> - ? ? ? free_data_contacts(data);

This free_data_contacts is used here for scenarios when there is more than
one part of data to send (for clearing data of contacts that are already fetched
from db).
So simply removing this will cause very weird behaviour - every part
of new vcard
data will have "old" data on the beginning..

To avoid these invalid reads some more sophisticated changes in
phonebook_tracker
logic will be needed - contacts data should be cleared only from one
place in the code
(now free_data_contacts is called from 2 places..)

BR,
Radek

2011-07-06 15:57:07

by Dmitriy Paliy

Subject: [PATCH obexd 2/2] Fix finalize request in callback function

Request shell be finalized in cache_ready_notify callback function.
Otherwise, reference to the request is kept until vobject is closed.
It is correct in current implementation since there are no nested
backend requests when pulling vcard listing.

However, this contradicts to current design (see phonebook_size_result,
query_result, cache_entry_done) and may cause problems when such would
be needed.
---
plugins/pbap.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/plugins/pbap.c b/plugins/pbap.c
index 0356ae7..bbd06c7 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -490,6 +490,9 @@ static void cache_ready_notify(void *user_data)

DBG("");

+ phonebook_req_finalize(pbap->obj->request);
+ pbap->obj->request = NULL;
+
pbap->cache.valid = TRUE;

err = generate_response(pbap);
--
1.7.4.1