-
Notifications
You must be signed in to change notification settings - Fork 3.2k
bhyve: Use getaddrinfo(3) consistently for AF_UNIX. #1958
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.
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
base: main
Are you sure you want to change the base?
Conversation
291b23e to
554971d
Compare
554971d to
e873aae
Compare
|
I refactored the
After the path is resolved, its length may be longer than the original. I checked the path length after resolving. And @hrs-allbsd told me that getaddrinfo(3) doesn't see the ai_flags in case of AF_UNIX. I changed to set the ai_flags unconditionally. The unlink(2) of the UNIX domain socket path serves a similar purpose to setsockopt(SO_REUSEADDR) if it were a TCP socket. The unlink(2) and the setsockopt(2) are exclusive among AF_UNIX and other cases. The AF_UNIX condition should separate unlink(2) and setsockopt(2). I removed the umask(2) call. It affects process-wide change. The bhyve process runs multiple threads; umask(2) may affect other threads, leading to unexpected behaviour. To change the UNIX domain socket permission, I use the chmod(2). The addrinfo structure is no longer needed after the bind(2) call. I changed freeing it after the bind(2) and removed the freeaddrinfo(3) call from the error: label. The intention is to improve maintainability by narrowing the effective scope of the addrinfo. Because the error: label is far away from the bind(2) line. |
The FreeBSD getaddrinfo(3) function also supports the AF_UNIX address family. Use it the same way as inet and inet6 so that we can share the same code. However, the getaddrinfo(3) accepts an absolute path only. The path string that follows 'unix:' will be resolved by the realpath(3) function. The addrinfo structure is no longer needed after the bind(2) call. Free it after the bind(2) to remove freeaddrinfo(3) in the error label code. The freeaddrinfo(3) function also supports a NULL argument, as does free(3). No need to check a NULL pointer. While I'm here, the UNIX domain socket should allow all users to access it as if it were a TCP port. Changes the permission to world wide readable and writable. Signed-off-by: Yuichiro NAITO <[email protected]>
e873aae to
f7f97e1
Compare
|
I refactored the |
markjdb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the comment, this looks ok, though I'm not sure that it's really an improvement, as it adds a bit more code than it deletes.
| } | ||
|
|
||
| if (family == AF_UNIX && chmod(hostname, | ||
| S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious to me that this is a desirable change: maybe one uses a unix socket specifically because it prevents unprivileged users from connecting to the socket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the original commit message and the review, a UNIX domain socket is used as an alternative to a network port on a jail that doesn't have network connectivity. Usually, all users can connect to a network port. If a UNIX domain socket is limited to a privileged user, it won't be an alternative. I just honor the original intention.
|
The reason why my patch code gets longer is to support a relative path to a UNIX domain socket. If it's not needed, we can remove the |
The FreeBSD getaddrinfo(3) function also supports the AF_UNIX address family. Use it the same way as inet and inet6 so that we can share the same code. However, the getaddrinfo(3) accepts an absolute path only. The path string that follows 'unix:' will be resolved by the realpath(3) function.
The freeaddrinfo(3) function also supports a NULL argument, as does free(3). No need to check a NULL pointer.
While I'm here, the UNIX domain socket should allow all users to access it as if it were a TCP port. Changes umask to 0 before bind(2), and restores the previous value after the bind(2).