960408 - [Bluetooth]Keep dbus arguments which are passed into the function alive until the callback is invoked

Status: RESOLVED INVALID

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Since all dbus operations have turned out to be async calls and moved to DBus thread, we have to keep arguments which are passed into the function alive until the callback is invoked.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #0)
> Since all dbus operations have turned out to be async calls and moved to
> DBus thread, we have to keep arguments which are passed into the function
> alive until the callback is invoked.

Depends. In most cases we simply build a message and send it to the Bluetooth daemon. Messages are build as part of the send operation and I'm not aware that the arguments are used any time later. So we don't need to keep these values around.

Then there are some callback functions which need additional data. This is usually passed as the data argument, or the callback is implemented with DBusReplyHandler, which can hold references to all extra parameters. The callback owns its data and is responsible for freeing it.

Do you have an example where we free data before it's being used?

Comment 2

[Eric Chou [:ericchou] [:echou]]

Hi Thomas,

> Depends. In most cases we simply build a message and send it to the Bluetooth daemon. Messages are build as part of 
> the send operation and I'm not aware that the arguments are used any time later. So we don't need to keep these 
> values around.
>
> Then there are some callback functions which need additional data. This is
> usually passed as the data argument, or the callback is implemented with
> DBusReplyHandler, which can hold references to all extra parameters. The
> callback owns its data and is responsible for freeing it.
> 
> Do you have an example where we free data before it's being used?

A typical example is in BluetoothDBusService::SetProperty(). The arguments attached with BlueZ command 'SetProperty' are addresses but not values. The variable 'val' is assigned with the address of local variables tmp_int and tempStr, and both would turn out to be invalid at the end of function. Since SendWithReply() is an async call, we may have a problem.

I observed this problem when I was reviewing Shawn's patch for bug 950220. In the past this didn't happen because SetProperty is a synchronous call on bt thread.

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Eric

> > Do you have an example where we free data before it's being used?
> 
> A typical example is in BluetoothDBusService::SetProperty(). The arguments
> attached with BlueZ command 'SetProperty' are addresses but not values. The
> variable 'val' is assigned with the address of local variables tmp_int and
> tempStr, and both would turn out to be invalid at the end of function. Since
> SendWithReply() is an async call, we may have a problem.

That's what I was talking about in the first paragraph. Whatever you pass to SendWithReply (or the DBus API it uses) gets marshaled into a string during the call and stored in a string in the DBusMessage structure. I just verified this a second time by stepping though dbus_message_new_method_call and dbus_message_iter_append_basic. The only reason why you pass pointers is to make the interface compatible with any type.

I had a look at the patch for bug 950220 and the reason why setting the device name failed was because tempStr went out of scope before its value gets marshaled into the message. But the rest of the code is fine.

Comment 4

[Eric Chou [:ericchou] [:echou]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> Hi Eric
> 
> > > Do you have an example where we free data before it's being used?
> > 
> > A typical example is in BluetoothDBusService::SetProperty(). The arguments
> > attached with BlueZ command 'SetProperty' are addresses but not values. The
> > variable 'val' is assigned with the address of local variables tmp_int and
> > tempStr, and both would turn out to be invalid at the end of function. Since
> > SendWithReply() is an async call, we may have a problem.
> 
> That's what I was talking about in the first paragraph. Whatever you pass to
> SendWithReply (or the DBus API it uses) gets marshaled into a string during
> the call and stored in a string in the DBusMessage structure. I just
> verified this a second time by stepping though dbus_message_new_method_call
> and dbus_message_iter_append_basic. The only reason why you pass pointers is
> to make the interface compatible with any type.

Oh, yes you're right. I didn't expect that dbus_message_iter_append_basic does so many things for caller, just checked the code in _dbus_marshal_write_basic and I think current implementation should be fine. I'm going to close this as resolved invalid. Thanks!

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #3)
> Hi Eric
> 
> > > Do you have an example where we free data before it's being used?
> > 
> > A typical example is in BluetoothDBusService::SetProperty(). The arguments
> > attached with BlueZ command 'SetProperty' are addresses but not values. The
> > variable 'val' is assigned with the address of local variables tmp_int and
> > tempStr, and both would turn out to be invalid at the end of function. Since
> > SendWithReply() is an async call, we may have a problem.
> 
> That's what I was talking about in the first paragraph. Whatever you pass to
> SendWithReply (or the DBus API it uses) gets marshaled into a string during
> the call and stored in a string in the DBusMessage structure. I just
> verified this a second time by stepping though dbus_message_new_method_call
> and dbus_message_iter_append_basic. The only reason why you pass pointers is
> to make the interface compatible with any type.
> 
> I had a look at the patch for bug 950220 and the reason why setting the
> device name failed was because tempStr went out of scope before its value
> gets marshaled into the message. But the rest of the code is fine.

My bad :(