Build failure with glibc 2.26

• Component: WebKitGTK
• Status: NEW ---

[Mike Gorse]

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.

[Mike Gorse]

Created attachment 442735 [details]
Patch.

[Michael Catanzaro]

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.)

[Mike Gorse]

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

[Michael Catanzaro]

Created attachment 442833 [details]
Patch

[Michael Catanzaro]

Please make sure my patch works for you! Thanks.

[Mike Gorse]

+#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).

[Adrian Perez]

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

=)

[Mike Gorse]

Builds for me with the patch, fwiw.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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

[Michael Catanzaro]

Adrian, any further thoughts?

[Michael Catanzaro]

Ping Adrian