Apply FD_CLOEXEC on sockets #1301
Conversation
|
This seems like a nice improvement. I guess the only question is whether or not it should be set unconditionally or optionally like I'm pretty sure 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. |
…mically in socket()
|
To support all use cases I've added an options flag, and also switched to |
Yes, that is much better. And they are hidden behind 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. |
|
Yeah I think that shorter snippit is good. Less duplication. |
|
Good feedback, thanks! |
Co-authored-by: Michael Grunder <michael.grunder@gmail.com>
Co-authored-by: Michael Grunder <michael.grunder@gmail.com>
|
Looks good to me, thanks! |
|
@michael-grunder do you have any additional comments? The PR currently requires your approval. |
This PR sets the
FD_CLOEXECflag on all sockets created by hiredis to prevent file descriptor leakage acrossfork()andexec()boundaries.When a program using hiredis forks, the file descriptors for sockets are inherited by the child process unless
FD_CLOEXECis 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 anyexec()'d child process, allowing the file object to be fully cleaned up and avoiding the above issue.