Re: [systemd-devel] [PATCH] core/socket: Add REMOTE_IP environment variable for Accept=true

2015-03-08 Thread Lennart Poettering 
On Fri, 06.03.15 13:10, Shawn Landden (sh...@churchofgit.com) wrote:

 On Thu, Mar 5, 2015 at 3:18 AM, Lennart Poettering lenn...@poettering.net
 wrote:
 
  On Wed, 04.03.15 15:18, Shawn Landden (sh...@churchofgit.com) wrote:
 
  Can't this just use getpeername_pretty()?
 
  Then I can't force it to only ipv4 and ipv6.

It wouldn't be too difficult to explicitly check for that before.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] core/socket: Add REMOTE_IP environment variable for Accept=true

2015-03-06 Thread Shawn Landden 
On Thu, Mar 5, 2015 at 3:18 AM, Lennart Poettering lenn...@poettering.net
wrote:

 On Wed, 04.03.15 15:18, Shawn Landden (sh...@churchofgit.com) wrote:

 Can't this just use getpeername_pretty()?

 Then I can't force it to only ipv4 and ipv6.

 Lennart

 --
 Lennart Poettering, Red Hat
 ___
 systemd-devel mailing list
 systemd-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/systemd-devel




-- 
Shawn Landden
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] core/socket: Add REMOTE_IP environment variable for Accept=true

2015-03-05 Thread Lennart Poettering 
On Thu, 05.03.15 01:27, Zbigniew Jędrzejewski-Szmek (zbys...@in.waw.pl) wrote:

 /varlistentry
   
 varlistentry
  diff --git a/src/core/service.c b/src/core/service.c
  index a89ff3f..0942072 100644
  --- a/src/core/service.c
  +++ b/src/core/service.c
  @@ -1119,6 +1119,30 @@ static int service_spawn(
   goto fail;
   }
   
  +if (s-accept_socket.unit) {
  +union sockaddr_union pn;
  +socklen_t pnlen = sizeof(pn);
  +_cleanup_free_ char *remote_addr = NULL;
  +
  +r = getpeername(s-socket_fd, pn.sa, pnlen);
 This happens before the fork, right? You cannot call a blocking function
 like this in PID 1. This could be called either asynchronously, or
 after the fork, in the child thread. The latter seems easier.

getpeername() just gets the peer sockaddr, it's not slow. It should be
OK to invoke it, and we actually do already, to generate instantiated
service names.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] core/socket: Add REMOTE_IP environment variable for Accept=true

2015-03-05 Thread Lennart Poettering 
On Wed, 04.03.15 15:18, Shawn Landden (sh...@churchofgit.com) wrote:

Can't this just use getpeername_pretty()?

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] core/socket: Add REMOTE_IP environment variable for Accept=true

2015-03-04 Thread Shawn Landden 
Fix handling of abstract unix domain sockets too.

v2
---
 TODO |  2 --
 man/systemd.socket.xml   |  5 -
 src/core/service.c   | 24 
 src/shared/socket-util.c | 25 +++--
 4 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/TODO b/TODO
index ae32388..780084a 100644
--- a/TODO
+++ b/TODO
@@ -164,8 +164,6 @@ Features:
 * as soon as we have kdbus, and sender timestamps, revisit coalescing multiple 
parallel daemon reloads:
   http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
 
-* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
doing per-connection socket activation. use format introduced by xinetd or CGI 
for this
-
 * the install state probably shouldn't get confused by generated units, think 
dbus1/kdbus compat!
 
 * in systemctl list-unit-files: show the install value the presets would 
suggest for a service in a third column
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 3938345..8796d7b 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -357,7 +357,10 @@
 daemons designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation./para
+paraFor IPv4 and IPv6 connections the varnameREMOTE_IP/varname
+environment variable will be set with remote IP and port seperated by a
+colon (for SOCK_RAW the port is the IP protocol)./para/listitem
   /varlistentry
 
   varlistentry
diff --git a/src/core/service.c b/src/core/service.c
index a89ff3f..2ee4e11 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1119,6 +1119,30 @@ static int service_spawn(
 goto fail;
 }
 
+if (s-accept_socket.unit) {
+union sockaddr_union pn;
+socklen_t pnlen = sizeof(pn);
+_cleanup_free_ char *remote_addr = NULL;
+
+r = getpeername(s-socket_fd, pn.sa, pnlen);
+if (r  0) {
+r = -errno;
+goto fail;
+}
+
+if (pn.in.sin_family == AF_INET ||
+pn.in.sin_family == AF_INET6) {
+r = sockaddr_pretty(pn.sa, pnlen, true, remote_addr);
+if (r  0)
+goto fail;
+
+if (asprintf(our_env + n_env++, REMOTE_IP=%s, 
remote_addr)  0) {
+r = -ENOMEM;
+goto fail;
+}
+}
+}
+
 final_env = strv_env_merge(2, UNIT(s)-manager-environment, our_env, 
NULL);
 if (!final_env) {
 r = -ENOMEM;
diff --git a/src/shared/socket-util.c b/src/shared/socket-util.c
index 74d90fa..dbe2bf7 100644
--- a/src/shared/socket-util.c
+++ b/src/shared/socket-util.c
@@ -522,19 +522,32 @@ int sockaddr_pretty(const struct sockaddr *_sa, socklen_t 
salen, bool translate_
 return -ENOMEM;
 
 } else if (sa-un.sun_path[0] == 0) {
+void *i;
 /* abstract */
 
-/* FIXME: We assume we can print the
- * socket path here and that it hasn't
- * more than one NUL byte. That is
- * actually an invalid assumption */
-
+/* see unix(7), abstract is wierd */
+i = memchr(sa-un.sun_path + 1, '\0', 
sizeof(sa-un.sun_path) - 1);
+if (i)
+for (i = (char *)i + 1;
+ (char *)i  
sa-un.sun_path[sizeof(sa-un.sun_path)];
+ i = (char *)i + 1)
+if (*(char *)i != '\0') {
+p = strdup(abstract 
unprintable);
+if (!p)
+return -ENOMEM;
+
+goto end;
+}
+
+/* no non-NUL bytes after second NUL (if any) */
 p = new(char, sizeof(sa-un.sun_path)+1);
 if (!p)
 return -ENOMEM;
 
 p[0] = '@';
 memcpy(p+1, sa-un.sun_path+1, 
sizeof(sa-un.sun_path)-1);
+
+/* make printable if there was no second NUL (i == 
NULL) */
 p[sizeof(sa-un.sun_path)] = 0;
 
 } else {
@@ -549,7 +562,7 @@ int sockaddr_pretty(const struct sockaddr *_sa, socklen_t 
salen, bool translate_

Re: [systemd-devel] [PATCH] core/socket: Add REMOTE_IP environment variable for Accept=true

2015-03-04 Thread Zbigniew Jędrzejewski-Szmek 
On Wed, Mar 04, 2015 at 03:15:02PM -0800, Shawn Landden wrote:
 Fix handling of abstract unix domain sockets too.
Please split it into two patches.

 ---
  TODO |  2 --
  man/systemd.socket.xml   |  5 -
  src/core/service.c   | 24 
  src/shared/socket-util.c | 25 +++--
  4 files changed, 47 insertions(+), 9 deletions(-)
 
 diff --git a/TODO b/TODO
 index ae32388..780084a 100644
 --- a/TODO
 +++ b/TODO
 @@ -164,8 +164,6 @@ Features:
  * as soon as we have kdbus, and sender timestamps, revisit coalescing 
 multiple parallel daemon reloads:

 http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
  
 -* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
 doing per-connection socket activation. use format introduced by xinetd or 
 CGI for this
 -
  * the install state probably shouldn't get confused by generated units, 
 think dbus1/kdbus compat!
  
  * in systemctl list-unit-files: show the install value the presets would 
 suggest for a service in a third column
 diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
 index 3938345..8796d7b 100644
 --- a/man/systemd.socket.xml
 +++ b/man/systemd.socket.xml
 @@ -357,7 +357,10 @@
  daemons designed for usage with
  
 citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
  to work unmodified with systemd socket
 -activation./para/listitem
 +activation./para
 +paraFor IPv4 and IPv6 connections the varnameREMOTE_IP/varname
 +environment variable will be set with remote IP and port seperated 
 by a
 +colon (for SOCK_RAW the port is the IP protocol)./para/listitem
Would be nice to have a note here that say that this is the same as xinetd etc.

/varlistentry
  
varlistentry
 diff --git a/src/core/service.c b/src/core/service.c
 index a89ff3f..0942072 100644
 --- a/src/core/service.c
 +++ b/src/core/service.c
 @@ -1119,6 +1119,30 @@ static int service_spawn(
  goto fail;
  }
  
 +if (s-accept_socket.unit) {
 +union sockaddr_union pn;
 +socklen_t pnlen = sizeof(pn);
 +_cleanup_free_ char *remote_addr = NULL;
 +
 +r = getpeername(s-socket_fd, pn.sa, pnlen);
This happens before the fork, right? You cannot call a blocking function
like this in PID 1. This could be called either asynchronously, or
after the fork, in the child thread. The latter seems easier.

 +if (r  0) {
 +r = -errno;
 +goto fail;
 +}
 +
 +if (pn.sa.sun_family == AF_INET ||
 +pn.sa.sun_family == AF_INET6) {
 +r = sockaddr_pretty(pn.sa, pnlen, true, 
 remote_addr);
 +if (r  0)
 +goto fail;
 +
 +if (asprintf(our_env + n_env++, REMOTE_IP=%s, 
 remote_addr)  0) {
 +r = -ENOMEM;
 +goto fail;
 +}
 +}
 +}
 +
  final_env = strv_env_merge(2, UNIT(s)-manager-environment, 
 our_env, NULL);
  if (!final_env) {
  r = -ENOMEM;
 diff --git a/src/shared/socket-util.c b/src/shared/socket-util.c
 index 74d90fa..dbe2bf7 100644
 --- a/src/shared/socket-util.c
 +++ b/src/shared/socket-util.c
 @@ -522,19 +522,32 @@ int sockaddr_pretty(const struct sockaddr *_sa, 
 socklen_t salen, bool translate_
  return -ENOMEM;
  
  } else if (sa-un.sun_path[0] == 0) {
 +void *i;
  /* abstract */
  
 -/* FIXME: We assume we can print the
 - * socket path here and that it hasn't
 - * more than one NUL byte. That is
 - * actually an invalid assumption */
 -
 +/* see unix(7), abstract is wierd */
 +i = memchr(sa-un.sun_path + 1, '\0', 
 sizeof(sa-un.sun_path) - 1);

Wouldn't it be better to cescape the string? We could say that
socket paths are cunescaped when reading, and cescaped when pretty
printing. Nobody sane uses sockets with zeros in the name, except
by istake, so it wouldn't really matter for normal use, but it would
help catch errors. Seems more useful than punting like this.

 +if (i)
 +for (i = (char *)i + 1;
 + (char *)i  
 sa-un.sun_path[sizeof(sa-un.sun_path)];
 + i = (char *)i + 1)
 +if (*(char *)i != '\0') {
 +p = strdup(abstract 
 unprintable);
 +

[systemd-devel] [PATCH] core/socket: Add REMOTE_IP environment variable for Accept=true

2015-03-04 Thread Shawn Landden 
Fix handling of abstract unix domain sockets too.
---
 TODO |  2 --
 man/systemd.socket.xml   |  5 -
 src/core/service.c   | 24 
 src/shared/socket-util.c | 25 +++--
 4 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/TODO b/TODO
index ae32388..780084a 100644
--- a/TODO
+++ b/TODO
@@ -164,8 +164,6 @@ Features:
 * as soon as we have kdbus, and sender timestamps, revisit coalescing multiple 
parallel daemon reloads:
   http://lists.freedesktop.org/archives/systemd-devel/2014-December/025862.html
 
-* set $REMOTE_IP (or $REMOTE_ADDR/$REMOTE_PORT) environment variable when 
doing per-connection socket activation. use format introduced by xinetd or CGI 
for this
-
 * the install state probably shouldn't get confused by generated units, think 
dbus1/kdbus compat!
 
 * in systemctl list-unit-files: show the install value the presets would 
suggest for a service in a third column
diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml
index 3938345..8796d7b 100644
--- a/man/systemd.socket.xml
+++ b/man/systemd.socket.xml
@@ -357,7 +357,10 @@
 daemons designed for usage with
 
citerefentryrefentrytitleinetd/refentrytitlemanvolnum8/manvolnum/citerefentry
 to work unmodified with systemd socket
-activation./para/listitem
+activation./para
+paraFor IPv4 and IPv6 connections the varnameREMOTE_IP/varname
+environment variable will be set with remote IP and port seperated by a
+colon (for SOCK_RAW the port is the IP protocol)./para/listitem
   /varlistentry
 
   varlistentry
diff --git a/src/core/service.c b/src/core/service.c
index a89ff3f..0942072 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -1119,6 +1119,30 @@ static int service_spawn(
 goto fail;
 }
 
+if (s-accept_socket.unit) {
+union sockaddr_union pn;
+socklen_t pnlen = sizeof(pn);
+_cleanup_free_ char *remote_addr = NULL;
+
+r = getpeername(s-socket_fd, pn.sa, pnlen);
+if (r  0) {
+r = -errno;
+goto fail;
+}
+
+if (pn.sa.sun_family == AF_INET ||
+pn.sa.sun_family == AF_INET6) {
+r = sockaddr_pretty(pn.sa, pnlen, true, remote_addr);
+if (r  0)
+goto fail;
+
+if (asprintf(our_env + n_env++, REMOTE_IP=%s, 
remote_addr)  0) {
+r = -ENOMEM;
+goto fail;
+}
+}
+}
+
 final_env = strv_env_merge(2, UNIT(s)-manager-environment, our_env, 
NULL);
 if (!final_env) {
 r = -ENOMEM;
diff --git a/src/shared/socket-util.c b/src/shared/socket-util.c
index 74d90fa..dbe2bf7 100644
--- a/src/shared/socket-util.c
+++ b/src/shared/socket-util.c
@@ -522,19 +522,32 @@ int sockaddr_pretty(const struct sockaddr *_sa, socklen_t 
salen, bool translate_
 return -ENOMEM;
 
 } else if (sa-un.sun_path[0] == 0) {
+void *i;
 /* abstract */
 
-/* FIXME: We assume we can print the
- * socket path here and that it hasn't
- * more than one NUL byte. That is
- * actually an invalid assumption */
-
+/* see unix(7), abstract is wierd */
+i = memchr(sa-un.sun_path + 1, '\0', 
sizeof(sa-un.sun_path) - 1);
+if (i)
+for (i = (char *)i + 1;
+ (char *)i  
sa-un.sun_path[sizeof(sa-un.sun_path)];
+ i = (char *)i + 1)
+if (*(char *)i != '\0') {
+p = strdup(abstract 
unprintable);
+if (!p)
+return -ENOMEM;
+
+goto end;
+}
+
+/* no non-NUL bytes after second NUL (if any) */
 p = new(char, sizeof(sa-un.sun_path)+1);
 if (!p)
 return -ENOMEM;
 
 p[0] = '@';
 memcpy(p+1, sa-un.sun_path+1, 
sizeof(sa-un.sun_path)-1);
+
+/* make printable if there was no second NUL (i == 
NULL) */
 p[sizeof(sa-un.sun_path)] = 0;
 
 } else {
@@ -549,7 +562,7 @@ int sockaddr_pretty(const struct sockaddr *_sa, socklen_t 
salen, bool translate_