From: Lucas De Marchi <[email protected]>
If we are walking a GSList and remove the element we are pointing to,
the next iteration g_slist_next() will access previously freed
memory.
---
src/device.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/device.c b/src/device.c
index c659164..0339bcf 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1469,7 +1469,7 @@ static void device_remove_profiles(struct btd_device *device, GSList *uuids)
char srcaddr[18], dstaddr[18];
bdaddr_t src;
sdp_list_t *records;
- GSList *l;
+ GSList *l, *next;
adapter_get_address(adapter, &src);
ba2str(&src, srcaddr);
@@ -1498,10 +1498,11 @@ static void device_remove_profiles(struct btd_device *device, GSList *uuids)
if (records)
sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
- for (l = device->profiles; l != NULL; l = g_slist_next(l)) {
+ for (l = device->profiles; l != NULL; l = next) {
struct btd_profile *profile = l->data;
GSList *probe_uuids;
+ next = l->next;
probe_uuids = device_match_profile(device, profile,
device->uuids);
if (probe_uuids != NULL) {
--
1.7.12.2
Hi Lucas,
On Thu, Oct 04, 2012, Lucas De Marchi wrote:
> If we are walking a GSList and remove the element we are pointing to,
> the next iteration g_slist_next() will access previously freed
> memory.
> ---
> src/device.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
Applied. Thanks.
Johan
On Thu, 2012-10-04 at 14:21 -0300, Lucas De Marchi wrote:
> On Thu, Oct 4, 2012 at 2:16 PM, Bastien Nocera <[email protected]> wrote:
> > On Thu, 2012-10-04 at 14:07 -0300, Lucas De Marchi wrote:
> >> - for (l = device->profiles; l != NULL; l = g_slist_next(l)) {
> >> + for (l = device->profiles; l != NULL; l = next) {
> >
> > + for (l = device->profiles; l != NULL; l = l->next) {
>
> nops. you can't access "l" if it was deleted from the list
Right.
On Thu, Oct 4, 2012 at 2:16 PM, Bastien Nocera <[email protected]> wrote:
> On Thu, 2012-10-04 at 14:07 -0300, Lucas De Marchi wrote:
>> - for (l = device->profiles; l != NULL; l = g_slist_next(l)) {
>> + for (l = device->profiles; l != NULL; l = next) {
>
> + for (l = device->profiles; l != NULL; l = l->next) {
nops. you can't access "l" if it was deleted from the list
>
>> struct btd_profile *profile = l->data;
>> GSList *probe_uuids;
>>
>> + next = l->next;
>
> Remove.
nops.
See previous patch.
Lucas De Marchi
On Thu, 2012-10-04 at 14:07 -0300, Lucas De Marchi wrote:
> - for (l = device->profiles; l != NULL; l = g_slist_next(l)) {
> + for (l = device->profiles; l != NULL; l = next) {
+ for (l = device->profiles; l != NULL; l = l->next) {
> struct btd_profile *profile = l->data;
> GSList *probe_uuids;
>
> + next = l->next;
Remove.
Hi Lucas,
On Thu, Oct 04, 2012, Lucas De Marchi wrote:
> If we are walking a GSList and remove the element we are pointing to,
> the next iteration g_slist_next() will access previously freed
> memory.
> ---
>
> This was caught only by inspecting the code. I don't know why valgrind
> didn't complain about accessing previously freed memory region.
>
> src/device.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/device.c b/src/device.c
> index c659164..6150963 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -1498,7 +1498,7 @@ static void device_remove_profiles(struct btd_device *device, GSList *uuids)
> if (records)
> sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
>
> - for (l = device->profiles; l != NULL; l = g_slist_next(l)) {
> + for (l = device->profiles; l != NULL;) {
> struct btd_profile *profile = l->data;
> GSList *probe_uuids;
>
> @@ -1506,9 +1506,11 @@ static void device_remove_profiles(struct btd_device *device, GSList *uuids)
> device->uuids);
> if (probe_uuids != NULL) {
> g_slist_free(probe_uuids);
> + l = l->next;
> continue;
> }
>
> + l = l->next;
> profile->device_remove(profile, device);
> device->profiles = g_slist_remove(device->profiles, profile);
> }
Thanks for catching this, however could you fix it the same way most
other similar loops in the code-base do it, i.e. add a GSList *next
helper variable:
GSList *l, *next;
for (l = device->profiles; l != NULL; l = next) {
<variable declarations>
next = l->next;
<rest of the loop code>
}
Johan