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@xxxxxxxxx> wrote:
>
> Hi Sonny,
>
> On Mon, Sep 16, 2019 at 2:46 PM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > 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.
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
