[PATCH v3] Add systemd socket launching support

[PATCH v3] Add systemd socket launching support

[Matthew Leach]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

Hi all,

Here is a new version of the systemd integration patches.  This time
I've remembered to include the documentation updates.  Speaking of
which, I'm not sure whether I should include the below unit samples as a
guide for people that are wanting to a use socket-activated Emacs
server; I'm not sure whether it is appropriate or where they should go.

emacs.socket
============
[Socket]
ListenStream=/home/foobar/test.socket

[Install]
WantedBy=sockets.target

emacs.service
=============
[Service]
Type=forking
ExecStart=/path/to/emacs --daemon

Feedback & comments welcome!

Changes since v1
================
* Call sd_listen_fds with a paramter of 1 to remove the state that
  systemd sets up.
* Check the socket's validity when passed from systemd.
* Remove lisp functions to simplify the API.
* Change the name of the paramter passed to make_network_process to
  ':usepassedsocket' to keep things more generic.

Changes since v2
================
* Add documentation and NEWS section.
* Rename :usepassedsockets to :use-systemd-socket.

Thanks,
-- 
Matt


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-for-libsystemd-when-building-Emacs.patch --]
[-- Type: text/x-patch, Size: 3037 bytes --]

From a4146c3db79923f6befc9acad730a4e891684a6f Mon Sep 17 00:00:00 2001
From: Matthew Leach <matthew@mattleach.net>
Date: Sat, 26 Mar 2016 16:41:17 +0000
Subject: [PATCH v3 1/4] Check for libsystemd when building Emacs.

* configure.ac: Add new default-on option systemd and check for
  necessary systemd libraries at configure time.
* src/Makefile.in: Add libsystemd library and C flags to the Emacs
  compilation options.
---
 configure.ac    | 13 +++++++++++++
 src/Makefile.in |  6 +++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index f3846f4..d3628d2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -330,6 +330,7 @@ OPTION_DEFAULT_ON([tiff],[don't compile with TIFF image support])
 OPTION_DEFAULT_ON([gif],[don't compile with GIF image support])
 OPTION_DEFAULT_ON([png],[don't compile with PNG image support])
 OPTION_DEFAULT_ON([rsvg],[don't compile with SVG image support])
+OPTION_DEFAULT_ON([systemd],[don't compile with systemd support])
 OPTION_DEFAULT_OFF([cairo],[compile with Cairo drawing (experimental)])
 OPTION_DEFAULT_ON([xml2],[don't compile with XML parsing support])
 OPTION_DEFAULT_ON([imagemagick],[don't compile with ImageMagick image support])
@@ -2716,6 +2717,18 @@ fi
 AC_SUBST(LIBGNUTLS_LIBS)
 AC_SUBST(LIBGNUTLS_CFLAGS)
 
+HAVE_SYSTEMD=no
+if test "${with_systemd}" = "yes" ; then
+  EMACS_CHECK_MODULES([LIBSYSTEMD], [libsystemd >= 226],
+    [HAVE_SYSTEMD=yes], [HAVE_SYSTEMD=no])
+  if test "${HAVE_SYSTEMD}" = "yes"; then
+    AC_DEFINE(HAVE_SYSTEMD, 1, [Define if using systemd.])
+  fi
+fi
+
+AC_SUBST(LIBSYSTEMD_LIBS)
+AC_SUBST(LIBSYSTEMD_CFLAGS)
+
 NOTIFY_OBJ=
 NOTIFY_SUMMARY=no
 
diff --git a/src/Makefile.in b/src/Makefile.in
index c290a60..fc9360a 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -307,6 +307,9 @@ LIBSELINUX_LIBS = @LIBSELINUX_LIBS@
 LIBGNUTLS_LIBS = @LIBGNUTLS_LIBS@
 LIBGNUTLS_CFLAGS = @LIBGNUTLS_CFLAGS@
 
+LIBSYSTEMD_LIBS = @LIBSYSTEMD_LIBS@
+LIBSYSTEMD_CFLAGS = @LIBSYSTEMD_CFLAGS@
+
 INTERVALS_H = dispextern.h intervals.h composite.h
 
 GETLOADAVG_LIBS = @GETLOADAVG_LIBS@
@@ -372,6 +375,7 @@ ALL_CFLAGS=-Demacs $(MYCPPFLAGS) -I. -I$(srcdir) \
   $(WEBKIT_CFLAGS) \
   $(SETTINGS_CFLAGS) $(FREETYPE_CFLAGS) $(FONTCONFIG_CFLAGS) \
   $(LIBOTF_CFLAGS) $(M17N_FLT_CFLAGS) $(DEPFLAGS) \
+  $(LIBSYSTEMD_CFLAGS) \
   $(LIBGNUTLS_CFLAGS) $(NOTIFY_CFLAGS) $(CAIRO_CFLAGS) \
   $(WARN_CFLAGS) $(WERROR_CFLAGS) $(CFLAGS)
 ALL_OBJC_CFLAGS=$(ALL_CFLAGS) $(GNU_OBJC_CFLAGS)
@@ -489,7 +493,7 @@ LIBES = $(LIBS) $(W32_LIBS) $(LIBS_GNUSTEP) $(LIBX_BASE) $(LIBIMAGE) \
    $(LIBS_TERMCAP) $(GETLOADAVG_LIBS) $(SETTINGS_LIBS) $(LIBSELINUX_LIBS) \
    $(FREETYPE_LIBS) $(FONTCONFIG_LIBS) $(LIBOTF_LIBS) $(M17N_FLT_LIBS) \
    $(LIBGNUTLS_LIBS) $(LIB_PTHREAD) $(GETADDRINFO_A_LIBS) \
-   $(NOTIFY_LIBS) $(LIB_MATH) $(LIBZ) $(LIBMODULES)
+   $(NOTIFY_LIBS) $(LIB_MATH) $(LIBZ) $(LIBMODULES) $(LIBSYSTEMD_LIBS)
 
 $(leimdir)/leim-list.el: bootstrap-emacs$(EXEEXT)
 	$(MAKE) -C ../leim leim-list.el EMACS="$(bootstrap_exe)"
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Read-the-number-of-sockets-passed-by-systemd.patch --]
[-- Type: text/x-patch, Size: 2096 bytes --]

From d28c2a5c97a14f3fc83b3665ba28deefbeee805e Mon Sep 17 00:00:00 2001
From: Matthew Leach <matthew@mattleach.net>
Date: Sat, 26 Mar 2016 18:50:14 +0000
Subject: [PATCH v3 2/4] Read the number of sockets passed by systemd.

* src/emacs.c (systemd_socket): New variable for storing the socket
  descriptor passed in by systemd.
  (main): Call sd_listen_fds to read the number of sockets passed.
---
 src/emacs.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/src/emacs.c b/src/emacs.c
index 95d1905..df5fade 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -56,6 +56,11 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <binary-io.h>
 #endif
 
+#ifdef HAVE_SYSTEMD
+#include <systemd/sd-daemon.h>
+#include <sys/socket.h>
+#endif /* HAVE_SYSTEMD */
+
 #ifdef HAVE_WINDOW_SYSTEM
 #include TERM_HEADER
 #endif /* HAVE_WINDOW_SYSTEM */
@@ -195,6 +200,12 @@ int daemon_pipe[2];
 HANDLE w32_daemon_event;
 #endif
 
+#ifdef HAVE_SYSTEMD
+/* The socket descriptor passed by systemd.  If nothing has been
+   passed, this will be 0. */
+int systemd_socket = 0;
+#endif /* HAVE_SYSTEMD */
+
 /* Save argv and argc.  */
 char **initial_argv;
 int initial_argc;
@@ -997,6 +1008,24 @@ main (int argc, char **argv)
 	  exit (1);
 	}
 
+#ifdef HAVE_SYSTEMD
+      /* Read the number of sockets passed through by systemd. */
+      systemd_socket = sd_listen_fds(1);
+
+      if (systemd_socket > 1)
+        {
+          fprintf (stderr, "\nWarning: systemd has passed more than one socket to the Emacs process.\n\
+Try adding 'Accept=false' in the Emacs socket unit file.\n");
+          systemd_socket = 0;
+        }
+      else if (systemd_socket == 1 &&
+               sd_is_socket (SD_LISTEN_FDS_START,
+                             AF_UNSPEC, SOCK_STREAM, 1) >= 0)
+        systemd_socket = SD_LISTEN_FDS_START;
+      else
+        systemd_socket = 0;
+#endif
+
 #ifndef DAEMON_MUST_EXEC
 #ifdef USE_GTK
       fprintf (stderr, "\nWarning: due to a long standing Gtk+ bug\nhttp://bugzilla.gnome.org/show_bug.cgi?id=85715\n\
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-Allow-network-processes-to-be-made-with-a-pre-alloca.patch --]
[-- Type: text/x-patch, Size: 5858 bytes --]

From d971465937c971beb2886ad59827005f69546247 Mon Sep 17 00:00:00 2001
From: Matthew Leach <matthew@mattleach.net>
Date: Sat, 26 Mar 2016 19:37:10 +0000
Subject: [PATCH v3 3/4] Allow network processes to be made with a
 pre-allocated fd.

* src/process.c (connect_network_socket): Allow a pre-allocated
socket descriptor to be used if passed to Emacs, avoiding the call
to socket() and bind().
(Fmake_network_process): Allow users to pass in :use-systemd-socket on
the parameter plist to use a socket descriptor that has been passed to
Emacs from systemd.
(wait_reading_process_output): Call socket() & bind() every time.
(syms_of_process): New symbol ":use-passed-socket".
* doc/lispref/processes.texi (Network Processes): Document new
  `make-network-process' option ':use-systemd-socket'.
---
 doc/lispref/processes.texi |  6 ++++++
 src/process.c              | 36 +++++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/doc/lispref/processes.texi b/doc/lispref/processes.texi
index 8d3df55..1d43c25 100644
--- a/doc/lispref/processes.texi
+++ b/doc/lispref/processes.texi
@@ -2367,6 +2367,12 @@ automatically for the given @var{host} and @var{service}.
 ignored.  @code{ipv4} and @code{ipv6} specify to use IPv4 and IPv6,
 respectively.
 
+@item :use-systemd-socket @var{use-systemd-socket}
+If @var{use-systemd-socket} is non-@code{nil} and Emacs was passed a
+network socket by systemd, use that socket instead of allocating one.
+This is used by the Emacs server code to allow on-demand socket
+activation.
+
 @item :local @var{local-address}
 For a server process, @var{local-address} is the address to listen on.
 It overrides @var{family}, @var{host} and @var{service}, so you
diff --git a/src/process.c b/src/process.c
index 198e7de..28d2631 100644
--- a/src/process.c
+++ b/src/process.c
@@ -3075,7 +3075,8 @@ finish_after_tls_connection (Lisp_Object proc)
 #endif
 
 static void
-connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses)
+connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses,
+                        Lisp_Object use_systemd_socket_p)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
   ptrdiff_t count1;
@@ -3089,6 +3090,16 @@ connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses)
   struct Lisp_Process *p = XPROCESS (proc);
   Lisp_Object contact = p->childp;
   int optbits = 0;
+  int systemd_socket_descriptor = 0;
+
+#ifdef HAVE_SYSTEMD
+  if (!NILP (use_systemd_socket_p))
+    {
+      extern int systemd_socket;
+      systemd_socket_descriptor = systemd_socket;
+    }
+#endif /* HAVE_SYSTEMD */
+
 
   /* Do this in case we never enter the while-loop below.  */
   count1 = SPECPDL_INDEX ();
@@ -3109,7 +3120,11 @@ connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses)
       sa = xmalloc (addrlen);
       conv_lisp_to_sockaddr (family, ip_address, sa, addrlen);
 
-      s = socket (family, p->socktype | SOCK_CLOEXEC, p->ai_protocol);
+      if (systemd_socket_descriptor)
+          s = systemd_socket_descriptor;
+      else
+          s = socket (family, p->socktype | SOCK_CLOEXEC, p->ai_protocol);
+
       if (s < 0)
 	{
 	  xerrno = errno;
@@ -3168,8 +3183,11 @@ connect_network_socket (Lisp_Object proc, Lisp_Object ip_addresses)
 		  report_file_error ("Cannot set reuse option on server socket", Qnil);
 	      }
 
-	  if (bind (s, sa, addrlen))
-	    report_file_error ("Cannot bind server socket", Qnil);
+          /* If we are passed a socket descriptor from systemd, it is
+             already bound. */
+	  if (!systemd_socket_descriptor)
+	    if (bind (s, sa, addrlen))
+	      report_file_error ("Cannot bind server socket", Qnil);
 
 #ifdef HAVE_GETSOCKNAME
 	  if (p->port == 0)
@@ -3534,6 +3552,8 @@ The following network options can be specified for this connection:
                       (this is allowed by default for a server process).
 :bindtodevice NAME -- bind to interface NAME.  Using this may require
                       special privileges on some systems.
+:use-systemd-socket BOOL -- Use any pre-allocated sockets that have
+                            been passed to Emacs by systemd.
 
 Consult the relevant system programmer's manual pages for more
 information on using these options.
@@ -3578,7 +3598,7 @@ usage: (make-network-process &rest ARGS)  */)
   EMACS_INT port = 0;
   Lisp_Object tem;
   Lisp_Object name, buffer, host, service, address;
-  Lisp_Object filter, sentinel;
+  Lisp_Object filter, sentinel, use_systemd_socket_p;
   Lisp_Object ip_addresses = Qnil;
   int socktype;
   int family = -1;
@@ -3618,6 +3638,7 @@ usage: (make-network-process &rest ARGS)  */)
   buffer = Fplist_get (contact, QCbuffer);
   filter = Fplist_get (contact, QCfilter);
   sentinel = Fplist_get (contact, QCsentinel);
+  use_systemd_socket_p = Fplist_get (contact, QCuse_systemd_socket);
 
   CHECK_STRING (name);
 
@@ -3914,7 +3935,7 @@ usage: (make-network-process &rest ARGS)  */)
     }
 #endif
 
-  connect_network_socket (proc, ip_addresses);
+  connect_network_socket (proc, ip_addresses, use_systemd_socket_p);
   return proc;
 }
 
@@ -4848,7 +4869,7 @@ wait_reading_process_output (intmax_t time_limit, int nsecs, int read_kbd,
 		  {
 		    Lisp_Object ip_addresses = check_for_dns (aproc);
 		    if (!NILP (ip_addresses) && !EQ (ip_addresses, Qt))
-		      connect_network_socket (aproc, ip_addresses);
+		      connect_network_socket (aproc, ip_addresses, Qnil);
 		    else
 		      retry_for_async = true;
 		  }
@@ -7837,6 +7858,7 @@ syms_of_process (void)
   DEFSYM (QCserver, ":server");
   DEFSYM (QCnowait, ":nowait");
   DEFSYM (QCsentinel, ":sentinel");
+  DEFSYM (QCuse_systemd_socket, ":use-systemd-socket");
   DEFSYM (QCtls_parameters, ":tls-parameters");
   DEFSYM (Qnsm_verify_connection, "nsm-verify-connection");
   DEFSYM (QClog, ":log");
-- 
2.7.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-When-Emacs-is-passed-a-socket-descriptor-make-the-se.patch --]
[-- Type: text/x-patch, Size: 2924 bytes --]

From 8804c97495a4b210447ce6195956526017f4d188 Mon Sep 17 00:00:00 2001
From: Matthew Leach <matthew@mattleach.net>
Date: Sat, 26 Mar 2016 20:43:26 +0000
Subject: [PATCH v3 4/4] When Emacs is passed a socket descriptor, make the
 server listen on it.

* lisp/server.el (server-start): Set :use-systemd-socket to `t' when
  calling `make-network-process'.
* etc/NEWS: Document new systemd functionality and build option to
  disable it.
* doc/emacs/misc.texi (Emacs Server): Document systemd socket passing
  functionality.

squash! When Emacs is passed a socket descriptor, make the server listen on it.
---
 doc/emacs/misc.texi | 9 +++++++++
 etc/NEWS            | 7 +++++++
 lisp/server.el      | 1 +
 3 files changed, 17 insertions(+)

diff --git a/doc/emacs/misc.texi b/doc/emacs/misc.texi
index b5a2150..d805648 100644
--- a/doc/emacs/misc.texi
+++ b/doc/emacs/misc.texi
@@ -1580,6 +1580,15 @@ option.  @xref{Initial Options}.  When Emacs is started this way, it
 calls @code{server-start} after initialization, and returns control to
 the calling terminal instead of opening an initial frame; it then
 waits in the background, listening for edit requests.
+
+@cindex socket activation, systemd, Emacs
+@item
+If Emacs has been built with @command{systemd} support, the Emacs
+server can be started by socket activation.  The @command{systemd}
+service creates a socket and listens for connections on it; when
+@command{emacsclient} connects to it for the first time,
+@command{systemd} can launch the Emacs server and hands over the
+socket to it for servicing @command{emacsclient} connections.
 @end itemize
 
 @cindex @env{TEXEDIT} environment variable
diff --git a/etc/NEWS b/etc/NEWS
index 66777e9..9afed5b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -26,6 +26,13 @@ otherwise leave it unmarked.
 * Installation Changes in Emacs 25.2
 
 +++
+** Emacs now has systemd support.  This allows socket based
+activation, where systemd can invoke the Emacs process upon a
+socket connection and hand over the socket to Emacs.  Emacs will
+use this socket for communicating with the emacsclient program.
+This can be disabled with the new configure option
+'--disable-systemd'.
+
 ** New configure option '--disable-build-details' attempts to build an
 Emacs that is more likely to be reproducible; that is, if you build
 and install Emacs twice, the second Emacs is a copy of the first.
diff --git a/lisp/server.el b/lisp/server.el
index 5243820..2c9113c 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -655,6 +655,7 @@ server or call `\\[server-force-delete]' to forcibly disconnect it."))
 		       :noquery t
 		       :sentinel #'server-sentinel
 		       :filter #'server-process-filter
+		       :use-systemd-socket t
 		       ;; We must receive file names without being decoded.
 		       ;; Those are decoded by server-process-filter according
 		       ;; to file-name-coding-system.  Also don't get
-- 
2.7.4

Re: [PATCH v3] Add systemd socket launching support

[Stefan Monnier]

> +#include <systemd/sd-daemon.h>
[..]
> +#ifdef HAVE_SYSTEMD
> +      /* Read the number of sockets passed through by systemd. */
> +      systemd_socket = sd_listen_fds(1);

AFAICT, this is the core of the dependency to systemd.
Could you explain to us, how the socket information is actually passed?
Is it passed via an env-var?


        Stefan

Re: [PATCH v3] Add systemd socket launching support

[Matthew Leach]

Hi Stefan,

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +#include <systemd/sd-daemon.h>
> [..]
>> +#ifdef HAVE_SYSTEMD
>> +      /* Read the number of sockets passed through by systemd. */
>> +      systemd_socket = sd_listen_fds(1);
>
> AFAICT, this is the core of the dependency to systemd.

Yes, that's correct.  There is also a call to `sd_is_socket' just below
this line.

> Could you explain to us, how the socket information is actually
> passed?  Is it passed via an env-var?

I believe so, I'm not a systemd expert, though.  Looking at the man page
for the sd_listen_fds:

Internally, sd_listen_fds() checks whether the $LISTEN_PID environment
variable equals the daemon PID. If not, it returns
immediately. Otherwise, it parses the number passed in the $LISTEN_FDS
environment variable, then sets the FD_CLOEXEC flag for the parsed
number of file descriptors starting from SD_LISTEN_FDS_START. Finally,
it returns the parsed number.

HTH,
-- 
Matt

Re: [PATCH v3] Add systemd socket launching support

[Eli Zaretskii]

> From: Matthew Leach <matthew@mattleach.net>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 30 Mar 2016 21:53:36 +0100
> 
> Here is a new version of the systemd integration patches.  This time
> I've remembered to include the documentation updates.

Thanks.

It looks like the :use-systemd-socket argument is silently ignored if
the socket is not available, or if Emacs wasn't compiled with
libsystemd support, is that right?  If so, the documentation should
say so.

> Speaking of which, I'm not sure whether I should include the below
> unit samples as a guide for people that are wanting to a use
> socket-activated Emacs server; I'm not sure whether it is
> appropriate or where they should go.

In the comments, and perhaps also in the Emacs user manual.

>  +++
> +** Emacs now has systemd support.  This allows socket based
> +activation, where systemd can invoke the Emacs process upon a
> +socket connection and hand over the socket to Emacs.  Emacs will
> +use this socket for communicating with the emacsclient program.
> +This can be disabled with the new configure option
> +'--disable-systemd'.

Would it be more appropriate to use "libsystemd" instead of "systemd"
here?

Re: [PATCH v3] Add systemd socket launching support

[Matthew Leach]

Hi Eli,

Thanks for taking another look.

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Matthew Leach <matthew@mattleach.net>
>> Cc: Eli Zaretskii <eliz@gnu.org>
>> Date: Wed, 30 Mar 2016 21:53:36 +0100
>> 
>> Here is a new version of the systemd integration patches.  This time
>> I've remembered to include the documentation updates.
>
> Thanks.
>
> It looks like the :use-systemd-socket argument is silently ignored if
> the socket is not available, or if Emacs wasn't compiled with
> libsystemd support, is that right?  

Correct.

> If so, the documentation should say so.

Sure, I'll update the patches.


[...]

>> +'--disable-systemd'.
>
> Would it be more appropriate to use "libsystemd" instead of "systemd"
> here?

Agreed, I link against 'libsystemd' rather than 'systemd'.  Should I
also update the configure flag to '--disable-libsystemd'?

Thanks,
-- 
Matt

Re: [PATCH v3] Add systemd socket launching support

[Eli Zaretskii]

> From: Matthew Leach <matthew@mattleach.net>
> Cc: emacs-devel@gnu.org
> Date: Thu, 31 Mar 2016 17:51:06 +0100
> 
> >> +'--disable-systemd'.
> >
> > Would it be more appropriate to use "libsystemd" instead of "systemd"
> > here?
> 
> Agreed, I link against 'libsystemd' rather than 'systemd'.  Should I
> also update the configure flag to '--disable-libsystemd'?

That was my intent, yes.

Re: [PATCH v3] Add systemd socket launching support

[Stefan Monnier]

> Internally, sd_listen_fds() checks whether the $LISTEN_PID environment
> variable equals the daemon PID. If not, it returns
> immediately. Otherwise, it parses the number passed in the $LISTEN_FDS
> environment variable, then sets the FD_CLOEXEC flag for the parsed
> number of file descriptors starting from SD_LISTEN_FDS_START. Finally,
> it returns the parsed number.

Then I think it would be better to follow an approach like the one with
which you started (i.e. without the internal systemd_socket variable),
so the systemd-specific code is limited to just a function that returns
an FD, and then everything else is code that could be used for other
purposes (such as xinetd) and can be explained/documented independently
from systemd.


        Stefan "IIUC"

Re: [PATCH v3] Add systemd socket launching support

[Matthew Leach]

Hi Stefan,

Thanks for taking a look.

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Internally, sd_listen_fds() checks whether the $LISTEN_PID environment
>> variable equals the daemon PID. If not, it returns
>> immediately. Otherwise, it parses the number passed in the $LISTEN_FDS
>> environment variable, then sets the FD_CLOEXEC flag for the parsed
>> number of file descriptors starting from SD_LISTEN_FDS_START. Finally,
>> it returns the parsed number.
>
> Then I think it would be better to follow an approach like the one with
> which you started (i.e. without the internal systemd_socket variable),

Could I clarify, would you like me to expose these variables to lisp,
like I did in v1 of the patches?  Or make the internal variable names
non-systemd specific?
-- 
Matt

Re: [PATCH v3] Add systemd socket launching support

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Thu, 31 Mar 2016 13:11:25 -0400
> Cc: emacs-devel@gnu.org
> 
> > Internally, sd_listen_fds() checks whether the $LISTEN_PID environment
> > variable equals the daemon PID. If not, it returns
> > immediately. Otherwise, it parses the number passed in the $LISTEN_FDS
> > environment variable, then sets the FD_CLOEXEC flag for the parsed
> > number of file descriptors starting from SD_LISTEN_FDS_START. Finally,
> > it returns the parsed number.
> 
> Then I think it would be better to follow an approach like the one with
> which you started (i.e. without the internal systemd_socket variable),

The original patch included the internal variable.

And I object to exposing file descriptors to Lisp, certainly when that
is not necessary.

Re: [PATCH v3] Add systemd socket launching support

[Stefan Monnier]

> And I object to exposing file descriptors to Lisp, certainly when that
> is not necessary.

In other similar functionality I've seen over the years, an alternative
was to pass the fd as an additional argument on the command line, but in
any case whether it's received from the environment or the command line,
the file-descriptor to use will just be an integer, and it's already
exposed to Lisp.

So, AFAICT we just need 3 elements:
- a function to get the FD number from systemd (which we could write in
  Elisp, but is much better implemented as a call to systemd's support
  library).
- a way to create a process-object from an existing file-descriptor number.
- changes in server.el to connect the previous 2 elements together.

AFAICT, only the first part is systemd-specific, so I think it would
make sense to try and avoid having systemd-specific details leak to the
other parts.


        Stefan

Re: [PATCH v3] Add systemd socket launching support

[Matthew Leach]

Hi Stefan,

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> And I object to exposing file descriptors to Lisp, certainly when that
>> is not necessary.
>
> In other similar functionality I've seen over the years, an alternative
> was to pass the fd as an additional argument on the command line, but in
> any case whether it's received from the environment or the command line,
> the file-descriptor to use will just be an integer, and it's already
> exposed to Lisp.
>
> So, AFAICT we just need 3 elements:
> - a function to get the FD number from systemd (which we could write in
>   Elisp, but is much better implemented as a call to systemd's support
>   library).
> - a way to create a process-object from an existing file-descriptor
> number.

Should the socket descriptor number be passed in from Lisp, or should
this be a Boolean flag that tells make-network-process to consult an
internal variable?  We can separate the libsystemd code from the
make-network-process logic easily in both scenarios.

Thanks,
-- 
Matt

Re: [PATCH v3] Add systemd socket launching support

[Matthew Leach]

Matthew Leach <matthew@mattleach.net> writes:

> Hi Stefan,
>
> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
>>> And I object to exposing file descriptors to Lisp, certainly when that
>>> is not necessary.
>>
>> In other similar functionality I've seen over the years, an alternative
>> was to pass the fd as an additional argument on the command line, but in
>> any case whether it's received from the environment or the command line,
>> the file-descriptor to use will just be an integer, and it's already
>> exposed to Lisp.
>>
>> So, AFAICT we just need 3 elements:
>> - a function to get the FD number from systemd (which we could write in
>>   Elisp, but is much better implemented as a call to systemd's support
>>   library).
>> - a way to create a process-object from an existing file-descriptor
>> number.
>
> Should the socket descriptor number be passed in from Lisp, or should
> this be a Boolean flag that tells make-network-process to consult an
> internal variable?  We can separate the libsystemd code from the
> make-network-process logic easily in both scenarios.

I forgot to mention, my vote is for latter as it prevents users passing
in garbage to make-network-process.
-- 
Matt

Re: [PATCH v3] Add systemd socket launching support

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: matthew@mattleach.net, emacs-devel@gnu.org
> Date: Thu, 31 Mar 2016 14:14:01 -0400
> 
> In other similar functionality I've seen over the years, an alternative
> was to pass the fd as an additional argument on the command line, but in
> any case whether it's received from the environment or the command line,
> the file-descriptor to use will just be an integer, and it's already
> exposed to Lisp.

It's an attack vector expecting to happen, so we should try to avoid
that.

> So, AFAICT we just need 3 elements:
> - a function to get the FD number from systemd (which we could write in
>   Elisp, but is much better implemented as a call to systemd's support
>   library).
> - a way to create a process-object from an existing file-descriptor number.
> - changes in server.el to connect the previous 2 elements together.
> 
> AFAICT, only the first part is systemd-specific, so I think it would
> make sense to try and avoid having systemd-specific details leak to the
> other parts.

I agree, and the last version of the patch already does that: it only
requires a boolean flag exposed to other APIs.

Re: [PATCH v3] Add systemd socket launching support

[Stefan Monnier]

>> In other similar functionality I've seen over the years, an alternative
>> was to pass the fd as an additional argument on the command line, but in
>> any case whether it's received from the environment or the command line,
>> the file-descriptor to use will just be an integer, and it's already
>> exposed to Lisp.
> It's an attack vector expecting to happen, so we should try to avoid
> that.

I'm not sure what kind of attack you're thinking about.  And I'm not
sure how the latest patch would address it (you can still cause Emacs
to use an arbitrary FD by providing the corresponding envvars).

>> AFAICT, only the first part is systemd-specific, so I think it would
>> make sense to try and avoid having systemd-specific details leak to the
>> other parts.
> I agree, and the last version of the patch already does that: it only
> requires a boolean flag exposed to other APIs.

But the process part of the patch could theoretically be used in other
contexts, fundamentally, whereas it ends up being tied to systemd, so
although it's code that can be compiled and could work regardless of
systemd, it ends up being systemd-specific since it only works when used
in the systemd-way and only if you linked with the systemd library.


        Stefan

Re: [PATCH v3] Add systemd socket launching support

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: matthew@mattleach.net,  emacs-devel@gnu.org
> Date: Thu, 31 Mar 2016 17:22:35 -0400
> 
> >> In other similar functionality I've seen over the years, an alternative
> >> was to pass the fd as an additional argument on the command line, but in
> >> any case whether it's received from the environment or the command line,
> >> the file-descriptor to use will just be an integer, and it's already
> >> exposed to Lisp.
> > It's an attack vector expecting to happen, so we should try to avoid
> > that.
> 
> I'm not sure what kind of attack you're thinking about.

I'm not sure myself, but I'm not a criminal, so don't ask me about the
details.  I just know that exposing a handle through which someone can
feed Emacs any commands they want is a bad idea.

> And I'm not sure how the latest patch would address it (you can
> still cause Emacs to use an arbitrary FD by providing the
> corresponding envvars).

That'd require a more serious breach of the system's security.

And anyway, that's not our problem, that's the systemd problem.  If
they don't protect their sockets against such attacks, they should get
their act together and do it.  And users should be aware of such
problems with systemd, and refrain from configuring Emacs to use it,
IF such problems indeed exist (I don't know if they do).

But what happens inside Emacs is _our_ responsibility.

> >> AFAICT, only the first part is systemd-specific, so I think it would
> >> make sense to try and avoid having systemd-specific details leak to the
> >> other parts.
> > I agree, and the last version of the patch already does that: it only
> > requires a boolean flag exposed to other APIs.
> 
> But the process part of the patch could theoretically be used in other
> contexts, fundamentally, whereas it ends up being tied to systemd, so
> although it's code that can be compiled and could work regardless of
> systemd, it ends up being systemd-specific since it only works when used
> in the systemd-way and only if you linked with the systemd library.

Do you know about any other Free software that offers similar
services?  If you do, by all means let's design an interface to be
less specific to one of them.  But if no such alternative exists, I'd
rather have something with as small a signature as possible, and worry
about extending that when we have a real contender.

Re: [PATCH v3] Add systemd socket launching support

[Stefan Monnier]

> I'm not sure myself, but I'm not a criminal, so don't ask me about the
> details.  I just know that exposing a handle through which someone can
> feed Emacs any commands they want is a bad idea.

Which part of my proposal exposes such a handle?  What do you mean by
"exposing"?  The only "exposing" happening is to provide a function that
extracts the FD number from our own process environment.  This function
would be implemented using libsystemd so as to avoid re-implementing it
in Elisp (and having to keep it up-to-date with systemd's way of
passing the FD info), but the actual FD data is already available to
Elisp as well as to any other process running on the same machine
(since the env is usually available via "ps" or "/proc/<pid>/environ").

I don't understand.

>> And I'm not sure how the latest patch would address it (you can
>> still cause Emacs to use an arbitrary FD by providing the
>> corresponding envvars).
> That'd require a more serious breach of the system's security.

No.  That only requires an Emacs compiled with systemd support.
That doesn't require systemd running.


        Stefan

Re: [PATCH v3] Add systemd socket launching support

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: matthew@mattleach.net,  emacs-devel@gnu.org
> Date: Fri, 01 Apr 2016 09:17:32 -0400
> 
> > I'm not sure myself, but I'm not a criminal, so don't ask me about the
> > details.  I just know that exposing a handle through which someone can
> > feed Emacs any commands they want is a bad idea.
> 
> Which part of my proposal exposes such a handle?

You asked for the original version, which did that.  That's the only
proposal I understand you made, and that's what I was talking about.

Re: [PATCH v3] Add systemd socket launching support

[Stefan Monnier]

>> > I'm not sure myself, but I'm not a criminal, so don't ask me about the
>> > details.  I just know that exposing a handle through which someone can
>> > feed Emacs any commands they want is a bad idea.
>> Which part of my proposal exposes such a handle?
> You asked for the original version, which did that.  That's the only
> proposal I understand you made, and that's what I was talking about.

Then I don't know wheat you mean by "expose".  It's already readily
available from the environment, so in which way is it more exposed?
I mean, yes, it's more conveniently available, but it's not like it's
otherwise "hidden" in anyway.


        Stefan

Re: [PATCH v3] Add systemd socket launching support

[Matthew Leach]

Hi Stefan,

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> > I'm not sure myself, but I'm not a criminal, so don't ask me about the
>>> > details.  I just know that exposing a handle through which someone can
>>> > feed Emacs any commands they want is a bad idea.
>>> Which part of my proposal exposes such a handle?
>> You asked for the original version, which did that.  That's the only
>> proposal I understand you made, and that's what I was talking about.
>
> Then I don't know wheat you mean by "expose".  It's already readily
> available from the environment, 

One thing to note is that the call to sd_listen_fds(1) will remove the
environment variables used by systemd to pass over the socket.  Since
this happens before the Lisp interpreter is invoked, this information
isn't easily available from Lisp.
-- 
Matt

Re: [PATCH v3] Add systemd socket launching support

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: matthew@mattleach.net, emacs-devel@gnu.org
> Date: Fri, 01 Apr 2016 14:43:55 -0400
> 
> >> > I'm not sure myself, but I'm not a criminal, so don't ask me about the
> >> > details.  I just know that exposing a handle through which someone can
> >> > feed Emacs any commands they want is a bad idea.
> >> Which part of my proposal exposes such a handle?
> > You asked for the original version, which did that.  That's the only
> > proposal I understand you made, and that's what I was talking about.
> 
> Then I don't know wheat you mean by "expose".  It's already readily
> available from the environment, so in which way is it more exposed?

If systemd exposes its sockets in the environment, it's their bug.
When we do the same, it's ours.

Re: [PATCH v3] Add systemd socket launching support

[Live System User]

Matthew Leach <matthew@mattleach.net> writes:

> Hi Stefan,
>
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> +#include <systemd/sd-daemon.h>
>> [..]
>>> +#ifdef HAVE_SYSTEMD
>>> +      /* Read the number of sockets passed through by systemd. */
>>> +      systemd_socket = sd_listen_fds(1);
>>
>> AFAICT, this is the core of the dependency to systemd.
>
> Yes, that's correct.  There is also a call to `sd_is_socket' just below
> this line.
>
>> Could you explain to us, how the socket information is actually
>> passed?  Is it passed via an env-var?
>
> I believe so, I'm not a systemd expert, though.  Looking at the man page
> for the sd_listen_fds:
>
> Internally, sd_listen_fds() checks whether the $LISTEN_PID environment
> variable equals the daemon PID. If not, it returns
> immediately. Otherwise, it parses the number passed in the $LISTEN_FDS
> environment variable, then sets the FD_CLOEXEC flag for the parsed
> number of file descriptors starting from SD_LISTEN_FDS_START. Finally,
> it returns the parsed number.
>
> HTH,

If Emacs is compiled and configured for systemd integration, would it
still be possible to startup Emacs(daemon) and use emacsclient *without*
utilizing systemd, including from an X startup script?

Thanks.