[Mesa-dev] [PATCH 7/9] wayland-drm: static inline wayland_drm_buffer_get

[Emil Velikov]

On 23 October 2017 at 16:41, Daniel Stone <daniel at fooishbar.org> wrote:
> Hi Emil,
>
> On 28 September 2017 at 13:36, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 20 September 2017 at 15:06, Daniel Stone <daniel at fooishbar.org> wrote:
>>> On 19 September 2017 at 11:25, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> It looks a bit icky and my Wayland knowledge is limited to actually review it.
>>>>
>>>> I still think that we're trying to do different things - me simplify
>>>> things, while you're focusing on fixing a bug.
>>>
>>> I like the idea of the simplification, but it's just that a) it can't
>>> actually be simplified that far, and b) whilst we still rely on
>>> consistent resolution of wl_buffer_interface, your proposed change may
>>> actually _introduce_ a bug.
>>
>> Right, I did not see that one. Can you share your train of thought?
>
> Happily!
>
> When a compositor creates a client buffer, the request is dispatched
> by libwayland-server.so into libEGL.so (libEGL explicitly links
> libwayland-server). This lands in
> src/egl/wayland/wayland-drm/wayland-drm.c's create_buffer(), which
> calls wl_resource_create() with 'wl_buffer_interface'. Since libEGL.so
> is explicitly linked against both libwayland-client and
> libwayland-server, and both of those DSOs provide a resolution of
> wl_buffer_interface, resource->interface could be
> libwayland-server.so::wl_buffer_interface, or
> libwayland-client.so::wl_buffer_interface.
>
> When a compositor wants to import a buffer into GBM, it calls
> gbm_bo_import(), which will call wl_drm_buffer_get().
> wl_drm_buffer_get will test (resource->interface ==
> &wl_buffer_interface). Previously, both create_buffer() and
> wl_drm_buffer_get() at least came from the same compilation unit, but
> now they are built and linked separately. This seems to make an
> existing problem (ambiguity of 'wl_buffer_interface') worse (symbol
> resolution now performed in separate compilation units).
>
> I don't really see a solution to this, apart from no longer relying on
> having a single canonical resolution of wl_buffer_interface. The above
> patch implements that, by removing the interface-address-equal test
> and replacing it with the destroy-listener test. The patch I provided
> is equally valid both with and without your series.
>
Ouch, that's something I did not expect looking at the Mesa code alone.

Generally, this type of issues are not specific to Mesa, but come due
to the implementation/use of wl_resource_instance_of. Is that correct?

I'm itching to bring back my suggestions to fix this in its root, but
let's ignore that for a moment ;-)

Having a look at the wl_resource_instance_of() implementation

WL_EXPORT int
wl_resource_instance_of(struct wl_resource *resource,
                   const struct wl_interface *interface,
                   const void *implementation)
{
   return wl_interface_equal(resource->object.interface, interface) &&
           resource->object.implementation == implementation;
}

... and wl_interface_equal()

int
wl_interface_equal(const struct wl_interface *a, const struct wl_interface *b)
{
     /* In most cases the pointer equality test is sufficient.
      * However, in some cases, depending on how things are split
      * across shared objects, we can end up with multiple
      * instances of the interface metadata constants.  So if the
      * pointers match, the interfaces are equal, if they don't
      * match we have to compare the interface names.
      */
     return a == b || strcmp(a->name, b->name) == 0;
}

Am I reading it incorrectly, or you forgot about the name check?
It seems like the name check was added explicitly to workaround the
issue you're talking about.

Thus wl_resource_instance_of() should return the same result with or
without my patch?
Or perhaps the interface check is safe, but the implementation one isn't?

Thanks
Emil