[PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation

[PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation

[Sonny Sasaka]

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

Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation

[Luiz Augusto von Dentz]

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

Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation

[Sonny Sasaka]

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

Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation

[Luiz Augusto von Dentz]

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

Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation

[Luiz Augusto von Dentz]

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

Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation

[Sonny Sasaka]

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

Re: [PATCH BlueZ] gdbus: Properly set owner of filter data at start of client creation

[Luiz Augusto von Dentz]

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