maxmind/libmaxminddb

FD_CLOEXEC set twice on systems that support O_CLOEXEC

rittneje opened this issue · 5 comments

#275 added an unconditional call to fcntl to set FD_CLOEXEC. However, this call should only be made on systems that do not support O_CLOEXEC. While setting it a second time won't cause a problem per se, it is not necessary.

One possible fix is to change this to #if defined(FD_CLOEXEC) && !defined(O_CLOEXEC).

#ifdef FD_CLOEXEC
int fd_flags = fcntl(fd, F_GETFD);
if (fd_flags >= 0) {
fcntl(fd, F_SETFD, fd_flags | FD_CLOEXEC);
}
#endif

cc @oschwald

Sorry for the confusion on this point by the way. When I wrote that snippet in #273 I was thinking of it more in isolation.

I actually did have #if !defined(O_CLOEXEC) && defined(FD_CLOEXEC) before committing, but looking at other project, I noticed most others did not bother with the O_CLOEXEC check. I removed it as it seemed harmless to use fcntl in this case and I wasn't sure if it was ever possible for O_CLOEXEC to be defined but for open to not support the flag.

I believe that if O_CLOEXEC were defined but open somehow did not support it, you could also get EINVAL back at runtime.

I guess another thing you could do is check the flags that come back from F_GETFD and only call F_SETFD if FD_CLOEXEC isn't already set. That at least cuts out one extraneous syscall.

Yeah, checking the flags might be a good compromise to eliminate the extra syscall. Things like this OpenJDK check are what gave me pause about O_CLOEXEC being available but not working. I assume it would likely only happen with a very old Linux kernel, but my confidence is pretty low.

That is describing a bug in AIX where open will return EINVAL. https://www.ibm.com/support/pages/apar/IV90804

Additional changes would be needed to make that work since it is a runtime failure (and it would have been broken since this library started using O_CLOEXEC in 1.3.2 anyway).