run: do not pass the pty slave fd to transient service in a machine

Follow-up for 28459ba

The pty path returned by OpenMachinePTY() cannot be opened from outside
the machine, hence let's use the plain Standard{Input,Output,Error}=tty
in such a case. This means if --machine= is specified, #32916 would occur.
A comprehensive fix requires a new dbus method in machined, which shall
be material for v257.

See also: systemd/systemd#33216 (comment)

Replaces #33216

Co-authored-by: Mike Yuan <[email protected]>
(cherry picked from commit ddef3ec)
(cherry picked from commit 639c922)

Author: 2 people

src/run/run.c

@@ -744,7 +744,7 @@ static int transient_kill_set_properties(sd_bus_message *m) {
         return 0;
 }
 
-static int transient_service_set_properties(sd_bus_message *m, const char *pty_path) {
+static int transient_service_set_properties(sd_bus_message *m, const char *pty_path, int pty_fd) {
         bool send_term = false;
         int r;
 
@@ -754,6 +754,7 @@ static int transient_service_set_properties(sd_bus_message *m, const char *pty_p
         bool use_ex_prop = arg_expand_environment == 0;
 
         assert(m);
+        assert(pty_path || pty_fd < 0);
 
         r = transient_unit_set_properties(m, UNIT_SERVICE, arg_property);
         if (r < 0)
@@ -804,18 +805,22 @@ static int transient_service_set_properties(sd_bus_message *m, const char *pty_p
         }
 
         if (pty_path) {
-                _cleanup_close_ int pty_slave = -EBADF;
-
-                pty_slave = open_terminal(pty_path, O_RDWR|O_NOCTTY|O_CLOEXEC);
-                if (pty_slave < 0)
-                        return pty_slave;
+                r = sd_bus_message_append(m, "(sv)", "TTYPath", "s", pty_path);
+                if (r < 0)
+                        return bus_log_create_error(r);
 
-                r = sd_bus_message_append(m,
-                                          "(sv)(sv)(sv)(sv)",
-                                          "StandardInputFileDescriptor", "h", pty_slave,
-                                          "StandardOutputFileDescriptor", "h", pty_slave,
-                                          "StandardErrorFileDescriptor", "h", pty_slave,
-                                          "TTYPath", "s", pty_path);
+                if (pty_fd >= 0)
+                        r = sd_bus_message_append(m,
+                                                  "(sv)(sv)(sv)",
+                                                  "StandardInputFileDescriptor", "h", pty_fd,
+                                                  "StandardOutputFileDescriptor", "h", pty_fd,
+                                                  "StandardErrorFileDescriptor", "h", pty_fd);
+                else
+                        r = sd_bus_message_append(m,
+                                                  "(sv)(sv)(sv)",
+                                                  "StandardInput", "s", "tty",
+                                                  "StandardOutput", "s", "tty",
+                                                  "StandardError", "s", "tty");
                 if (r < 0)
                         return bus_log_create_error(r);
 
@@ -1166,7 +1171,8 @@ static int make_transient_service_unit(
                 sd_bus *bus,
                 sd_bus_message **message,
                 const char *service,
-                const char *pty_path) {
+                const char *pty_path,
+                int pty_fd) {
 
         _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL;
         int r;
@@ -1193,7 +1199,7 @@ static int make_transient_service_unit(
         if (r < 0)
                 return bus_log_create_error(r);
 
-        r = transient_service_set_properties(m, pty_path);
+        r = transient_service_set_properties(m, pty_path, pty_fd);
         if (r < 0)
                 return r;
 
@@ -1238,7 +1244,7 @@ static int start_transient_service(sd_bus *bus) {
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
         _cleanup_(bus_wait_for_jobs_freep) BusWaitForJobs *w = NULL;
         _cleanup_free_ char *service = NULL, *pty_path = NULL;
-        _cleanup_close_ int master = -EBADF;
+        _cleanup_close_ int master = -EBADF, slave = -EBADF;
         int r;
 
         assert(bus);
@@ -1257,6 +1263,10 @@ static int start_transient_service(sd_bus *bus) {
                         if (unlockpt(master) < 0)
                                 return log_error_errno(errno, "Failed to unlock tty: %m");
 
+                        slave = open_terminal(pty_path, O_RDWR|O_NOCTTY|O_CLOEXEC);
+                        if (slave < 0)
+                                return log_error_errno(slave, "Failed to open pty slave: %m");
+
                 } else if (arg_transport == BUS_TRANSPORT_MACHINE) {
                         _cleanup_(sd_bus_unrefp) sd_bus *system_bus = NULL;
                         _cleanup_(sd_bus_message_unrefp) sd_bus_message *pty_reply = NULL;
@@ -1286,6 +1296,9 @@ static int start_transient_service(sd_bus *bus) {
                         pty_path = strdup(s);
                         if (!pty_path)
                                 return log_oom();
+
+                        // FIXME: Introduce OpenMachinePTYEx() that accepts ownership/permission as param
+                        // and additionally returns the pty fd, for #33216 and #32999
                 } else
                         assert_not_reached();
         }
@@ -1312,9 +1325,10 @@ static int start_transient_service(sd_bus *bus) {
                         return r;
         }
 
-        r = make_transient_service_unit(bus, &m, service, pty_path);
+        r = make_transient_service_unit(bus, &m, service, pty_path, slave);
         if (r < 0)
                 return r;
+        slave = safe_close(slave);
 
         polkit_agent_open_if_enabled(arg_transport, arg_ask_password);
 
@@ -1731,7 +1745,7 @@ static int make_transient_trigger_unit(
                 if (r < 0)
                         return bus_log_create_error(r);
 
-                r = transient_service_set_properties(m, NULL);
+                r = transient_service_set_properties(m, /* pty_path = */ NULL, /* pty_fd = */ -EBADF);
                 if (r < 0)
                         return r;