* [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation @ 2019-09-14 4:32 Sonny Sasaka 2019-09-15 17:14 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 7+ messages in thread From: Sonny Sasaka @ 2019-09-14 4:32 UTC (permalink / raw) To: linux-bluetooth Currently at the start of client creation (g_dbus_client_new), the |owner| in |filter_data| is not set until the |name| is resolved. This creates a time window where the filter doesn't work properly, i.e. it filters in more than it should. To solve this issue, this patch does the following: 1. At the start of client creation, set the |owner| in |filter_data| based on the current resolved |name| if any, or set it explicitly to unknown (empty string) as opposed to NULL otherwise. The unknown |owner| lets the filter reject any message, unlike NULL |owner| which accepts any message. 2. Step 1 above reveals another bug: message_filter fails to accept messages which have |sender| set directly to D-Bus service name rather than D-Bus address. Therefore this patch relaxes the filter requirement in message_filter to accept a message if its |sender| is equal directly to our filter's |name|. 3. After the initial service name resolution (after GetNameOwner) returns, immediately update our name cache with the result, otherwise the filters' |owner| would be stuck to unknown (empty string) until "NameOwnerChanged" signal arrives. --- gdbus/watch.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/gdbus/watch.c b/gdbus/watch.c index 447e48671..2ae0fd5a7 100644 --- a/gdbus/watch.c +++ b/gdbus/watch.c @@ -78,6 +78,8 @@ struct filter_data { gboolean registered; }; +static const char *check_name_cache(const char *name); + static struct filter_data *filter_data_find_match(DBusConnection *connection, const char *name, const char *owner, @@ -265,7 +267,10 @@ proceed: data->connection = dbus_connection_ref(connection); data->name = g_strdup(name); - data->owner = g_strdup(owner); + if (name) + data->owner = g_strdup(check_name_cache(name) ? : ""); + else + data->owner = g_strdup(owner); data->path = g_strdup(path); data->interface = g_strdup(interface); data->member = g_strdup(member); @@ -534,8 +539,12 @@ static DBusHandlerResult message_filter(DBusConnection *connection, if (!sender && data->owner) continue; - if (data->owner && g_str_equal(sender, data->owner) == FALSE) + if (data->owner && + g_str_equal(sender, data->owner) == FALSE && + data->name && + g_str_equal(sender, data->name) == FALSE) { continue; + } if (data->path && g_str_equal(path, data->path) == FALSE) continue; @@ -627,6 +636,7 @@ static void service_reply(DBusPendingCall *call, void *user_data) DBUS_TYPE_INVALID) == FALSE) goto fail; + update_name_cache(data->name, data->owner); update_service(data); goto done; -- 2.21.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation 2019-09-14 4:32 [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation Sonny Sasaka @ 2019-09-15 17:14 ` Luiz Augusto von Dentz 2019-09-15 17:38 ` Sonny Sasaka 0 siblings, 1 reply; 7+ messages in thread From: Luiz Augusto von Dentz @ 2019-09-15 17:14 UTC (permalink / raw) To: Sonny Sasaka; +Cc: linux-bluetooth Hi Sonny, On Sat, Sep 14, 2019 at 5:57 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > Currently at the start of client creation (g_dbus_client_new), the > |owner| in |filter_data| is not set until the |name| is resolved. This > creates a time window where the filter doesn't work properly, i.e. it > filters in more than it should. To solve this issue, this patch does the > following: > 1. At the start of client creation, set the |owner| in |filter_data| > based on the current resolved |name| if any, or set it explicitly to > unknown (empty string) as opposed to NULL otherwise. The unknown |owner| > lets the filter reject any message, unlike NULL |owner| which accepts > any message. > 2. Step 1 above reveals another bug: message_filter fails to accept > messages which have |sender| set directly to D-Bus service name rather > than D-Bus address. Therefore this patch relaxes the filter requirement > in message_filter to accept a message if its |sender| is equal directly > to our filter's |name|. > 3. After the initial service name resolution (after GetNameOwner) > returns, immediately update our name cache with the result, otherwise > the filters' |owner| would be stuck to unknown (empty string) until > "NameOwnerChanged" signal arrives. > > --- > gdbus/watch.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/gdbus/watch.c b/gdbus/watch.c > index 447e48671..2ae0fd5a7 100644 > --- a/gdbus/watch.c > +++ b/gdbus/watch.c > @@ -78,6 +78,8 @@ struct filter_data { > gboolean registered; > }; > > +static const char *check_name_cache(const char *name); > + > static struct filter_data *filter_data_find_match(DBusConnection *connection, > const char *name, > const char *owner, > @@ -265,7 +267,10 @@ proceed: > > data->connection = dbus_connection_ref(connection); > data->name = g_strdup(name); > - data->owner = g_strdup(owner); > + if (name) > + data->owner = g_strdup(check_name_cache(name) ? : ""); I follow this it would ignore the owner address and use the cache name or set "" to be resolved shouldn't that use the owner instead? If the called don't have it resolved then it should optionally set the owner resolution. > + else > + data->owner = g_strdup(owner); > data->path = g_strdup(path); > data->interface = g_strdup(interface); > data->member = g_strdup(member); > @@ -534,8 +539,12 @@ static DBusHandlerResult > message_filter(DBusConnection *connection, > if (!sender && data->owner) > continue; > > - if (data->owner && g_str_equal(sender, data->owner) == FALSE) > + if (data->owner && > + g_str_equal(sender, data->owner) == FALSE && > + data->name && > + g_str_equal(sender, data->name) == FALSE) { iirc messages never use the friendly name only the bus connection as sender so I wonder if this really does make any difference, are there any example of this not working? Perhaps it would be worth creating a test case in unit/test-gdbus.c to capture this case. > continue; > + } > > if (data->path && g_str_equal(path, data->path) == FALSE) > continue; > @@ -627,6 +636,7 @@ static void service_reply(DBusPendingCall *call, > void *user_data) > DBUS_TYPE_INVALID) == FALSE) > goto fail; > > + update_name_cache(data->name, data->owner); > update_service(data); > > goto done; > -- > 2.21.0 -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation 2019-09-15 17:14 ` Luiz Augusto von Dentz @ 2019-09-15 17:38 ` Sonny Sasaka 2019-09-16 11:46 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 7+ messages in thread From: Sonny Sasaka @ 2019-09-15 17:38 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hi Luiz, On Sun, Sep 15, 2019 at 10:14 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Sonny, > > On Sat, Sep 14, 2019 at 5:57 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > > > Currently at the start of client creation (g_dbus_client_new), the > > |owner| in |filter_data| is not set until the |name| is resolved. This > > creates a time window where the filter doesn't work properly, i.e. it > > filters in more than it should. To solve this issue, this patch does the > > following: > > 1. At the start of client creation, set the |owner| in |filter_data| > > based on the current resolved |name| if any, or set it explicitly to > > unknown (empty string) as opposed to NULL otherwise. The unknown |owner| > > lets the filter reject any message, unlike NULL |owner| which accepts > > any message. > > 2. Step 1 above reveals another bug: message_filter fails to accept > > messages which have |sender| set directly to D-Bus service name rather > > than D-Bus address. Therefore this patch relaxes the filter requirement > > in message_filter to accept a message if its |sender| is equal directly > > to our filter's |name|. > > 3. After the initial service name resolution (after GetNameOwner) > > returns, immediately update our name cache with the result, otherwise > > the filters' |owner| would be stuck to unknown (empty string) until > > "NameOwnerChanged" signal arrives. > > > > --- > > gdbus/watch.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/gdbus/watch.c b/gdbus/watch.c > > index 447e48671..2ae0fd5a7 100644 > > --- a/gdbus/watch.c > > +++ b/gdbus/watch.c > > @@ -78,6 +78,8 @@ struct filter_data { > > gboolean registered; > > }; > > > > +static const char *check_name_cache(const char *name); > > + > > static struct filter_data *filter_data_find_match(DBusConnection *connection, > > const char *name, > > const char *owner, > > @@ -265,7 +267,10 @@ proceed: > > > > data->connection = dbus_connection_ref(connection); > > data->name = g_strdup(name); > > - data->owner = g_strdup(owner); > > + if (name) > > + data->owner = g_strdup(check_name_cache(name) ? : ""); > > I follow this it would ignore the owner address and use the cache name > or set "" to be resolved shouldn't that use the owner instead? If the > called don't have it resolved then it should optionally set the owner > resolution. if |name| is set, |owner| must be NULL (refer to if block before proceed: label above). > > > + else > > + data->owner = g_strdup(owner); > > data->path = g_strdup(path); > > data->interface = g_strdup(interface); > > data->member = g_strdup(member); > > @@ -534,8 +539,12 @@ static DBusHandlerResult > > message_filter(DBusConnection *connection, > > if (!sender && data->owner) > > continue; > > > > - if (data->owner && g_str_equal(sender, data->owner) == FALSE) > > + if (data->owner && > > + g_str_equal(sender, data->owner) == FALSE && > > + data->name && > > + g_str_equal(sender, data->name) == FALSE) { > > iirc messages never use the friendly name only the bus connection as > sender so I wonder if this really does make any difference, are there > any example of this not working? Perhaps it would be worth creating a > test case in unit/test-gdbus.c to capture this case. There is a case where the sender is D-Bus daemon itself. For example NameOwnerChanged signal is sent via a message which has sender="org.freedesktop.dbus" instead of a D-Bus address. I am not aware of any other case other than messages sent by D-Bus daemon. > > > continue; > > + } > > > > if (data->path && g_str_equal(path, data->path) == FALSE) > > continue; > > @@ -627,6 +636,7 @@ static void service_reply(DBusPendingCall *call, > > void *user_data) > > DBUS_TYPE_INVALID) == FALSE) > > goto fail; > > > > + update_name_cache(data->name, data->owner); > > update_service(data); > > > > goto done; > > -- > > 2.21.0 > > > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation 2019-09-15 17:38 ` Sonny Sasaka @ 2019-09-16 11:46 ` Luiz Augusto von Dentz 2019-09-19 19:51 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 7+ messages in thread From: Luiz Augusto von Dentz @ 2019-09-16 11:46 UTC (permalink / raw) To: Sonny Sasaka; +Cc: linux-bluetooth Hi Sonny, On Sun, Sep 15, 2019 at 8:38 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > Hi Luiz, > > On Sun, Sep 15, 2019 at 10:14 AM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Sonny, > > > > On Sat, Sep 14, 2019 at 5:57 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > > > > > Currently at the start of client creation (g_dbus_client_new), the > > > |owner| in |filter_data| is not set until the |name| is resolved. This > > > creates a time window where the filter doesn't work properly, i.e. it > > > filters in more than it should. To solve this issue, this patch does the > > > following: > > > 1. At the start of client creation, set the |owner| in |filter_data| > > > based on the current resolved |name| if any, or set it explicitly to > > > unknown (empty string) as opposed to NULL otherwise. The unknown |owner| > > > lets the filter reject any message, unlike NULL |owner| which accepts > > > any message. > > > 2. Step 1 above reveals another bug: message_filter fails to accept > > > messages which have |sender| set directly to D-Bus service name rather > > > than D-Bus address. Therefore this patch relaxes the filter requirement > > > in message_filter to accept a message if its |sender| is equal directly > > > to our filter's |name|. > > > 3. After the initial service name resolution (after GetNameOwner) > > > returns, immediately update our name cache with the result, otherwise > > > the filters' |owner| would be stuck to unknown (empty string) until > > > "NameOwnerChanged" signal arrives. > > > > > > --- > > > gdbus/watch.c | 14 ++++++++++++-- > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/gdbus/watch.c b/gdbus/watch.c > > > index 447e48671..2ae0fd5a7 100644 > > > --- a/gdbus/watch.c > > > +++ b/gdbus/watch.c > > > @@ -78,6 +78,8 @@ struct filter_data { > > > gboolean registered; > > > }; > > > > > > +static const char *check_name_cache(const char *name); > > > + > > > static struct filter_data *filter_data_find_match(DBusConnection *connection, > > > const char *name, > > > const char *owner, > > > @@ -265,7 +267,10 @@ proceed: > > > > > > data->connection = dbus_connection_ref(connection); > > > data->name = g_strdup(name); > > > - data->owner = g_strdup(owner); > > > + if (name) > > > + data->owner = g_strdup(check_name_cache(name) ? : ""); > > > > I follow this it would ignore the owner address and use the cache name > > or set "" to be resolved shouldn't that use the owner instead? If the > > called don't have it resolved then it should optionally set the owner > > resolution. > if |name| is set, |owner| must be NULL (refer to if block before > proceed: label above). Right so the subsequent change is actually the result of owner being set "", not a bug that existed before. > > > > > + else > > > + data->owner = g_strdup(owner); > > > data->path = g_strdup(path); > > > data->interface = g_strdup(interface); > > > data->member = g_strdup(member); > > > @@ -534,8 +539,12 @@ static DBusHandlerResult > > > message_filter(DBusConnection *connection, > > > if (!sender && data->owner) > > > continue; > > > > > > - if (data->owner && g_str_equal(sender, data->owner) == FALSE) > > > + if (data->owner && > > > + g_str_equal(sender, data->owner) == FALSE && > > > + data->name && > > > + g_str_equal(sender, data->name) == FALSE) { > > > > iirc messages never use the friendly name only the bus connection as > > sender so I wonder if this really does make any difference, are there > > any example of this not working? Perhaps it would be worth creating a > > test case in unit/test-gdbus.c to capture this case. > There is a case where the sender is D-Bus daemon itself. For example > NameOwnerChanged signal is sent via a message which has > sender="org.freedesktop.dbus" instead of a D-Bus address. I am not > aware of any other case other than messages sent by D-Bus daemon. We could perhaps have the check as if(data->owner && data->owner != '\0'... so we skip the check if it was not resolved yet, in that case we would accept signals for watches which we do not have resolved their bus name resolved yet, which I guess it is the real issue you are trying to fix here. > > > > > continue; > > > + } > > > > > > if (data->path && g_str_equal(path, data->path) == FALSE) > > > continue; > > > @@ -627,6 +636,7 @@ static void service_reply(DBusPendingCall *call, > > > void *user_data) > > > DBUS_TYPE_INVALID) == FALSE) > > > goto fail; > > > This is not needed btw, update_name_cache will just lookup the same data and overwrite the owner with the same owner, well except if we do have other data pointing to the same name which is normally not the case of here, or is it? Also Im not sure how service_reply would be triggered since data->owner will always going to be set to either the cache or "", so it looks like this would only work because the bus name is no longer matched. > > > + update_name_cache(data->name, data->owner); > > > update_service(data); > > > > > > goto done; > > > -- > > > 2.21.0 > > > > > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation 2019-09-16 11:46 ` Luiz Augusto von Dentz @ 2019-09-19 19:51 ` Luiz Augusto von Dentz 2019-09-20 19:48 ` Sonny Sasaka 0 siblings, 1 reply; 7+ messages in thread From: Luiz Augusto von Dentz @ 2019-09-19 19:51 UTC (permalink / raw) To: Sonny Sasaka; +Cc: linux-bluetooth Hi Sonny, On Mon, Sep 16, 2019 at 2:46 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Sonny, > > On Sun, Sep 15, 2019 at 8:38 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > > > Hi Luiz, > > > > On Sun, Sep 15, 2019 at 10:14 AM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Sonny, > > > > > > On Sat, Sep 14, 2019 at 5:57 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > > > > > > > Currently at the start of client creation (g_dbus_client_new), the > > > > |owner| in |filter_data| is not set until the |name| is resolved. This > > > > creates a time window where the filter doesn't work properly, i.e. it > > > > filters in more than it should. To solve this issue, this patch does the > > > > following: > > > > 1. At the start of client creation, set the |owner| in |filter_data| > > > > based on the current resolved |name| if any, or set it explicitly to > > > > unknown (empty string) as opposed to NULL otherwise. The unknown |owner| > > > > lets the filter reject any message, unlike NULL |owner| which accepts > > > > any message. > > > > 2. Step 1 above reveals another bug: message_filter fails to accept > > > > messages which have |sender| set directly to D-Bus service name rather > > > > than D-Bus address. Therefore this patch relaxes the filter requirement > > > > in message_filter to accept a message if its |sender| is equal directly > > > > to our filter's |name|. > > > > 3. After the initial service name resolution (after GetNameOwner) > > > > returns, immediately update our name cache with the result, otherwise > > > > the filters' |owner| would be stuck to unknown (empty string) until > > > > "NameOwnerChanged" signal arrives. > > > > > > > > --- > > > > gdbus/watch.c | 14 ++++++++++++-- > > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/gdbus/watch.c b/gdbus/watch.c > > > > index 447e48671..2ae0fd5a7 100644 > > > > --- a/gdbus/watch.c > > > > +++ b/gdbus/watch.c > > > > @@ -78,6 +78,8 @@ struct filter_data { > > > > gboolean registered; > > > > }; > > > > > > > > +static const char *check_name_cache(const char *name); > > > > + > > > > static struct filter_data *filter_data_find_match(DBusConnection *connection, > > > > const char *name, > > > > const char *owner, > > > > @@ -265,7 +267,10 @@ proceed: > > > > > > > > data->connection = dbus_connection_ref(connection); > > > > data->name = g_strdup(name); > > > > - data->owner = g_strdup(owner); > > > > + if (name) > > > > + data->owner = g_strdup(check_name_cache(name) ? : ""); > > > > > > I follow this it would ignore the owner address and use the cache name > > > or set "" to be resolved shouldn't that use the owner instead? If the > > > called don't have it resolved then it should optionally set the owner > > > resolution. > > if |name| is set, |owner| must be NULL (refer to if block before > > proceed: label above). > > Right so the subsequent change is actually the result of owner being > set "", not a bug that existed before. > > > > > > > > + else > > > > + data->owner = g_strdup(owner); > > > > data->path = g_strdup(path); > > > > data->interface = g_strdup(interface); > > > > data->member = g_strdup(member); > > > > @@ -534,8 +539,12 @@ static DBusHandlerResult > > > > message_filter(DBusConnection *connection, > > > > if (!sender && data->owner) > > > > continue; > > > > > > > > - if (data->owner && g_str_equal(sender, data->owner) == FALSE) > > > > + if (data->owner && > > > > + g_str_equal(sender, data->owner) == FALSE && > > > > + data->name && > > > > + g_str_equal(sender, data->name) == FALSE) { > > > > > > iirc messages never use the friendly name only the bus connection as > > > sender so I wonder if this really does make any difference, are there > > > any example of this not working? Perhaps it would be worth creating a > > > test case in unit/test-gdbus.c to capture this case. > > There is a case where the sender is D-Bus daemon itself. For example > > NameOwnerChanged signal is sent via a message which has > > sender="org.freedesktop.dbus" instead of a D-Bus address. I am not > > aware of any other case other than messages sent by D-Bus daemon. > > We could perhaps have the check as if(data->owner && data->owner != > '\0'... so we skip the check if it was not resolved yet, in that case > we would accept signals for watches which we do not have resolved > their bus name resolved yet, which I guess it is the real issue you > are trying to fix here. > > > > > > > > continue; > > > > + } > > > > > > > > if (data->path && g_str_equal(path, data->path) == FALSE) > > > > continue; > > > > @@ -627,6 +636,7 @@ static void service_reply(DBusPendingCall *call, > > > > void *user_data) > > > > DBUS_TYPE_INVALID) == FALSE) > > > > goto fail; > > > > > > This is not needed btw, update_name_cache will just lookup the same > data and overwrite the owner with the same owner, well except if we do > have other data pointing to the same name which is normally not the > case of here, or is it? Also Im not sure how service_reply would be > triggered since data->owner will always going to be set to either the > cache or "", so it looks like this would only work because the bus > name is no longer matched. > > > > > + update_name_cache(data->name, data->owner); > > > > update_service(data); > > > > > > > > goto done; > > > > -- > > > > 2.21.0 Are you still planning on working on this, it seems to be valid fix we just have to work on the details and I hope I didn't demotivate you with my comments. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation 2019-09-19 19:51 ` Luiz Augusto von Dentz @ 2019-09-20 19:48 ` Sonny Sasaka 2019-09-27 8:43 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 7+ messages in thread From: Sonny Sasaka @ 2019-09-20 19:48 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: linux-bluetooth Hi Luiz, Sorry for the late reply. Yes I think this should be fixed. My comments below. On Thu, Sep 19, 2019 at 12:51 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Sonny, > > On Mon, Sep 16, 2019 at 2:46 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Sonny, > > > > On Sun, Sep 15, 2019 at 8:38 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > > > > > Hi Luiz, > > > > > > On Sun, Sep 15, 2019 at 10:14 AM Luiz Augusto von Dentz > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > Hi Sonny, > > > > > > > > On Sat, Sep 14, 2019 at 5:57 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > > > > > > > > > Currently at the start of client creation (g_dbus_client_new), the > > > > > |owner| in |filter_data| is not set until the |name| is resolved. This > > > > > creates a time window where the filter doesn't work properly, i.e. it > > > > > filters in more than it should. To solve this issue, this patch does the > > > > > following: > > > > > 1. At the start of client creation, set the |owner| in |filter_data| > > > > > based on the current resolved |name| if any, or set it explicitly to > > > > > unknown (empty string) as opposed to NULL otherwise. The unknown |owner| > > > > > lets the filter reject any message, unlike NULL |owner| which accepts > > > > > any message. > > > > > 2. Step 1 above reveals another bug: message_filter fails to accept > > > > > messages which have |sender| set directly to D-Bus service name rather > > > > > than D-Bus address. Therefore this patch relaxes the filter requirement > > > > > in message_filter to accept a message if its |sender| is equal directly > > > > > to our filter's |name|. > > > > > 3. After the initial service name resolution (after GetNameOwner) > > > > > returns, immediately update our name cache with the result, otherwise > > > > > the filters' |owner| would be stuck to unknown (empty string) until > > > > > "NameOwnerChanged" signal arrives. > > > > > > > > > > --- > > > > > gdbus/watch.c | 14 ++++++++++++-- > > > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/gdbus/watch.c b/gdbus/watch.c > > > > > index 447e48671..2ae0fd5a7 100644 > > > > > --- a/gdbus/watch.c > > > > > +++ b/gdbus/watch.c > > > > > @@ -78,6 +78,8 @@ struct filter_data { > > > > > gboolean registered; > > > > > }; > > > > > > > > > > +static const char *check_name_cache(const char *name); > > > > > + > > > > > static struct filter_data *filter_data_find_match(DBusConnection *connection, > > > > > const char *name, > > > > > const char *owner, > > > > > @@ -265,7 +267,10 @@ proceed: > > > > > > > > > > data->connection = dbus_connection_ref(connection); > > > > > data->name = g_strdup(name); > > > > > - data->owner = g_strdup(owner); > > > > > + if (name) > > > > > + data->owner = g_strdup(check_name_cache(name) ? : ""); > > > > > > > > I follow this it would ignore the owner address and use the cache name > > > > or set "" to be resolved shouldn't that use the owner instead? If the > > > > called don't have it resolved then it should optionally set the owner > > > > resolution. > > > if |name| is set, |owner| must be NULL (refer to if block before > > > proceed: label above). > > > > Right so the subsequent change is actually the result of owner being > > set "", not a bug that existed before. > > > > > > > > > > > + else > > > > > + data->owner = g_strdup(owner); > > > > > data->path = g_strdup(path); > > > > > data->interface = g_strdup(interface); > > > > > data->member = g_strdup(member); > > > > > @@ -534,8 +539,12 @@ static DBusHandlerResult > > > > > message_filter(DBusConnection *connection, > > > > > if (!sender && data->owner) > > > > > continue; > > > > > > > > > > - if (data->owner && g_str_equal(sender, data->owner) == FALSE) > > > > > + if (data->owner && > > > > > + g_str_equal(sender, data->owner) == FALSE && > > > > > + data->name && > > > > > + g_str_equal(sender, data->name) == FALSE) { > > > > > > > > iirc messages never use the friendly name only the bus connection as > > > > sender so I wonder if this really does make any difference, are there > > > > any example of this not working? Perhaps it would be worth creating a > > > > test case in unit/test-gdbus.c to capture this case. > > > There is a case where the sender is D-Bus daemon itself. For example > > > NameOwnerChanged signal is sent via a message which has > > > sender="org.freedesktop.dbus" instead of a D-Bus address. I am not > > > aware of any other case other than messages sent by D-Bus daemon. > > > > We could perhaps have the check as if(data->owner && data->owner != > > '\0'... so we skip the check if it was not resolved yet, in that case > > we would accept signals for watches which we do not have resolved > > their bus name resolved yet, which I guess it is the real issue you > > are trying to fix here. It should be the opposite: If name is not yet resolved, we don't want to accept anything because that would be receiving messages from anybody, which is wrong. > > > > > > > > > > > continue; > > > > > + } > > > > > > > > > > if (data->path && g_str_equal(path, data->path) == FALSE) > > > > > continue; > > > > > @@ -627,6 +636,7 @@ static void service_reply(DBusPendingCall *call, > > > > > void *user_data) > > > > > DBUS_TYPE_INVALID) == FALSE) > > > > > goto fail; > > > > > > > > > This is not needed btw, update_name_cache will just lookup the same > > data and overwrite the owner with the same owner, well except if we do > > have other data pointing to the same name which is normally not the > > case of here, or is it? Also Im not sure how service_reply would be > > triggered since data->owner will always going to be set to either the > > cache or "", so it looks like this would only work because the bus > > name is no longer matched. This is needed because there might be another filter_data which does not have the name resolved, so we should update them all with the resolved name. > > > > > > > + update_name_cache(data->name, data->owner); > > > > > update_service(data); > > > > > > > > > > goto done; > > > > > -- > > > > > 2.21.0 > > Are you still planning on working on this, it seems to be valid fix we > just have to work on the details and I hope I didn't demotivate you > with my comments. > > -- > Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation 2019-09-20 19:48 ` Sonny Sasaka @ 2019-09-27 8:43 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 7+ messages in thread From: Luiz Augusto von Dentz @ 2019-09-27 8:43 UTC (permalink / raw) To: Sonny Sasaka; +Cc: linux-bluetooth Hi Sonny, On Fri, Sep 20, 2019 at 10:48 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > Hi Luiz, > > Sorry for the late reply. Yes I think this should be fixed. My comments below. > > On Thu, Sep 19, 2019 at 12:51 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Sonny, > > > > On Mon, Sep 16, 2019 at 2:46 PM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Sonny, > > > > > > On Sun, Sep 15, 2019 at 8:38 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > > > > > > > Hi Luiz, > > > > > > > > On Sun, Sep 15, 2019 at 10:14 AM Luiz Augusto von Dentz > > > > <luiz.dentz@gmail.com> wrote: > > > > > > > > > > Hi Sonny, > > > > > > > > > > On Sat, Sep 14, 2019 at 5:57 PM Sonny Sasaka <sonnysasaka@gmail.com> wrote: > > > > > > > > > > > > Currently at the start of client creation (g_dbus_client_new), the > > > > > > |owner| in |filter_data| is not set until the |name| is resolved. This > > > > > > creates a time window where the filter doesn't work properly, i.e. it > > > > > > filters in more than it should. To solve this issue, this patch does the > > > > > > following: > > > > > > 1. At the start of client creation, set the |owner| in |filter_data| > > > > > > based on the current resolved |name| if any, or set it explicitly to > > > > > > unknown (empty string) as opposed to NULL otherwise. The unknown |owner| > > > > > > lets the filter reject any message, unlike NULL |owner| which accepts > > > > > > any message. > > > > > > 2. Step 1 above reveals another bug: message_filter fails to accept > > > > > > messages which have |sender| set directly to D-Bus service name rather > > > > > > than D-Bus address. Therefore this patch relaxes the filter requirement > > > > > > in message_filter to accept a message if its |sender| is equal directly > > > > > > to our filter's |name|. > > > > > > 3. After the initial service name resolution (after GetNameOwner) > > > > > > returns, immediately update our name cache with the result, otherwise > > > > > > the filters' |owner| would be stuck to unknown (empty string) until > > > > > > "NameOwnerChanged" signal arrives. > > > > > > > > > > > > --- > > > > > > gdbus/watch.c | 14 ++++++++++++-- > > > > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/gdbus/watch.c b/gdbus/watch.c > > > > > > index 447e48671..2ae0fd5a7 100644 > > > > > > --- a/gdbus/watch.c > > > > > > +++ b/gdbus/watch.c > > > > > > @@ -78,6 +78,8 @@ struct filter_data { > > > > > > gboolean registered; > > > > > > }; > > > > > > > > > > > > +static const char *check_name_cache(const char *name); > > > > > > + > > > > > > static struct filter_data *filter_data_find_match(DBusConnection *connection, > > > > > > const char *name, > > > > > > const char *owner, > > > > > > @@ -265,7 +267,10 @@ proceed: > > > > > > > > > > > > data->connection = dbus_connection_ref(connection); > > > > > > data->name = g_strdup(name); > > > > > > - data->owner = g_strdup(owner); > > > > > > + if (name) > > > > > > + data->owner = g_strdup(check_name_cache(name) ? : ""); > > > > > > > > > > I follow this it would ignore the owner address and use the cache name > > > > > or set "" to be resolved shouldn't that use the owner instead? If the > > > > > called don't have it resolved then it should optionally set the owner > > > > > resolution. > > > > if |name| is set, |owner| must be NULL (refer to if block before > > > > proceed: label above). > > > > > > Right so the subsequent change is actually the result of owner being > > > set "", not a bug that existed before. > > > > > > > > > > > > > > + else > > > > > > + data->owner = g_strdup(owner); > > > > > > data->path = g_strdup(path); > > > > > > data->interface = g_strdup(interface); > > > > > > data->member = g_strdup(member); > > > > > > @@ -534,8 +539,12 @@ static DBusHandlerResult > > > > > > message_filter(DBusConnection *connection, > > > > > > if (!sender && data->owner) > > > > > > continue; > > > > > > > > > > > > - if (data->owner && g_str_equal(sender, data->owner) == FALSE) > > > > > > + if (data->owner && > > > > > > + g_str_equal(sender, data->owner) == FALSE && > > > > > > + data->name && > > > > > > + g_str_equal(sender, data->name) == FALSE) { > > > > > > > > > > iirc messages never use the friendly name only the bus connection as > > > > > sender so I wonder if this really does make any difference, are there > > > > > any example of this not working? Perhaps it would be worth creating a > > > > > test case in unit/test-gdbus.c to capture this case. > > > > There is a case where the sender is D-Bus daemon itself. For example > > > > NameOwnerChanged signal is sent via a message which has > > > > sender="org.freedesktop.dbus" instead of a D-Bus address. I am not > > > > aware of any other case other than messages sent by D-Bus daemon. > > > > > > We could perhaps have the check as if(data->owner && data->owner != > > > '\0'... so we skip the check if it was not resolved yet, in that case > > > we would accept signals for watches which we do not have resolved > > > their bus name resolved yet, which I guess it is the real issue you > > > are trying to fix here. > It should be the opposite: If name is not yet resolved, we don't want > to accept anything because that would be receiving messages from > anybody, which is wrong. Got it, I though the problem was the opposite. > > > > > > > > > > > > > > continue; > > > > > > + } > > > > > > > > > > > > if (data->path && g_str_equal(path, data->path) == FALSE) > > > > > > continue; > > > > > > @@ -627,6 +636,7 @@ static void service_reply(DBusPendingCall *call, > > > > > > void *user_data) > > > > > > DBUS_TYPE_INVALID) == FALSE) > > > > > > goto fail; > > > > > > > > > > > > This is not needed btw, update_name_cache will just lookup the same > > > data and overwrite the owner with the same owner, well except if we do > > > have other data pointing to the same name which is normally not the > > > case of here, or is it? Also Im not sure how service_reply would be > > > triggered since data->owner will always going to be set to either the > > > cache or "", so it looks like this would only work because the bus > > > name is no longer matched. > This is needed because there might be another filter_data which does > not have the name resolved, so we should update them all with the > resolved name. Weird, I though we already grouped the filter_data so the had be updated altogether or is this really for different path, interface, etc? > > > > > > > > > + update_name_cache(data->name, data->owner); > > > > > > update_service(data); > > > > > > > > > > > > goto done; > > > > > > -- > > > > > > 2.21.0 > > > > Are you still planning on working on this, it seems to be valid fix we > > just have to work on the details and I hope I didn't demotivate you > > with my comments. > > > > -- > > Luiz Augusto von Dentz -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-27 8:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-14 4:32 [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation Sonny Sasaka 2019-09-15 17:14 ` Luiz Augusto von Dentz 2019-09-15 17:38 ` Sonny Sasaka 2019-09-16 11:46 ` Luiz Augusto von Dentz 2019-09-19 19:51 ` Luiz Augusto von Dentz 2019-09-20 19:48 ` Sonny Sasaka 2019-09-27 8:43 ` Luiz Augusto von Dentz
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).