On Oct 19, 2014 12:03 AM, "Pekka Paalanen" <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote: > > On Sat, 18 Oct 2014 11:19:55 -0700 > Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > > > On Thu, Oct 16, 2014 at 11:16 PM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> > > wrote: > > > > > On Thu, 16 Oct 2014 10:40:20 +0300 > > > Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote: > > > > > > > On Thu, 16 Oct 2014 09:46:39 +0300 > > > > Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com">ppaalanen@gmail.com</a>> wrote: > > > > > > > > > On Wed, 15 Oct 2014 22:44:25 +0400 > > > > > Dmitry Cherkassov <<a href="mailto:dcherkassov@gmail.com">dcherkassov@gmail.com</a>> 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 > <a href="http://cgit.freedesktop.org/mesa/mesa/tree/src/egl/main/egldisplay.c?id=1fc124b80f228319ded06f80a51681c75dc0a4f3#n110">http://cgit.freedesktop.org/mesa/mesa/tree/src/egl/main/egldisplay.c?id=1fc124b80f228319ded06f80a51681c75dc0a4f3#n110</a> > > 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