Recognizing wl_display pointer for EGL (Re: Smart comparing of wl_display_interfaces)

[Pekka Paalanen]

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;


Thanks,
pq