Recognizing wl_display pointer for EGL (Re: Smart comparing of wl_display_interfaces)
Jason Ekstrand
jason at jlekstrand.net
Sun Oct 19 08:41:36 PDT 2014
On Oct 19, 2014 12:03 AM, "Pekka Paalanen" <ppaalanen at gmail.com> wrote:
>
> On Sat, 18 Oct 2014 11:19:55 -0700
> Jason Ekstrand <jason at jlekstrand.net> wrote:
>
> > On Thu, Oct 16, 2014 at 11:16 PM, Pekka Paalanen <ppaalanen at gmail.com>
> > wrote:
> >
> > > On Thu, 16 Oct 2014 10:40:20 +0300
> > > Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > >
> > > > On Thu, 16 Oct 2014 09:46:39 +0300
> > > > Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > > >
> > > > > On Wed, 15 Oct 2014 22:44:25 +0400
> > > > > Dmitry Cherkassov <dcherkassov at gmail.com> wrote:
> > > > >
> > > > > > Hi list!
> > > > > >
> > > > > > The definition of wl_display_interface symbol can come from
> > > > > > libwayland-server.so or libwayland-client.so.
> > > > > >
> > > > > > There is a code in closed source EGL implementation that does
> > > > > > interfaces comparison via pointers, yielding negative results
when
> > > > > > addresses are different (one address is saved by QtWayland, the
other
> > > > > > one is from &wl_display_interface in EGL source code).
> > > > > >
> > > > > > Is there a smarter way to compare wl_display_interface's?
> > > > > >
> > > > > > There is this function:
> > > > > >
> > > > > > wl_interface_equal(const struct wl_interface *a, const struct
> > > wl_interface *b)
> > > > > > {
> > > > > > return a == b || strcmp(a->name, b->name) == 0;
> > > > > > }
> > > > > >
> > > > > > But as you can see it's not safe to use it alone because in
case of
> > > > > > bad pointer we will get undefined results.
> > > > > >
> > > > > > Currently struct wl_interface is the following:
> > > > > > struct wl_interface {
> > > > > > const char *name;
> > > > > > int version;
> > > > > > int method_count;
> > > > > > const struct wl_message *methods;
> > > > > > int event_count;
> > > > > > const struct wl_message *events;
> > > > > > };
> > > > > >
> > > > > > Since there is no magic field at the beginning it is not
possible to
> > > > > > see whether a pointer points to the valid wl_interface
structure.
> > > > > >
> > > > > > So my question is: how can we compare wl_interfaces in more
clever
> > > and
> > > > > > safer way?
> > > > >
> > > > > Hmm, why would you ever have a bad pointer?
> > > > >
> > > > > Bad pointers in general tend to cause crashes and stuff, so you
> > > > > shouldn't be having a bad one in the first place.
> > > > >
> > > > > What is the background for your question/problem?
> > > >
> > > > Ok, I suppose this was discussed in IRC last night: the actual
problem
> > > > behind the question is how to detect whether the void pointer
> > > > (EGLNativeDisplayType) given to eglCreateDisplay is actually a
> > > > wl_display.
> > > >
> > > > That's tough.
> > > >
> > > > A couple of hacks come to mind.
> > > >
> > > > bool
> > > > _eglNativePlatformDetectWayland(void *nativeDisplay)
> > > > {
> > > > void *f;
> > > >
> > > > if (!dereferencable(nativeDisplay))
> > > > return false;
> > > >
> > > > f = *(void **)nativeDisplay;
> > > >
> > > > The first is to dlopen both libwayland-client and libwayland-server
in
> > > > turn with RTLD_NOLOAD | RTLD_LOCAL, and fetching the
> > > > wl_display_interface symbol from each, comparing those to 'f'. If
> > > > either matches, return true.
> > >
> > > While discussing in IRC, I realized half of this is nonsense.
> > >
> > > eglGetDisplay() can only get a wl_display created by
libwayland-client,
> > > and never one created by libwayland-server. Assuming the dynamic
> > > linking is sane, the interface pointer in wl_display is *always* the
> > > one from libwayland-client.so.
> > >
> > > Now we just need to make sure, that we compare 'f' to the interface
> > > pointer from libwayland-client.so. That could probably be done with
the
> > > dlopen/dlsym trick, or...
> > >
> > > > Another would be to check if 'f' is dereferencable (and not equal to
> > > > the default wl_display_interface), and check if (char *)f starts
with
> > > > "wl_d". If yes, do a full string comparison with "wl_display". It's
> > > > not reliable, but maybe works enough.
> > > >
> > > > The proper solution is for apps to use EGL_KHR_platform_wayland to
> > > > explicitly say the pointer is a wl_display. That of course doesn't
work
> > > > on old apps, so you might implement a workaround through environment
> > > > like Mesa does: EGL_PLATFORM=wayland bypasses all autodetection and
> > > > assumes it is a wl_display.
> > > >
> > > > In any case, EGL really should implement EGL_KHR_platform_wayland
so we
> > > > have a chance to get rid of the inherently unreliable autodetect.
And we
> > > > should start using that in our demo apps...
> > > >
> > > > If we consider autodetecting client wl_display pointers worth to
solve
> > > > properly, we should probably add a function for that in
> > > > libwayland-client.so. However, implementing that portably might be
> > > > difficult.
> > >
> > > ...adding a function int/bool wl_is_wl_display_pointer(void *p) into
> > > libwayland-client.so. As that function would be built into
> > > libwayland-client.so, it naturally uses the client.so version of
> > > wl_display_interface symbol (unless dynamic linking miraculously
> > > manages to mess that up?).
> > >
> > > 'p' would be the tentative 'struct wl_display *', so that the
> > > implementation details of struct wl_display would be contained in
> > > libwayland-client.so, and not leaked elsewhere like e.g. in Mesa.
> > >
> > > All this means we should be able to get away with just one pointer
> > > comparison, after assuring that the first sizeof(void*) bytes pointed
> > > to by the given unknown pointer (EGLNativeDisplayType) are
> > > dereferencable without a segfault or other fatal error.
> > >
> > > So, a quick fix for EGL implementations right now would be to try the
> > > dlopen hack, until they can start to depend on a new
> > > libwayland-client.so providing wl_is_wl_display_pointer().
> > >
> > > I am unsure about the exact definition of wl_is_wl_display_pointer():
> > > can it do the dereferencability check itself, or should we require the
> > > caller to guarantee that dereferencing cannot crash. The latter would
> > > leave the portability burden on the callers rather than libwayland.
> > >
> > > How's that sound?
> > >
> >
> > That sounds fine. I think we can count on the client passing in a valid
> > pointer to something. Chances are that whatever the client passes in
will
> > be a pointer to some sort of structure and so we can probably read 8
bytes
> > without a problem. If we really wanted to, we could do a quick
alignment
> > check to ensure that it's sizeof(void *)-aligned.
>
> There are perfectly valid cases, where the value passed in to
> eglGetDisplay is non-zero and not a valid pointer, see below.
>
> The exact question is, do we expect the caller have already
> done the referenceability test, or do we promise to do that
> ourselves inside is_wl_display function?
>
> For the reference, this is the check in Mesa:
> /**
> * Perform validity checks on a generic pointer.
> */
> static EGLBoolean
> _eglPointerIsDereferencable(void *p)
> {
> #ifdef HAVE_MINCORE
> uintptr_t addr = (uintptr_t) p;
> unsigned char valid = 0;
> const long page_size = getpagesize();
>
> if (p == NULL)
> return EGL_FALSE;
>
> /* align addr to page_size */
> addr &= ~(page_size - 1);
>
> if (mincore((void *) addr, page_size, &valid) < 0) {
> _eglLog(_EGL_DEBUG, "mincore failed: %m");
> return EGL_FALSE;
> }
>
> return (valid & 0x01) == 0x01;
> #else
> return p != NULL;
> #endif
> }
>
> From
>
http://cgit.freedesktop.org/mesa/mesa/tree/src/egl/main/egldisplay.c?id=1fc124b80f228319ded06f80a51681c75dc0a4f3#n110
>
> Bill seemed to think, that the only non-pointer value we need check
> against is zero (NULL), but that is not the case.
>
> The value passed as and cast into EGLNativeDisplayType could also
> be an arbitrary integer (raspberry pi proprietary EGL), file
> descriptor (Mesa DRM platform), or anything really. It's up to the
> EGL platform to define what it is, and the EGL platform is what we
> are desperately trying to autodetect when the application is not
> using EGL_EXT_platform_base -based platform indication.
>
> > I don't know that I care whether we do your wl_is_wl_display_pointer or
> > Bill's "dynamic cast" version. It doesn't much matter.
>
> Yeah, doesn't matter, can do it either way, this is a special case
> that will not happen again, is a fallback path, will only ever
> be used inside libEGL implementations, etc.
>
> > > Is it possible to have several instances of libwayland-client in a
> > > process address space? Meaning we would have several different
> > > client-side wl_display_interface pointers?
> > >
> >
> > I don't think so. At least not without some very bizarre hackery.
Also, I
> > think it's reasonable to assume that either a) libEGL is reasonably
> > normally linked against the same chunks of code that link to
> > libwayland-client or b) The client really knows what they're doing and
is
> > prepared for the consequences. Because really, if they're pulling those
> > kinds of tricks they're asking for trouble.
>
> I hope so too.
>
> > > If yes, it's back to the string comparisons...
> > >
> >
> > String comparisons are right out. They'll cause a segfault the moment
it's
> > not a wl_display pointer.
>
> No, it could work. For the EGLNativeDisplayType cast to void*:
>
> 1. check it is not NULL
> 2. check it is dereferencable for at least sizeof(void*) bytes
> 3. extract a void *f from the memory pointed to by it
> 4. check 'f' is dereferencable for N=sizeof("wl_display") bytes
> 5. check the first N bytes at address 'f' against "wl_display"
>
> Steps 1-3 have to be done in any case. Steps 4-5 are the string
> comparison case. As we already checked that the bytes can be read,
> the string comparison should be safe.
>
> But yes, I'd rather not do that.
>
> Still, something has to do the first dereferecability test in step
> 2, and it is not really portable I believe, so the question is who
> do we require to do that?
>
> I think I'd be tempted to say that its the libEGL, since it's
> probably fairly OS dependent anyway. That only works when we
> don't need a dereferencability test for 'f', so no problem. That
> would make is_wl_display(arg) to be as simple as
> return *(void**)arg == &wl_display_interface;
Fine with me.
--Jason
>
> Thanks,
> pq
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141019/81249f38/attachment-0001.html>
More information about the wayland-devel
mailing list