[PATCH v1] dbus: Fix add invalid memory during interface removal

[PATCH v1] dbus: Fix add invalid memory during interface removal

[Shuai Zhang]

test setp
register_service <uuid>
register_application <uuid>
unregister_service <uuid>
unregister_application
register_service <uuid>
register_application <uuid>   
core dump

invalidate_parent_data is called to add the service to the application's
glist when unregister_service. However, this service has already been
added to the glist of root object in register_service. This results in
services existing in both queues,but only the services in the
application's glist are freed upon removal. A null address is stored
in root object's glist, a crash dump will occur when get_object is called.

Add a check for the parent pointer to avoid adding the service again.

0  0x0000007ff7df6058 in dbus_message_iter_append_basic ()
   from /usr/lib/libdbus-1.so.3
1  0x00000055555a3780 in append_object (data=0x31306666,
  user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1117
2  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
3  0x00000055555a37ac in append_object (data=0x5555642cf0,
  user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1122
4  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
5  0x00000055555a3630 in get_objects (connection=<optimized out>,
    message=<optimized out>, user_data=0x555563b390)
    at /usr/src/debug/bluez5/5.72/gdbus/object.c:1154
6  0x00000055555a51d0 in process_message (
    connection=connection@entry=0x5555639310,
    message=message@entry=0x5555649ac0,
    method=method@entry=0x55555facf8 <manager_methods>,
    iface_user_data=<optimized out>)
    at /usr/src/debug/bluez5/5.72/gdbus/object.c:246
7  0x00000055555a575c in generic_message (connection=0x5555639310,
    message=0x5555649ac0, user_data=<optimized out>)

Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
---
 gdbus/object.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdbus/object.c b/gdbus/object.c
index 7b0476f1a..d87a81160 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -809,7 +809,8 @@ static struct generic_data *invalidate_parent_data(DBusConnection *conn,
 
 	if (child == NULL || g_slist_find(data->objects, child) != NULL)
 		goto done;
-
+	if(g_slist_find(parent->objects, child) != NULL)
+		goto done;
 	data->objects = g_slist_prepend(data->objects, child);
 	child->parent = data;
 
-- 
2.34.1

RE: [v1] dbus: Fix add invalid memory during interface removal

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1260 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=947676

---Test result---

Test Summary:
CheckPatch                    PENDING   0.21 seconds
GitLint                       PENDING   0.21 seconds
BuildEll                      PASS      20.58 seconds
BluezMake                     PASS      1463.81 seconds
MakeCheck                     PASS      13.21 seconds
MakeDistcheck                 PASS      156.36 seconds
CheckValgrind                 PASS      211.73 seconds
CheckSmatch                   PASS      281.97 seconds
bluezmakeextell               PASS      97.21 seconds
IncrementalBuild              PENDING   0.28 seconds
ScanBuild                     PASS      859.96 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH v1] dbus: Fix add invalid memory during interface removal

[Luiz Augusto von Dentz]

Hi Shuai,

On Thu, Mar 27, 2025 at 4:41 AM Shuai Zhang <quic_shuaz@quicinc.com> wrote:
>
> test setp
> register_service <uuid>
> register_application <uuid>
> unregister_service <uuid>
> unregister_application
> register_service <uuid>
> register_application <uuid>
> core dump

Ok, what exactly are you talking about above, you are not referring to
D-Bus api it seems, are these bluetoothctl commands?

> invalidate_parent_data is called to add the service to the application's
> glist when unregister_service. However, this service has already been
> added to the glist of root object in register_service. This results in
> services existing in both queues,but only the services in the
> application's glist are freed upon removal. A null address is stored
> in root object's glist, a crash dump will occur when get_object is called.
>
> Add a check for the parent pointer to avoid adding the service again.
>
> 0  0x0000007ff7df6058 in dbus_message_iter_append_basic ()
>    from /usr/lib/libdbus-1.so.3
> 1  0x00000055555a3780 in append_object (data=0x31306666,
>   user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1117
> 2  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
> 3  0x00000055555a37ac in append_object (data=0x5555642cf0,
>   user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1122
> 4  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
> 5  0x00000055555a3630 in get_objects (connection=<optimized out>,
>     message=<optimized out>, user_data=0x555563b390)
>     at /usr/src/debug/bluez5/5.72/gdbus/object.c:1154
> 6  0x00000055555a51d0 in process_message (
>     connection=connection@entry=0x5555639310,
>     message=message@entry=0x5555649ac0,
>     method=method@entry=0x55555facf8 <manager_methods>,
>     iface_user_data=<optimized out>)
>     at /usr/src/debug/bluez5/5.72/gdbus/object.c:246
> 7  0x00000055555a575c in generic_message (connection=0x5555639310,
>     message=0x5555649ac0, user_data=<optimized out>)
>
> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
> ---
>  gdbus/object.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gdbus/object.c b/gdbus/object.c
> index 7b0476f1a..d87a81160 100644
> --- a/gdbus/object.c
> +++ b/gdbus/object.c
> @@ -809,7 +809,8 @@ static struct generic_data *invalidate_parent_data(DBusConnection *conn,
>
>         if (child == NULL || g_slist_find(data->objects, child) != NULL)
>                 goto done;
> -
> +       if(g_slist_find(parent->objects, child) != NULL)
> +               goto done;

Use empty lines after if statements, and keep the one you are removing.


>         data->objects = g_slist_prepend(data->objects, child);
>         child->parent = data;
>
> --
> 2.34.1
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v1] dbus: Fix add invalid memory during interface removal

[Luiz Augusto von Dentz]

Hi Shuai,

On Thu, Mar 27, 2025 at 10:58 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Shuai,
>
> On Thu, Mar 27, 2025 at 4:41 AM Shuai Zhang <quic_shuaz@quicinc.com> wrote:
> >
> > test setp
> > register_service <uuid>
> > register_application <uuid>
> > unregister_service <uuid>
> > unregister_application
> > register_service <uuid>
> > register_application <uuid>
> > core dump
>
> Ok, what exactly are you talking about above, you are not referring to
> D-Bus api it seems, are these bluetoothctl commands?

Tried this with bluetoothctl, doesn't seem to trigger any errors:

https://gist.github.com/Vudentz/7b20ff8b59e3122606a2239e1b63aa8a

>
> > invalidate_parent_data is called to add the service to the application's
> > glist when unregister_service. However, this service has already been
> > added to the glist of root object in register_service. This results in
> > services existing in both queues,but only the services in the
> > application's glist are freed upon removal. A null address is stored
> > in root object's glist, a crash dump will occur when get_object is called.
> >
> > Add a check for the parent pointer to avoid adding the service again.
> >
> > 0  0x0000007ff7df6058 in dbus_message_iter_append_basic ()
> >    from /usr/lib/libdbus-1.so.3
> > 1  0x00000055555a3780 in append_object (data=0x31306666,
> >   user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1117
> > 2  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
> > 3  0x00000055555a37ac in append_object (data=0x5555642cf0,
> >   user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1122
> > 4  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
> > 5  0x00000055555a3630 in get_objects (connection=<optimized out>,
> >     message=<optimized out>, user_data=0x555563b390)
> >     at /usr/src/debug/bluez5/5.72/gdbus/object.c:1154
> > 6  0x00000055555a51d0 in process_message (
> >     connection=connection@entry=0x5555639310,
> >     message=message@entry=0x5555649ac0,
> >     method=method@entry=0x55555facf8 <manager_methods>,
> >     iface_user_data=<optimized out>)
> >     at /usr/src/debug/bluez5/5.72/gdbus/object.c:246
> > 7  0x00000055555a575c in generic_message (connection=0x5555639310,
> >     message=0x5555649ac0, user_data=<optimized out>)
> >
> > Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
> > ---
> >  gdbus/object.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdbus/object.c b/gdbus/object.c
> > index 7b0476f1a..d87a81160 100644
> > --- a/gdbus/object.c
> > +++ b/gdbus/object.c
> > @@ -809,7 +809,8 @@ static struct generic_data *invalidate_parent_data(DBusConnection *conn,
> >
> >         if (child == NULL || g_slist_find(data->objects, child) != NULL)
> >                 goto done;
> > -
> > +       if(g_slist_find(parent->objects, child) != NULL)
> > +               goto done;
>
> Use empty lines after if statements, and keep the one you are removing.
>
>
> >         data->objects = g_slist_prepend(data->objects, child);
> >         child->parent = data;
> >
> > --
> > 2.34.1
> >
> >
>
>
> --
> Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH v1] dbus: Fix add invalid memory during interface removal

[Shuai Zhang]

Hi Luiz

On 3/27/2025 10:58 PM, Luiz Augusto von Dentz wrote:
> Hi Shuai,
> 
> On Thu, Mar 27, 2025 at 4:41 AM Shuai Zhang <quic_shuaz@quicinc.com> wrote:
>>
>> test setp
>> register_service <uuid>
>> register_application <uuid>
>> unregister_service <uuid>
>> unregister_application
>> register_service <uuid>
>> register_application <uuid>
>> core dump
> 
> Ok, what exactly are you talking about above, you are not referring to
> D-Bus api it seems, are these bluetoothctl commands?
> 

Yes,this is bluetoothctl command. I will add explanation in new patch.

>> invalidate_parent_data is called to add the service to the application's
>> glist when unregister_service. However, this service has already been
>> added to the glist of root object in register_service. This results in
>> services existing in both queues,but only the services in the
>> application's glist are freed upon removal. A null address is stored
>> in root object's glist, a crash dump will occur when get_object is called.
>>
>> Add a check for the parent pointer to avoid adding the service again.
>>
>> 0  0x0000007ff7df6058 in dbus_message_iter_append_basic ()
>>    from /usr/lib/libdbus-1.so.3
>> 1  0x00000055555a3780 in append_object (data=0x31306666,
>>   user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1117
>> 2  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
>> 3  0x00000055555a37ac in append_object (data=0x5555642cf0,
>>   user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1122
>> 4  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
>> 5  0x00000055555a3630 in get_objects (connection=<optimized out>,
>>     message=<optimized out>, user_data=0x555563b390)
>>     at /usr/src/debug/bluez5/5.72/gdbus/object.c:1154
>> 6  0x00000055555a51d0 in process_message (
>>     connection=connection@entry=0x5555639310,
>>     message=message@entry=0x5555649ac0,
>>     method=method@entry=0x55555facf8 <manager_methods>,
>>     iface_user_data=<optimized out>)
>>     at /usr/src/debug/bluez5/5.72/gdbus/object.c:246
>> 7  0x00000055555a575c in generic_message (connection=0x5555639310,
>>     message=0x5555649ac0, user_data=<optimized out>)
>>
>> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
>> ---
>>  gdbus/object.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdbus/object.c b/gdbus/object.c
>> index 7b0476f1a..d87a81160 100644
>> --- a/gdbus/object.c
>> +++ b/gdbus/object.c
>> @@ -809,7 +809,8 @@ static struct generic_data *invalidate_parent_data(DBusConnection *conn,
>>
>>         if (child == NULL || g_slist_find(data->objects, child) != NULL)
>>                 goto done;
>> -
>> +       if(g_slist_find(parent->objects, child) != NULL)
>> +               goto done;
> 
> Use empty lines after if statements, and keep the one you are removing.
> 

I will modify it, thanks.

> 
>>         data->objects = g_slist_prepend(data->objects, child);
>>         child->parent = data;
>>
>> --
>> 2.34.1
>>
>>
> 
> 

Re: [PATCH v1] dbus: Fix add invalid memory during interface removal

[Shuai Zhang]

Hi Luiz

On 3/27/2025 11:10 PM, Luiz Augusto von Dentz wrote:
> Hi Shuai,
> 
> On Thu, Mar 27, 2025 at 10:58 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>>
>> Hi Shuai,
>>
>> On Thu, Mar 27, 2025 at 4:41 AM Shuai Zhang <quic_shuaz@quicinc.com> wrote:
>>>
>>> test setp
>>> register_service <uuid>
>>> register_application <uuid>
>>> unregister_service <uuid>
>>> unregister_application
>>> register_service <uuid>
>>> register_application <uuid>
>>> core dump
>>
>> Ok, what exactly are you talking about above, you are not referring to
>> D-Bus api it seems, are these bluetoothctl commands?
> 
> Tried this with bluetoothctl, doesn't seem to trigger any errors:
> 
> https://gist.github.com/Vudentz/7b20ff8b59e3122606a2239e1b63aa8a
> 

This error is a probabilistic issue, and the frequency of reproduction is 
quite high. I have left the reproduction commit in your GitHub glist.

>>
>>> invalidate_parent_data is called to add the service to the application's
>>> glist when unregister_service. However, this service has already been
>>> added to the glist of root object in register_service. This results in
>>> services existing in both queues,but only the services in the
>>> application's glist are freed upon removal. A null address is stored
>>> in root object's glist, a crash dump will occur when get_object is called.
>>>
>>> Add a check for the parent pointer to avoid adding the service again.
>>>
>>> 0  0x0000007ff7df6058 in dbus_message_iter_append_basic ()
>>>    from /usr/lib/libdbus-1.so.3
>>> 1  0x00000055555a3780 in append_object (data=0x31306666,
>>>   user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1117
>>> 2  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
>>> 3  0x00000055555a37ac in append_object (data=0x5555642cf0,
>>>   user_data=0x7ffffff760) at /usr/src/debug/bluez5/5.72/gdbus/object.c:1122
>>> 4  0x0000007ff7ece0cc in g_slist_foreach () from /usr/lib/libglib-2.0.so.0
>>> 5  0x00000055555a3630 in get_objects (connection=<optimized out>,
>>>     message=<optimized out>, user_data=0x555563b390)
>>>     at /usr/src/debug/bluez5/5.72/gdbus/object.c:1154
>>> 6  0x00000055555a51d0 in process_message (
>>>     connection=connection@entry=0x5555639310,
>>>     message=message@entry=0x5555649ac0,
>>>     method=method@entry=0x55555facf8 <manager_methods>,
>>>     iface_user_data=<optimized out>)
>>>     at /usr/src/debug/bluez5/5.72/gdbus/object.c:246
>>> 7  0x00000055555a575c in generic_message (connection=0x5555639310,
>>>     message=0x5555649ac0, user_data=<optimized out>)
>>>
>>> Signed-off-by: Shuai Zhang <quic_shuaz@quicinc.com>
>>> ---
>>>  gdbus/object.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gdbus/object.c b/gdbus/object.c
>>> index 7b0476f1a..d87a81160 100644
>>> --- a/gdbus/object.c
>>> +++ b/gdbus/object.c
>>> @@ -809,7 +809,8 @@ static struct generic_data *invalidate_parent_data(DBusConnection *conn,
>>>
>>>         if (child == NULL || g_slist_find(data->objects, child) != NULL)
>>>                 goto done;
>>> -
>>> +       if(g_slist_find(parent->objects, child) != NULL)
>>> +               goto done;
>>
>> Use empty lines after if statements, and keep the one you are removing.
>>
>>
>>>         data->objects = g_slist_prepend(data->objects, child);
>>>         child->parent = data;
>>>
>>> --
>>> 2.34.1
>>>
>>>
>>
>>
>> --
>> Luiz Augusto von Dentz
> 
> 
>