Hi Sonny,
On Sun, Sep 15, 2019 at 8:38 PM Sonny Sasaka <sonnysasaka@xxxxxxxxx> wrote:
>
> Hi Luiz,
>
> On Sun, Sep 15, 2019 at 10:14 AM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > Hi Sonny,
> >
> > On Sat, Sep 14, 2019 at 5:57 PM Sonny Sasaka <sonnysasaka@xxxxxxxxx> 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
