GNOME Bugzilla – Bug 653570
eventfd syscall check is unsafe
Last modified: 2011-06-30 18:08:53 UTC
The eventfd support for the mainloop is careful to check that the eventfd() syscall is supported before using it. It does it with the following check: efd = eventfd (0, EFD_CLOEXEC); if (efd == -1 && errno == ENOSYS) the problem is that, according to the manpage: EINVAL An unsupported value was specified in flags. and EFD_CLOEXEC (since Linux 2.6.27) Set the close-on-exec (FD_CLOEXEC) flag on the new file descrip‐ tor. See the description of the O_CLOEXEC flag in open(2) for reasons why this may be useful. So if you have a kernel before 2.6.27, you'll have eventfd but not EFD_CLOEXEC and the syscall will return EINVAL. That will cause you to hit: else g_error ("Cannot create eventfd for main loop wake-up: %s", g_strerror (errno)); Two options: 1) Use pipes in this case 2) Create it and then fnctl() (in the style of g_unix_open_pipe())
side-note: the use of the bitfields in the 'state' structure here is really unnecessarily confusing. Worse, it could possibly lead to some bad effects due to the non-atomic nature of bitfield updates and the fact that you access it without a lock from the supposedly-threadsafe GMainContext. The way you're using it, you could actually just have one variable: gboolean eventfd_unsupported; You can set it to TRUE the when you notice that eventfd() fails and then you can check it on access to the fd to determine if you should treat it as eventfd (FALSE) or pipe (TRUE).
Created attachment 190862 [details] [review] gmain: Fall back to pipes if kernel doesn't support EFD_CLOEXEC for eventfd() Also remove the caching of checking for eventfd; just try it every time, it's cheap enough to do so.
Review of attachment 190862 [details] [review]: Looks good. Nice idea to use the second pipe fd as a flag for having eventfd or not. That's cleaner than my proposed solution. ::: glib/gmain.c @@ +575,3 @@ + else if (efd >= 0) + { + context->wake_up_pipe[0] = efd; should you have context->wake_up_pipe[1] = -1; here?
(In reply to comment #3) > Review of attachment 190862 [details] [review]: > > Looks good. > > Nice idea to use the second pipe fd as a flag for having eventfd or not. > That's cleaner than my proposed solution. Yeah, I came up with it when doing the GCancellable change. > ::: glib/gmain.c > @@ +575,3 @@ > + else if (efd >= 0) > + { > + context->wake_up_pipe[0] = efd; > > should you have context->wake_up_pipe[1] = -1; here? That's actually done in g_main_context_new()
Comment on attachment 190862 [details] [review] gmain: Fall back to pipes if kernel doesn't support EFD_CLOEXEC for eventfd() +1.
Comment on attachment 190862 [details] [review] gmain: Fall back to pipes if kernel doesn't support EFD_CLOEXEC for eventfd() Looks fine to me too