The SocketAddress 'fd' kind accepts the name of a file descriptor passed
to the monitor with the 'getfd' command. This makes it impossible to use
the 'fd' kind in cases where a monitor is not available. This can apply in
handling command line argv at startup, or simply if internal code wants to
use SocketAddress and pass a numeric FD it has acquired from elsewhere.

Fortunately the 'getfd' command mandated that the FD names must not start
with a leading digit. We can thus safely extend semantics of the
SocketAddress 'fd' kind, to allow a purely numeric name to reference an
file descriptor that QEMU already has open. There will be restrictions on
when each kind can be used.

In codepaths where we are handling a monitor command (ie cur_mon != NULL),
we will only support use of named file descriptors as before. Use of FD
numbers is still not permitted for monitor commands.

In codepaths where we are not handling a monitor command (ie cur_mon ==
NULL), we will not support named file descriptors. Instead we can reference
FD numers explicitly. This allows the app spawning QEMU to intentionally
"leak" a pre-opened socket to QEMU and reference that in a SocketAddress
definition, or for code inside QEMU to pass pre-opened FDs around.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qapi/sockets.json    |   7 ++++
 tests/test-sockets.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++---
 util/qemu-sockets.c  |  16 ++++++--
 3 files changed, 126 insertions(+), 9 deletions(-)

diff --git a/qapi/sockets.json b/qapi/sockets.json
index ac022c6ad0..fc81d8d5e8 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -123,6 +123,13 @@
 #
 # @unix:  Unix domain socket
 #
+# @vsock: VMCI address
+#
+# @fd: decimal is for file descriptor number, otherwise a file descriptor name.
+#      Named file descriptors are permitted in monitor commands, in combination
+#      with the 'getfd' command. Decimal file descriptors are permitted at
+#      startup or other contexts where no monitor context is active.
+#
 # Since: 2.9
 ##
 { 'enum': 'SocketAddressType',
diff --git a/tests/test-sockets.c b/tests/test-sockets.c
index 8a2962fe7b..8307e4774f 100644
--- a/tests/test-sockets.c
+++ b/tests/test-sockets.c
@@ -49,7 +49,7 @@ Monitor *cur_mon;
 void monitor_init(Chardev *chr, int flags) {}
 
 
-static void test_socket_fd_pass_good(void)
+static void test_socket_fd_pass_name_good(void)
 {
     SocketAddress addr;
     int fd;
@@ -80,7 +80,7 @@ static void test_socket_fd_pass_good(void)
     cur_mon = NULL;
 }
 
-static void test_socket_fd_pass_bad(void)
+static void test_socket_fd_pass_name_bad(void)
 {
     SocketAddress addr;
     Error *err = NULL;
@@ -110,6 +110,98 @@ static void test_socket_fd_pass_bad(void)
     cur_mon = NULL;
 }
 
+static void test_socket_fd_pass_name_nomon(void)
+{
+    SocketAddress addr;
+    Error *err = NULL;
+    int fd;
+
+    g_assert(cur_mon == NULL);
+
+    addr.type = SOCKET_ADDRESS_TYPE_FD;
+    addr.u.fd.str = g_strdup("myfd");
+
+    fd = socket_connect(&addr, &err);
+    g_assert_cmpint(fd, ==, -1);
+    error_free_or_abort(&err);
+
+    fd = socket_listen(&addr, &err);
+    g_assert_cmpint(fd, ==, -1);
+    error_free_or_abort(&err);
+
+    g_free(addr.u.fd.str);
+}
+
+
+static void test_socket_fd_pass_num_good(void)
+{
+    SocketAddress addr;
+    int fd, sfd;
+
+    g_assert(cur_mon == NULL);
+    sfd = qemu_socket(AF_INET, SOCK_STREAM, 0);
+    g_assert_cmpint(sfd, >, STDERR_FILENO);
+
+    addr.type = SOCKET_ADDRESS_TYPE_FD;
+    addr.u.fd.str = g_strdup_printf("%d", sfd);
+
+    fd = socket_connect(&addr, &error_abort);
+    g_assert_cmpint(fd, ==, sfd);
+
+    fd = socket_listen(&addr, &error_abort);
+    g_assert_cmpint(fd, ==, sfd);
+
+    g_free(addr.u.fd.str);
+    close(sfd);
+}
+
+static void test_socket_fd_pass_num_bad(void)
+{
+    SocketAddress addr;
+    Error *err = NULL;
+    int fd, sfd;
+
+    g_assert(cur_mon == NULL);
+    sfd = dup(STDOUT_FILENO);
+
+    addr.type = SOCKET_ADDRESS_TYPE_FD;
+    addr.u.fd.str = g_strdup_printf("%d", sfd);
+
+    fd = socket_connect(&addr, &err);
+    g_assert_cmpint(fd, ==, -1);
+    error_free_or_abort(&err);
+
+    fd = socket_listen(&addr, &err);
+    g_assert_cmpint(fd, ==, -1);
+    error_free_or_abort(&err);
+
+    g_free(addr.u.fd.str);
+    close(sfd);
+}
+
+static void test_socket_fd_pass_num_nocli(void)
+{
+    SocketAddress addr;
+    Error *err = NULL;
+    int fd;
+
+    cur_mon = g_malloc(1); /* Fake a monitor */
+
+    addr.type = SOCKET_ADDRESS_TYPE_FD;
+    addr.u.fd.str = g_strdup_printf("%d", STDOUT_FILENO);
+
+    fd = socket_connect(&addr, &err);
+    g_assert_cmpint(fd, ==, -1);
+    error_free_or_abort(&err);
+
+    fd = socket_listen(&addr, &err);
+    g_assert_cmpint(fd, ==, -1);
+    error_free_or_abort(&err);
+
+    g_free(addr.u.fd.str);
+}
+
+
 int main(int argc, char **argv)
 {
     bool has_ipv4, has_ipv6;
@@ -129,10 +221,18 @@ int main(int argc, char **argv)
     }
 
     if (has_ipv4) {
-        g_test_add_func("/socket/fd-pass/good",
-                        test_socket_fd_pass_good);
-        g_test_add_func("/socket/fd-pass/bad",
-                        test_socket_fd_pass_bad);
+        g_test_add_func("/socket/fd-pass/name/good",
+                        test_socket_fd_pass_name_good);
+        g_test_add_func("/socket/fd-pass/name/bad",
+                        test_socket_fd_pass_name_bad);
+        g_test_add_func("/socket/fd-pass/name/nomon",
+                        test_socket_fd_pass_name_nomon);
+        g_test_add_func("/socket/fd-pass/num/good",
+                        test_socket_fd_pass_num_good);
+        g_test_add_func("/socket/fd-pass/num/bad",
+                        test_socket_fd_pass_num_bad);
+        g_test_add_func("/socket/fd-pass/num/nocli",
+                        test_socket_fd_pass_num_nocli);
     }
 
     return g_test_run();
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 6a7b194428..8ac4eed122 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1029,9 +1029,19 @@ fail:
 
 static int socket_get_fd(const char *fdstr, Error **errp)
 {
-    int fd = monitor_get_fd(cur_mon, fdstr, errp);
-    if (fd < 0) {
-        return -1;
+    int fd;
+    if (cur_mon) {
+        fd = monitor_get_fd(cur_mon, fdstr, errp);
+        if (fd < 0) {
+            return -1;
+        }
+    } else {
+        if (qemu_strtoi(fdstr, NULL, 10, &fd) < 0) {
+            error_setg_errno(errp, errno,
+                             "Unable to parse FD number %s",
+                             fdstr);
+            return -1;
+        }
     }
     if (!fd_is_socket(fd)) {
         error_setg(errp, "File descriptor '%s' is not a socket", fdstr);
-- 
2.14.3

Hi

On Fri, Dec 22, 2017 at 2:45 PM, Daniel P. Berrange <berrange@redhat.com> wrote:
> The SocketAddress 'fd' kind accepts the name of a file descriptor passed
> to the monitor with the 'getfd' command. This makes it impossible to use
> the 'fd' kind in cases where a monitor is not available. This can apply in
> handling command line argv at startup, or simply if internal code wants to
> use SocketAddress and pass a numeric FD it has acquired from elsewhere.
>
> Fortunately the 'getfd' command mandated that the FD names must not start
> with a leading digit. We can thus safely extend semantics of the
> SocketAddress 'fd' kind, to allow a purely numeric name to reference an
> file descriptor that QEMU already has open. There will be restrictions on
> when each kind can be used.
>
> In codepaths where we are handling a monitor command (ie cur_mon != NULL),
> we will only support use of named file descriptors as before. Use of FD
> numbers is still not permitted for monitor commands.
>
> In codepaths where we are not handling a monitor command (ie cur_mon ==
> NULL), we will not support named file descriptors. Instead we can reference
> FD numers explicitly. This allows the app spawning QEMU to intentionally
> "leak" a pre-opened socket to QEMU and reference that in a SocketAddress
> definition, or for code inside QEMU to pass pre-opened FDs around.

That looks reasonable to me. If in the future we get rid of cur_mon
somehow, the code should be fairly simple to adapt, but I don't think
we need to worry about it now,

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qapi/sockets.json    |   7 ++++
>  tests/test-sockets.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  util/qemu-sockets.c  |  16 ++++++--
>  3 files changed, 126 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index ac022c6ad0..fc81d8d5e8 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -123,6 +123,13 @@
>  #
>  # @unix:  Unix domain socket
>  #
> +# @vsock: VMCI address
> +#
> +# @fd: decimal is for file descriptor number, otherwise a file descriptor name.
> +#      Named file descriptors are permitted in monitor commands, in combination
> +#      with the 'getfd' command. Decimal file descriptors are permitted at
> +#      startup or other contexts where no monitor context is active.
> +#
>  # Since: 2.9
>  ##
>  { 'enum': 'SocketAddressType',
> diff --git a/tests/test-sockets.c b/tests/test-sockets.c
> index 8a2962fe7b..8307e4774f 100644
> --- a/tests/test-sockets.c
> +++ b/tests/test-sockets.c
> @@ -49,7 +49,7 @@ Monitor *cur_mon;
>  void monitor_init(Chardev *chr, int flags) {}
>
>
> -static void test_socket_fd_pass_good(void)
> +static void test_socket_fd_pass_name_good(void)
>  {
>      SocketAddress addr;
>      int fd;
> @@ -80,7 +80,7 @@ static void test_socket_fd_pass_good(void)
>      cur_mon = NULL;
>  }
>
> -static void test_socket_fd_pass_bad(void)
> +static void test_socket_fd_pass_name_bad(void)
>  {
>      SocketAddress addr;
>      Error *err = NULL;
> @@ -110,6 +110,98 @@ static void test_socket_fd_pass_bad(void)
>      cur_mon = NULL;
>  }
>
> +static void test_socket_fd_pass_name_nomon(void)
> +{
> +    SocketAddress addr;
> +    Error *err = NULL;
> +    int fd;
> +
> +    g_assert(cur_mon == NULL);
> +
> +    addr.type = SOCKET_ADDRESS_TYPE_FD;
> +    addr.u.fd.str = g_strdup("myfd");
> +
> +    fd = socket_connect(&addr, &err);
> +    g_assert_cmpint(fd, ==, -1);
> +    error_free_or_abort(&err);
> +
> +    fd = socket_listen(&addr, &err);
> +    g_assert_cmpint(fd, ==, -1);
> +    error_free_or_abort(&err);
> +
> +    g_free(addr.u.fd.str);
> +}
> +
> +
> +static void test_socket_fd_pass_num_good(void)
> +{
> +    SocketAddress addr;
> +    int fd, sfd;
> +
> +    g_assert(cur_mon == NULL);
> +    sfd = qemu_socket(AF_INET, SOCK_STREAM, 0);
> +    g_assert_cmpint(sfd, >, STDERR_FILENO);
> +
> +    addr.type = SOCKET_ADDRESS_TYPE_FD;
> +    addr.u.fd.str = g_strdup_printf("%d", sfd);
> +
> +    fd = socket_connect(&addr, &error_abort);
> +    g_assert_cmpint(fd, ==, sfd);
> +
> +    fd = socket_listen(&addr, &error_abort);
> +    g_assert_cmpint(fd, ==, sfd);
> +
> +    g_free(addr.u.fd.str);
> +    close(sfd);
> +}
> +
> +static void test_socket_fd_pass_num_bad(void)
> +{
> +    SocketAddress addr;
> +    Error *err = NULL;
> +    int fd, sfd;
> +
> +    g_assert(cur_mon == NULL);
> +    sfd = dup(STDOUT_FILENO);
> +
> +    addr.type = SOCKET_ADDRESS_TYPE_FD;
> +    addr.u.fd.str = g_strdup_printf("%d", sfd);
> +
> +    fd = socket_connect(&addr, &err);
> +    g_assert_cmpint(fd, ==, -1);
> +    error_free_or_abort(&err);
> +
> +    fd = socket_listen(&addr, &err);
> +    g_assert_cmpint(fd, ==, -1);
> +    error_free_or_abort(&err);
> +
> +    g_free(addr.u.fd.str);
> +    close(sfd);
> +}
> +
> +static void test_socket_fd_pass_num_nocli(void)
> +{
> +    SocketAddress addr;
> +    Error *err = NULL;
> +    int fd;
> +
> +    cur_mon = g_malloc(1); /* Fake a monitor */
> +
> +    addr.type = SOCKET_ADDRESS_TYPE_FD;
> +    addr.u.fd.str = g_strdup_printf("%d", STDOUT_FILENO);
> +
> +    fd = socket_connect(&addr, &err);
> +    g_assert_cmpint(fd, ==, -1);
> +    error_free_or_abort(&err);
> +
> +    fd = socket_listen(&addr, &err);
> +    g_assert_cmpint(fd, ==, -1);
> +    error_free_or_abort(&err);
> +
> +    g_free(addr.u.fd.str);
> +}
> +
> +
>  int main(int argc, char **argv)
>  {
>      bool has_ipv4, has_ipv6;
> @@ -129,10 +221,18 @@ int main(int argc, char **argv)
>      }
>
>      if (has_ipv4) {
> -        g_test_add_func("/socket/fd-pass/good",
> -                        test_socket_fd_pass_good);
> -        g_test_add_func("/socket/fd-pass/bad",
> -                        test_socket_fd_pass_bad);
> +        g_test_add_func("/socket/fd-pass/name/good",
> +                        test_socket_fd_pass_name_good);
> +        g_test_add_func("/socket/fd-pass/name/bad",
> +                        test_socket_fd_pass_name_bad);
> +        g_test_add_func("/socket/fd-pass/name/nomon",
> +                        test_socket_fd_pass_name_nomon);
> +        g_test_add_func("/socket/fd-pass/num/good",
> +                        test_socket_fd_pass_num_good);
> +        g_test_add_func("/socket/fd-pass/num/bad",
> +                        test_socket_fd_pass_num_bad);
> +        g_test_add_func("/socket/fd-pass/num/nocli",
> +                        test_socket_fd_pass_num_nocli);
>      }
>
>      return g_test_run();
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 6a7b194428..8ac4eed122 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1029,9 +1029,19 @@ fail:
>
>  static int socket_get_fd(const char *fdstr, Error **errp)
>  {
> -    int fd = monitor_get_fd(cur_mon, fdstr, errp);
> -    if (fd < 0) {
> -        return -1;
> +    int fd;
> +    if (cur_mon) {
> +        fd = monitor_get_fd(cur_mon, fdstr, errp);
> +        if (fd < 0) {
> +            return -1;
> +        }
> +    } else {
> +        if (qemu_strtoi(fdstr, NULL, 10, &fd) < 0) {
> +            error_setg_errno(errp, errno,
> +                             "Unable to parse FD number %s",
> +                             fdstr);
> +            return -1;
> +        }
>      }
>      if (!fd_is_socket(fd)) {
>          error_setg(errp, "File descriptor '%s' is not a socket", fdstr);
> --
> 2.14.3
>
>



-- 
Marc-André Lureau