The build fails with glibc 2.26 after the fix for bug 231479. We have this conditional #if !defined(MFD_ALLOW_SEALING) && HAVE(LINUX_MEMFD_H) guarding the definition for our memfd_create implementation, but now MEMFD_ALLOW_SEALING can be defined, even if glibc hasn't defined it, because it might be defined in linux/memfd.h, which now we've already included.
Created attachment 442735 [details] Patch.
Comment on attachment 442735 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=442735&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:51 > -#if !defined(MFD_ALLOW_SEALING) && HAVE(LINUX_MEMFD_H) > +#if HAVE(LINUX_MEMFD_H) && !__GLIBC_PREREQ(2, 27) Mmmm... we have to be friendly to other libcs, though. I think this will go badly if __GLIBC_PREREQ is not defined. I'm also suspicious that this guard no longer matches the guard around #include <linux/memfd.h>. I think the intent is: if the libc supports memfd_create(), then MFD_ALLOW_SEALING is defined in sys/mman.h and we can use the libc memfd_create(). If not, then #include linux/memfd and define everything ourselves. But after my commit, that's broken, because #including linux/memfd.h can define MFD_ALLOW_SEALING. Maybe a simpler solution would be to just revert this back to how it was before? The style checker will not be happy -- it whines when the #includes are not alphabetical -- but in this case we seem to have a good reason to ignore it. I would also add a warning comment to indicate why the #includes have to be in a special order. Sound good? Then we don't need __GLIBC_PREREQ, which seems fragile. (Especially bad for things like uclibc-ng, which pretends to be glibc.)
Thanks for the review. That's fine with me. I was assuming that glibc would be in use because linux/memfd.h would only exist on Linux, but I wasn't thinking about uclibc-ng...
Created attachment 442833 [details] Patch
Please make sure my patch works for you! Thanks.
+#include <linux/memfd.h> Does this need to be guarded by #if HAVE_LINUX_FD_H? The old code had the entire block guarded by both. Anyway, I'm testing a build on SLE-15-SP2 (which is where I was seeing the failure).
Comment on attachment 442833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442833&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:-38 > - May I suggest this solution, which works everywhere, for all the libc implementations we support: #if __has_include(<linux/memfd.h>) #include <linux/meemfd.h> #endif #ifndef MFD_ALLOW_SEALING #define MFD_ALLOW_SEALING 0x0002U #endif =)
Builds for me with the patch, fwiw.
(In reply to Adrian Perez from comment #7) > May I suggest this solution, which works everywhere, for all the libc > implementations we support: > > #if __has_include(<linux/memfd.h>) > #include <linux/meemfd.h> > #endif Again, there is no point to this guard because the build cannot succeed without it. There is no fallback for lack of linux/memfd.h if memfd stuff isn't defined in sys/mman.h, and we cannot add one. I could add an #error to fail the build more cleanly in this case, though. > #ifndef MFD_ALLOW_SEALING > #define MFD_ALLOW_SEALING 0x0002U > #endif > > =) That's not right, because if we #include linux/memfd.h, then we have MFD_ALLOW_SEALING for sure.
> we cannot add one Note I'm assuming lack of linux/memfd.h indicates old kernel. If we're just missing kernel headers, hmm, that we *could* try to handle. But then we have to fail at runtime rather than build time as syscall() could fail on ancient kernels.
Adrian, any further thoughts?
Ping Adrian