Apply FD_CLOEXEC on sockets

[lennyly]

This PR sets the FD_CLOEXEC flag on all sockets created by hiredis to prevent file descriptor leakage across fork() and exec() boundaries.

When a program using hiredis forks, the file descriptors for sockets are inherited by the child process unless FD_CLOEXEC is set. This can lead to hard to diagnose issues, especially in environments that use epoll or changelist-based polling optimizations.

In one such case, the parent process initiated a reconnection, which reused the same file descriptor. However because the original socket object was still "alive" due to being duplicated in the child process, epoll continued to monitor the old underlying file object associated with the same file descriptor. Since epoll watches file objects, not just file descriptors, this caused the epoll instance to incorrectly associate events with the wrong socket.

Under a changelist optimization, this is particularly problematic because epoll_ctl() calls can be skipped, and the internal epoll state may remain stale. As a result, the event loop may enter a tight loop, leading to 100% CPU usage due to busy looping.

By marking sockets with FD_CLOEXEC, we ensure that the file descriptor is automatically closed in any exec()'d child process, allowing the file object to be fully cleaned up and avoiding the above issue.

[michael-grunder]

This seems like a nice improvement. I guess the only question is whether or not it should be set unconditionally or optionally like TCP_KEEPALIVE.

I'm pretty sure FD_CLOEXEC is basically everywhere but I'm thinking of exotic systems or very niche use cases. Hiredis is absolutely everywhere so is there an edge case where someone would not want this flag set?

edit: Thinking about this a bit more, I can see use cases where we would not want this flag set, so personally I think we should put it behind an option.

[lennyly]

To support all use cases I've added an options flag, and also switched to SOCK_CLOEXEC so that the option will be set atomically in socket() instead of later on, thus avoiding the risk of having another thread fork in between socket() and fcntl().

[collinfunk]

To support all use cases I've added an options flag, and also switched to SOCK_CLOEXEC so that the option will be set atomically in socket() instead of later on, thus avoiding the risk of having another thread fork in between socket() and fcntl().

Yes, that is much better. And they are hidden behind #ifdef SOCK_CLOEXEC. Thanks!

One thing. This:

#ifdef SOCK_CLOEXEC
    int flags = SOCK_STREAM;
    if (c->flags & REDIS_OPT_SET_SOCK_CLOEXEC) {
        flags |= SOCK_CLOEXEC;
    }
    if ((s = socket(type, flags, 0)) == REDIS_INVALID_FD) {
        __redisSetErrorFromErrno(c, REDIS_ERR_IO, NULL);
        return REDIS_ERR;
    }
#else
    if ((s = socket(type, SOCK_STREAM, 0)) == REDIS_INVALID_FD) {
        __redisSetErrorFromErrno(c,REDIS_ERR_IO,NULL);
        __redisSetErrorFromErrno(c, REDIS_ERR_IO, NULL);
        return REDIS_ERR;
    }
#endif /* SOCK_CLOEXEC */

Can be made shorter by doing:

   int flags = SOCK_STREAM;
#ifdef SOCK_CLOEXEC
    if (c->flags & REDIS_OPT_SET_SOCK_CLOEXEC) {
        flags |= SOCK_CLOEXEC;
    }
#endif /* SOCK_CLOEXEC */
    if ((s = socket(type, flags, 0)) == REDIS_INVALID_FD) {
        __redisSetErrorFromErrno(c, REDIS_ERR_IO, NULL);
        return REDIS_ERR;
    }

But will let @michael-grunder what he thinks.

[michael-grunder]

Yeah I think that shorter snippit is good. Less duplication.

[lennyly]

Good feedback, thanks!

[michael-grunder]

Just those two minor changes and then it looks good to me.

[collinfunk]

Looks good to me, thanks!

[lennyly]

@michael-grunder do you have any additional comments? The PR currently requires your approval.

[michael-grunder]

LGTM.

Gave it a local spin and it works as expected. Thanks!