* [PATCH BlueZ] core: Fix walking the list while removing elements
@ 2012-10-04 6:06 Lucas De Marchi
2012-10-04 7:49 ` Johan Hedberg
0 siblings, 1 reply; 7+ messages in thread
From: Lucas De Marchi @ 2012-10-04 6:06 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Lucas De Marchi
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);
}
--
1.7.12.2
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH BlueZ] core: Fix walking the list while removing elements
2012-10-04 6:06 [PATCH BlueZ] core: Fix walking the list while removing elements Lucas De Marchi
@ 2012-10-04 7:49 ` Johan Hedberg
0 siblings, 0 replies; 7+ messages in thread
From: Johan Hedberg @ 2012-10-04 7:49 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: linux-bluetooth, Lucas De Marchi
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH BlueZ] core: Fix walking the list while removing elements
@ 2012-10-04 17:07 Lucas De Marchi
2012-10-04 17:16 ` Bastien Nocera
2012-10-04 18:22 ` Johan Hedberg
0 siblings, 2 replies; 7+ messages in thread
From: Lucas De Marchi @ 2012-10-04 17:07 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Lucas De Marchi
From: Lucas De Marchi <lucas.de.marchi@gmail.com>
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH BlueZ] core: Fix walking the list while removing elements
2012-10-04 17:07 Lucas De Marchi
@ 2012-10-04 17:16 ` Bastien Nocera
2012-10-04 17:21 ` Lucas De Marchi
2012-10-04 18:22 ` Johan Hedberg
1 sibling, 1 reply; 7+ messages in thread
From: Bastien Nocera @ 2012-10-04 17:16 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: linux-bluetooth, 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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH BlueZ] core: Fix walking the list while removing elements
2012-10-04 17:16 ` Bastien Nocera
@ 2012-10-04 17:21 ` Lucas De Marchi
2012-10-04 17:30 ` Bastien Nocera
0 siblings, 1 reply; 7+ messages in thread
From: Lucas De Marchi @ 2012-10-04 17:21 UTC (permalink / raw)
To: Bastien Nocera; +Cc: Lucas De Marchi, linux-bluetooth
On Thu, Oct 4, 2012 at 2:16 PM, Bastien Nocera <hadess@hadess.net> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH BlueZ] core: Fix walking the list while removing elements
2012-10-04 17:21 ` Lucas De Marchi
@ 2012-10-04 17:30 ` Bastien Nocera
0 siblings, 0 replies; 7+ messages in thread
From: Bastien Nocera @ 2012-10-04 17:30 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: Lucas De Marchi, linux-bluetooth
On Thu, 2012-10-04 at 14:21 -0300, Lucas De Marchi wrote:
> On Thu, Oct 4, 2012 at 2:16 PM, Bastien Nocera <hadess@hadess.net> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ] core: Fix walking the list while removing elements
2012-10-04 17:07 Lucas De Marchi
2012-10-04 17:16 ` Bastien Nocera
@ 2012-10-04 18:22 ` Johan Hedberg
1 sibling, 0 replies; 7+ messages in thread
From: Johan Hedberg @ 2012-10-04 18:22 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: linux-bluetooth, Lucas De Marchi
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-10-04 18:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-04 6:06 [PATCH BlueZ] core: Fix walking the list while removing elements Lucas De Marchi
2012-10-04 7:49 ` Johan Hedberg
-- strict thread matches above, loose matches on Subject: below --
2012-10-04 17:07 Lucas De Marchi
2012-10-04 17:16 ` Bastien Nocera
2012-10-04 17:21 ` Lucas De Marchi
2012-10-04 17:30 ` Bastien Nocera
2012-10-04 18:22 ` Johan Hedberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).