Use O_CLOEXEC for exec-safety in MMDB_open

[blblack]

Otherwise, a program which might concurrently call MMDB_open() and
fork()->exec() in different threads of execution cannot prevent
the possible leak of a copy of the temporary database filehandle
to the new child process.

[horgh]

Thanks @blblack! I think this is a good idea. However, as you can see the Travis build failed. It looks like that's because we specify POSIX.1-2001 specifically and O_CLOEXEC was specified in POSIX.1-2008.

Out of concern for portability, I'm hesitant to bump our POSIX version unless it's very critical. Is the lack of close-on-exec causing particular problems for you?

[blblack]

Yes, the issue is I have a daemon which (re-)loads maxminddb updates in a separate pthread as they become available (filesystem notification), and then the main thread can also do fork->exec() cycles on user demand for downtime-less restarts.

The race window is short, but without atomic close-on-exec at creation time there's no way to avoid the issue here, and the leaked FDs would potentially build over time with every exec()-based restart of the daemon.

The only alternative in a pure POSIX.1-2001 world is to use fcntl() immediately after the open to set the FD_CLOEXEC bit. This makes the window much shorter, but doesn't completely solve the problem.

Another alternative would be something like:

#ifndef O_CLOEXEC
#  define O_CLOEXEC 0
#endif

Which would get it working right for the OS's that do support O_CLOEXEC without requiring the standards bump.

[horgh]

I see. Thanks for the explanation!

Yeah, fcntl() doesn't seem like a full solution either for the reasons you mention.

I think using the #ifndef workaround would work only if we bumped _POSIX_C_SOURCE, as otherwise it will never be defined.

I wonder if there is anything that could be done on the application side. Would something like a global lock before forking or opening the database solve this? Another idea would be to close all open fds you can after fork+exec. Working around above the library is not ideal either of course, but it might be a way out.

Another idea might be to look into conditionally using POSIX 2008 here. Or maybe I'm wrong and we don't need to worry about keeping POSIX 2001! I wonder what systems we'd be losing compatibility with.

[blblack]

Yeah it's a deep and thorny issue, sorry! The "close all fds" approach isn't considered sane for various other reasons in the general case, and the mutex solution between unrelated concerns is dicey to impose on apps as well. The bottom line is that atomic close-on-exec enhancements to various POSIX APIs that create file descriptors is the only way out of this mess, but it has taken a long time for standards to catch up.

If you want to burn some time looking down this rabbithole, the general wider-scope issue with all the other fd-creating interfaces is well-documented in http://austingroupbugs.net/view.php?id=411 . Most *nixes have adopted most of the most-important fixups from that link ahead of the standards being published, but there's definitely some questions about portability there since there's been no updated POSIX standard since. FreeBSD has a nice list of the issues here too: https://wiki.freebsd.org/AtomicCloseOnExec

Luckily this particular case (O_CLOEXEC for open(2)) is already in POSIX.1-2008, unlike some others.

On a pragmatic level for libmaxminddb, if you want to keep to strict POSIX.1-2001, another path might be to offer an interface that takes an already-open fd that the app manages. e.g. MMDB_open_fd(int fd, uint32_t flags, MMDB_s *const mmdb). I'm not really sure as to the impact of raising the bar to POSIX.1-2008 on OS compatibility levels. I know for MacOS, they didn't support O_CLOEXEC until 10.7.

[horgh]

Thanks for the details! It's definitely an interesting problem.

I've been thinking about this more. I like your API addition suggestion, as well as the possibility of conditionally bumping the entire library to defaulting to POSIX 2008 if it's available. However, I think your #ifndef idea might be the simplest and a way we can target this precisely. Sorry for dismissing it initially!

I missed the opportunity we have from conditionally defining _POSIX_C_SOURCE:

#ifndef _POSIX_C_SOURCE
#define _POSIX_C_SOURCE 200112L
#endif

So if an app defined it to POSIX 2008 prior to including the library, we'd have O_CLOEXEC available indeed!

Then where we go to call open(2), we can check if we've exposed POSIX 2008 functionality, and use it if so. Something like:

    int flags = O_RDONLY;
#if _POSIX_C_SOURCE >= 200809L
    flags |= O_CLOEXEC;
#endif
    int fd = open(mmdb->filename, flags);

This means we'd be able to keep defaulting to 2001 (and so avoid mistakenly breaking compatibility), but be able to use 2008 features, like this one, where they're useful.

What do you think?

[blblack]

Seems like a possibly sane approach! I think you'll probably also need to add #include <unistd.h> to maxminddb.h, before the _POSIX_C_SOURCE check (because that's what will define it to 2008+ on a newer system).

[horgh]

Hmm, yeah. We'd have to do that or something like build the library with -D_POSIX_C_SOURCE=200809L. Not ideal.

Oh, there's _POSIX_VERSION we can check to determine what the implementation provided, separate from what we asked for with _POSIX_C_SOURCE. We could change to defaulting to _POSIX_C_SOURCE=200809L in maxminddb.h. Then when we need to use features that are in a particular standard, check _POSIX_VERSION rather than _POSIX_C_SOURCE:

#if _POSIX_C_VERSION == 200809L
    flags |= O_CLOEXEC;
#endif

That way building the library we'll get up to and including 2008 by default, and be able to handle it if implementations give us something else.

We'd want to add builds to Travis where we specifically request _POSIX_C_SOURCE=200112L (probably with CFLAGS) as well so we maintain compatibility.

This does bump us to 2008 in a way, but as long as we keep testing with 2001, we'd still support both.

Would you be interested in updating your PR to an alternate approach like this? Otherwise I can do so.

[blblack]

Yeah, I think your approach above makes sense, the more I read about various related things. To recap for gauranteed clarity:

1. change maxminddb.h's default _POSIX_C_SOURCE value to 200809L
2. wrap use of O_CLOEXEC in a _POSIX_VERSION >= 200809L check
3. Explicitly test building with -D_POSIX_C_SOURCE=200112L to ensure that any use of 2008 features is optional going forward

The source changes are pretty trivial and I'm not sure about other concerns around your travis build list and how you want to do (3) above, and life is busy at the moment. If you want to patch it up yourself and close off this PR I won't be offended :)

[horgh]

Your recap is exactly right!

Okay, I will take a stab at this. Thank you for your help figuring it out!

[horgh]

I've made a new PR: #163. I'll close this one in favour of it. Thanks again!