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

Fix systemd socket activation support #491

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix systemd socket activation support #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33bb72d
Fix systemd socket activation support
maxnet Dec 12, 2021
1c24496
Add support for httpd systemd socket activation
maxnet Dec 12, 2021
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
30 changes: 30 additions & 0 deletions libvncserver/httpd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@
#include <tcpd.h>
#endif

#ifdef LIBVNCSERVER_WITH_SYSTEMD
#include <systemd/sd-daemon.h>
#endif


#define NOT_FOUND_STR "HTTP/1.0 404 Not found\r\nConnection: close\r\n\r\n" \
"<HEAD><TITLE>File Not Found</TITLE></HEAD>\n" \
Expand Down Expand Up @@ -100,6 +104,32 @@ rfbHttpInitSockets(rfbScreenInfoPtr rfbScreen)
if (!rfbScreen->httpDir)
return;

#ifdef LIBVNCSERVER_WITH_SYSTEMD
if (sd_listen_fds(0) == 2) {
rfbSocket sock = SD_LISTEN_FDS_START + 1;
if (sd_is_socket(sock, AF_UNSPEC, 0, 0)) {
rfbErr("Systemd socket activation passed 'accept=yes' inetd style socket. Only passing listening sockets is supported for http socket\n");
} else if (sd_is_socket(sock, AF_UNSPEC, 0, 1)) {
struct sockaddr_storage sa;
int sa_len = sizeof(sa);
char hoststr[NI_MAXHOST];
char portstr[NI_MAXSERV];

if (getsockname(sock, (struct sockaddr *) &sa, &sa_len) == 0
&& getnameinfo((struct sockaddr *)&sa, sa_len, hoststr, sizeof(hoststr),
portstr, sizeof(portstr), NI_NUMERICHOST | NI_NUMERICSERV) == 0) {
rfbLog("Socket activation through systemd. Listening for HTTP connections on %s:%s\n", hoststr, portstr);
} else {
rfbLogPerror("Error retrieving nameinfo of socket received from systemd\n");
}

rfbScreen->httpListenSock = sock;
}

return;
}
#endif

if (rfbScreen->httpPort == 0) {
rfbScreen->httpPort = rfbScreen->port-100;
}
Expand Down
24 changes: 21 additions & 3 deletions libvncserver/sockets.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,31 @@ rfbInitSockets(rfbScreenInfoPtr rfbScreen)
rfbScreen->socketState = RFB_SOCKET_READY;

#ifdef LIBVNCSERVER_WITH_SYSTEMD
if (sd_listen_fds(0) == 1)
if (sd_listen_fds(0) == 1 || sd_listen_fds(0) == 2)
{
rfbSocket sock = SD_LISTEN_FDS_START + 0;
if (sd_is_socket(sock, AF_UNSPEC, 0, 0))
rfbNewConnectionFromSock(rfbScreen, sock);
else if (sd_is_socket(sock, AF_UNSPEC, 0, 1))
rfbProcessNewConnection(rfbScreen);
else if (sd_is_socket(sock, AF_UNSPEC, 0, 1)) {
struct sockaddr_storage sa;
int sa_len = sizeof(sa);
char hoststr[NI_MAXHOST];
char portstr[NI_MAXSERV];

if (getsockname(sock, (struct sockaddr *) &sa, &sa_len) == 0
&& getnameinfo((struct sockaddr *)&sa, sa_len, hoststr, sizeof(hoststr),
portstr, sizeof(portstr), NI_NUMERICHOST | NI_NUMERICSERV) == 0) {
rfbLog("Socket activation through systemd. Listening for VNC connections on %s:%s\n", hoststr, portstr);
rfbScreen->port = atoi(portstr);
Copy link
Member

Choose a reason for hiding this comment

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

if there's no ListenStream=nnnn it falls back to the default port? Would you be able to add some quick docs about the different systemd approaches to the README?

Copy link
Contributor Author

@maxnet maxnet Apr 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

if there's no ListenStream=nnnn it falls back to the default port?

In that case sd_listen_fds(0) will return 0, which is tested in the "if" some lines above, and the code quoted will not be reached.
Instead it will print "Unable to establish connection with systemd socket\n" to log (not sure if that log message is desired, but that was/is the existing behavior if libvncserver is compiled with systemd support), and fallback to default port.

Would you be able to add some quick docs about the different systemd approaches to the README?

Not sure if a sample systemd file wouldn't be better in x11vnc repo.

Although I left it there because the existing code had support for it, the accept=yes approach is something you would normally NOT use, unless you have some very strange use-case where you want to have a separate VNC server program instance running for each TCP connection opened.
Not sure if you want to document such rare corner case.
One would normally only use the approach without "accept=yes" for x11vnc and the like, where a single program instance manages all client connections.

Copy link
Member

Choose a reason for hiding this comment

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

TBH, I'm lacking the knowledge how this is supposed to work. Can you quickly sketch out for me what system activation does in the "accept=yes" mode and in this PR's approach?

My rough understanding is that systemd activation is similar to inetd...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, I'm lacking the knowledge how this is supposed to work. Can you quickly sketch out for me what system activation
does in the "accept=yes" mode and in this PR's approach?

This PR does not change anything regarding accept=yes mode.
Which is indeed similar to inetd in that it launches a new VNC server program every time there is a new TCP connection.
And which I would NOT recommend.

What the PR does fix is the normal systemd socket activation mode (accept=no, which is the default), which was broken.
With that systemd will start the VNC server program the first time a connection to the port is made, and passes the listening socket to the program.
The program will then behave the same way as when it was started normally without systemd, and can use that listening socket to accept further connections.
So x11vnc can have 10 clients, but there will only be one x11vnc program like normal (when running with -shared).
Main advantage of using systemd socket activation VS starting the program normally, is that the program will only be started if there is an actual connection.
And no memory will be wasted if you are not using VNC on this boot.

Copy link
Contributor Author

@maxnet maxnet Apr 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Note that from the code, this is the important part that does the real work, that simply starts using the listening socket it gets passed through from systemd (and accepts the first connection, that we know is pending):

       FD_ZERO(&(rfbScreen->allFds));
       rfbScreen->listenSock = sock;
       FD_SET(rfbScreen->listenSock, &(rfbScreen->allFds));
       rfbScreen->maxFd = rfbScreen->listenSock;
       rfbProcessNewConnection(rfbScreen);

The part at the top is not strictly necessary.
It tries to figure out what the original listening port was that was configured in systemd, and puts that in rfbScreen->port here:

       struct sockaddr_storage sa;
       int sa_len = sizeof(sa);
       char hoststr[NI_MAXHOST];
       char portstr[NI_MAXSERV];

       if (getsockname(sock, (struct sockaddr *) &sa, &sa_len) == 0
           && getnameinfo((struct sockaddr *)&sa, sa_len, hoststr, sizeof(hoststr),
                           portstr, sizeof(portstr), NI_NUMERICHOST | NI_NUMERICSERV) == 0) {
           rfbLog("Socket activation through systemd. Listening for VNC connections on %s:%s\n", hoststr, portstr);
           rfbScreen->port = atoi(portstr);

I added that because x11vnc prints out the port number that is in rfbScreen->port to the log output.
And it looks weird if it says port 0 there.
But is not strictly necessary for the socket code to function.

} else {
rfbLogPerror("Error retrieving nameinfo of socket received from systemd\n");
}

FD_ZERO(&(rfbScreen->allFds));
rfbScreen->listenSock = sock;
FD_SET(rfbScreen->listenSock, &(rfbScreen->allFds));
rfbScreen->maxFd = rfbScreen->listenSock;
}
return;
}
else
Expand Down