[core] prevent socket fd leaks to child processes #1465
Conversation
|
You are right with this fix, thanks for finding it. I think this needs some improvements, however. The definition of So what I think needs improvement is:
Might even be that for the sake of portability it would be even easier to rely exclusively on |
|
@ethouris , thank you for your reply, you are totally right, I made some changes, use O_CLOEXEC instead of SO_CLOEXEC, and create again without O_CLOEXEC if the call failed, is there anything I missed? |
|
@ethouris , I found my pull request was base on an old version, what should I do? Should I close this pull request and create an new one base on the lasted version, or just update my fork repository and do the merge? |
update from the official repository
|
As for "old version": If Github doesn't report conflicts, don't worry. If it does, got to your repository, update the upstream master branch (pull), then switch to your branch and do "merge" from master. Your fix is still wrong, unfortunately. The |
|
Ok, that's much better. For Windows I found also something like: If you have no possibility to check if it works on Windows - don't worry, just please add the above in the comment somewhere and add some "XXX" so that it can catch an eye. We'll fix and test it ourselves later. I need you to add this on your branch because in this case we can simply merge your branch directly. |
|
Sorry, I can't check it on Windows, the comment already added, thank you for your patience. |
|
Hi @aogun I wonder if you have some workflows with forked processes utilizing SRT? I am thinking it might be better to forbid such forking in SRT, would that be possible. |
|
No, sharing the same UDP socket is not my expect, in my use case, there have a process A which call srt create a socket and bind port X on it, after that at some time A needs call popen to execute command, it will create an sub process and the sub process will hold the udp socket inherit from parent, then you will get failed when you reopen the socket and bind port X in A during subprocess running, add O_CLOEXEC flag will avoid such problem, I suppose forbid process to use popen is not a good idea. |
|
Looks like there is no clean way to prevent a descriptor to be shared with child process.
|
|
@maxsharabayko Fedora Docs mentioned two ways, the first is set close-on-exec flag, and the doc figured out the downside of this approach is needs set the flag to every descriptors which sub process not needed. The second way is loop over file descriptors and close it before creating a new process image, but there's no safe and efficiency way to loop all the file descriptors.
and gstreamer, uWebsocket, etc. |
|
Hi @aogun Looking at the code again, why are #if defined(_AIX) || \
defined(__APPLE__) || \
defined(__DragonFly__) || \
defined(__FreeBSD__) || \
defined(__FreeBSD_kernel__) || \
defined(__linux__) || \
defined(__OpenBSD__) || \
defined(__NetBSD__)
#define set_cloexec set_cloexec_ioctl
#else
#define set_cloexec set_cloexec_fcntl
#endif
#if !defined(__CYGWIN__) && !defined(__MSYS__) && !defined(__HAIKU__)
static int set_cloexec_ioctl(int fd, int set) {
int r;
do
r = ioctl(fd, set ? FIOCLEX : FIONCLEX);
while (r == -1 && errno == EINTR);
if (r)
return errno;
return 0;
}
#endifShould not the whole section be the following? Or even just defining the #ifndef _WIN32
#if defined(_AIX) || \
defined(__APPLE__) || \
defined(__DragonFly__) || \
defined(__FreeBSD__) || \
defined(__FreeBSD_kernel__) || \
defined(__linux__) || \
defined(__OpenBSD__) || \
defined(__NetBSD__)
#define set_cloexec set_cloexec_ioctl
static int set_cloexec_ioctl(int fd, int set) {
int r;
do
r = ioctl(fd, set ? FIOCLEX : FIONCLEX);
while (r == -1 && errno == EINTR);
if (r)
return errno;
return 0;
}
#else
#define set_cloexec set_cloexec_fcntl
static int set_cloexec_fcntl(int fd, int set) {
int flags;
int r;
do
r = fcntl(fd, F_GETFD);
while (r == -1 && errno == EINTR);
if (r == -1)
return errno;
/* Bail out now if already set/clear. */
if (!!(r & FD_CLOEXEC) == !!set)
return 0;
if (set)
flags = r | FD_CLOEXEC;
else
flags = r & ~FD_CLOEXEC;
do
r = fcntl(fd, F_SETFD, flags);
while (r == -1 && errno == EINTR);
if (r)
return errno;
return 0;
}
#endif
#endif // #ifndef _WIN32 |
|
you are right, just define set_cloexec would be better, there's no need to define different ifdef. |
|
@maxsharabayko Socket leak test case passed if turn on option ENABLE_SOCK_CLOEXEC, failed if ENABLE_SOCK_CLOEXEC turn off, the PR #1562 works good. |
|
@aogun Thank you for the confirmation! |
Child processed created with fork share the file descriptors with their parent process, if we fork a child process after startup a srt listener, the child process will hold the socket after we close it in the parent process, then the parent can't listen on the same port again. So I suggest set the SOCK_CLOEXEC flag to prevent socket leaks to child processes.