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

[Pekka Paalanen]

On Mon, 23 Oct 2017 18:05:12 +0100
Emil Velikov <emil.l.velikov at gmail.com> wrote:

> 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?

Hi Emil,

your analysis is correct, there should be no problem. The
implementation check is safe as well, because the thing that is being
compared there is the 'struct wl_buffer_interface' pointer. Looking at
your original patch, it seems that the implementation check ensures the
wl_buffer was created with the specific 'struct wl_drm' instance and
therefore has no relation to any DSO identity (I assume 'struct wl_drm'
is allocated dynamically).


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171024/c78ffb2e/attachment-0001.sig>