Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

workaround for F_DUPFD_CLOEXEC on Linux kernel <2.6.24 before it existed #30357

Closed
wants to merge 1 commit into from
+7 −0
Closed

workaround for F_DUPFD_CLOEXEC on Linux kernel <2.6.24 before it existed #30357

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,15 @@ int uv_dup(uv_os_fd_t fd, uv_os_fd_t* dupfd) {
}
#else
int uv_dup(uv_os_fd_t fd, uv_os_fd_t* dupfd) {
// F_DUPFD_CLOEXEC only available since Linux 2.6.24
#ifdef F_DUPFD_CLOEXEC
if ((*dupfd = fcntl(fd, F_DUPFD_CLOEXEC, 3)) == -1)
return -errno;
#else
if ((*dupfd = fcntl(fd, F_DUPFD, 3)) == -1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be unconditionally compiled and be used if the F_DUPFD_CLOEXEC one fails. In another word, this is only handling the compile time kernel header version, not the runtime kernel version. The generic binary, for example, are likely going to be used on a different kernel than the one it is compiled on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it might be undefined behavior to use F_DUPFD_CLOEXEC on older kernels? Is that okay?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether that's undefined or not, nothing in this PR can stop that. The change I request only make it better if it wasn't undefined.

And in general, I believe the kernel API are written so that passing a newer flag to an older kernel should be fine. You'll just get a EINVAL if you do that. The kernel does all the checks to make sure the flag you pass in is what it understand before it proceed. It should even be fine to define that value manually when compiling with an older header. It just might be arch dependent (it's not syscall number so hopefully not but one need to check) and generally doesn't worth the effort unless the buildbot has an old kernel. See also fcntl(3p)

   EINVAL The cmd argument is invalid, or the cmd argument is F_DUPFD or F_DUPFD_CLOEXEC  and
         arg  is  negative  or  greater  than or equal to {OPEN_MAX}, or the cmd argument is
         F_GETLK, F_SETLK, or F_SETLKW and the data pointed to  by  arg  is  not  valid,  or
         fildes refers to a file that does not support locking.

FWIW, I think UB is mostly a C thing. I don't think the kernel is in that business probably mostly for security and backward compatibility reasons....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there's a more complete version of this as libuv/libuv#1994

return -errno;
fcntl(fd, F_SETFD, FD_CLOEXEC);
#endif
return 0;
}
#endif
Expand Down