From 5b1bad8d45319f45e5679d5d47be74e43ee344d3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Dec 2016 22:56:39 +0100 Subject: [PATCH 1/6] run: fix race for "systemd-run --wait" D-Bus is inherently racy when a function returns an object path for a newly allocated object the client shall watch: as the object already exists before the client can subscribe to it, it might lose messages from it. Let's fix this, by explicitly querying unit properties right after subscribing to its property changes. Fixes: #4920 --- src/run/run.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/run/run.c b/src/run/run.c index 99f03465b0..3e3116f235 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -798,7 +798,7 @@ static void run_context_check_done(RunContext *c) { sd_event_exit(c->event, EXIT_SUCCESS); } -static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error *error) { +static int run_context_update(RunContext *c, const char *path) { static const struct bus_properties_map map[] = { { "ActiveState", "s", NULL, offsetof(RunContext, active_state) }, @@ -811,12 +811,11 @@ static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error {} }; - RunContext *c = userdata; int r; r = bus_map_all_properties(c->bus, "org.freedesktop.systemd1", - sd_bus_message_get_path(m), + path, map, c); if (r < 0) { @@ -828,6 +827,15 @@ static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error return 0; } +static int on_properties_changed(sd_bus_message *m, void *userdata, sd_bus_error *error) { + RunContext *c = userdata; + + assert(m); + assert(c); + + return run_context_update(c, sd_bus_message_get_path(m)); +} + static int pty_forward_handler(PTYForward *f, int rcode, void *userdata) { RunContext *c = userdata; @@ -1027,6 +1035,10 @@ static int start_transient_service( r = sd_bus_attach_event(bus, c.event, 0); if (r < 0) return log_error_errno(r, "Failed to attach bus to event loop."); + + r = run_context_update(&c, path); + if (r < 0) + return r; } r = sd_event_loop(c.event); From 9a1c8f2d2454ba5ccac4538137bc112d6fe6454f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Dec 2016 23:25:49 +0100 Subject: [PATCH 2/6] ptyfwd: set event source description strings for all event sources of a ptyfwd object --- src/shared/ptyfwd.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index 293c6673fc..3a02ae98dc 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -438,6 +438,9 @@ int pty_forward_new( r = sd_event_add_io(f->event, &f->stdin_event_source, STDIN_FILENO, EPOLLIN|EPOLLET, on_stdin_event, f); if (r < 0 && r != -EPERM) return r; + + if (r >= 0) + (void) sd_event_source_set_description(f->stdin_event_source, "ptyfwd-stdin"); } r = sd_event_add_io(f->event, &f->stdout_event_source, STDOUT_FILENO, EPOLLOUT|EPOLLET, on_stdout_event, f); @@ -446,15 +449,21 @@ int pty_forward_new( f->stdout_writable = true; else if (r < 0) return r; + else + (void) sd_event_source_set_description(f->stdout_event_source, "ptyfwd-stdout"); r = sd_event_add_io(f->event, &f->master_event_source, master, EPOLLIN|EPOLLOUT|EPOLLET, on_master_event, f); if (r < 0) return r; + (void) sd_event_source_set_description(f->master_event_source, "ptyfwd-master"); + r = sd_event_add_signal(f->event, &f->sigwinch_event_source, SIGWINCH, on_sigwinch_event, f); if (r < 0) return r; + (void) sd_event_source_set_description(f->sigwinch_event_source, "ptyfwd-sigwinch"); + *ret = f; f = NULL; From 8f5c235d9e5f2e80cd2cf55c4585cddcdec5931b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Dec 2016 23:26:15 +0100 Subject: [PATCH 3/6] sd-event: when an event source fails, don't assume the type of it is still set If a callback of an event source returns an error, then the event source might already be half-destroyed, if the callback dropped all refs. Hence, don't assume that the type is still valid, and save it before we issue the callback. --- src/libsystemd/sd-event/sd-event.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index f94959adac..4816bd1f67 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -2226,11 +2226,16 @@ static int process_signal(sd_event *e, struct signal_data *d, uint32_t events) { } static int source_dispatch(sd_event_source *s) { + EventSourceType saved_type; int r = 0; assert(s); assert(s->pending || s->type == SOURCE_EXIT); + /* Save the event source type, here, so that we still know it after the event callback which might invalidate + * the event. */ + saved_type = s->type; + if (s->type != SOURCE_DEFER && s->type != SOURCE_EXIT) { r = source_set_pending(s, false); if (r < 0) @@ -2318,7 +2323,7 @@ static int source_dispatch(sd_event_source *s) { if (r < 0) log_debug_errno(r, "Event source %s (type %s) returned error, disabling: %m", - strna(s->description), event_source_type_to_string(s->type)); + strna(s->description), event_source_type_to_string(saved_type)); if (s->n_ref == 0) source_free(s); From 578c03bce0816b21f00e4ef9734e5c1c1a63dcc6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 24 Dec 2016 00:30:49 +0100 Subject: [PATCH 4/6] run: complain when --pty is used together with --no-block, which makes no sense --- src/run/run.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/run/run.c b/src/run/run.c index 3e3116f235..df3b47af29 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -403,6 +403,11 @@ static int parse_argv(int argc, char *argv[]) { return -EINVAL; } + if (arg_pty && arg_no_block) { + log_error("--pty is not compatible with --no-block."); + return -EINVAL; + } + if (arg_scope && with_timer()) { log_error("Timer options are not supported in --scope mode."); return -EINVAL; From 9182fb52acc5993a86b83c2fe4216a542cecf226 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 24 Dec 2016 00:34:34 +0100 Subject: [PATCH 5/6] run: explicitly close pty forwarder before printing summary If the PTY forwarder is still around our TTY will have borked settings, regarding newlines, hence explicitly close it before showing the summary, so that it looks pretty. --- src/run/run.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/run/run.c b/src/run/run.c index df3b47af29..d3d491e87f 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -1059,6 +1059,12 @@ static int start_transient_service( } if (!arg_quiet) { + + /* Explicitly destroy the PTY forwarder, so that the PTY device is usable again, in its + * original settings (i.e. proper line breaks), so that we can show the summary in a pretty + * way. */ + c.forward = pty_forward_free(c.forward); + if (!isempty(c.result)) log_info("Finished with result: %s", strna(c.result)); From 95f1d6bfecde60b245fae1ab0313b550201e7880 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 24 Dec 2016 00:35:58 +0100 Subject: [PATCH 6/6] run: exit early in --pty if service failed This reworks systemd-run so that in --pty mode we watch the unit state the way we do it in --wait mode. Whenever we notice that the service is in failed or inactive state finish right-away, but first write all unwritten characters we can read from the master TTY device. This makes sure that when the TTY service fails before it opens the slave PTY device we properly notice that and exit early, so that borked start parameters result in immediate systemd-run failure. Previously, we'd not notice this at all, as a PTY slave that never was opened won't result in POLLHUP events, and we'd hence simply keep reading from it forever. In essence, --pty now enables the same unit watching logic that --wait enables. However, unless --wait is specified we won#t show the final summary, hence the effective difference should be pretty minimal. Fixes: #3915 --- src/run/run.c | 54 ++++++++++++++++++++++----------------------- src/shared/ptyfwd.c | 21 ++++++++++++++++++ src/shared/ptyfwd.h | 2 ++ 3 files changed, 50 insertions(+), 27 deletions(-) diff --git a/src/run/run.c b/src/run/run.c index d3d491e87f..08f7e12336 100644 --- a/src/run/run.c +++ b/src/run/run.c @@ -789,15 +789,17 @@ static void run_context_free(RunContext *c) { } static void run_context_check_done(RunContext *c) { - bool done = true; + bool done; assert(c); if (c->match) - done = done && (c->active_state && STR_IN_SET(c->active_state, "inactive", "failed")); + done = STRPTR_IN_SET(c->active_state, "inactive", "failed"); + else + done = true; - if (c->forward) - done = done && pty_forward_is_done(c->forward); + if (c->forward && done) /* If the service is gone, it's time to drain the output */ + done = pty_forward_drain(c->forward); if (done) sd_event_exit(c->event, EXIT_SUCCESS); @@ -998,6 +1000,8 @@ static int start_transient_service( if (arg_wait || master >= 0) { _cleanup_(run_context_free) RunContext c = {}; + _cleanup_free_ char *path = NULL; + const char *mt; c.bus = sd_bus_ref(bus); @@ -1020,31 +1024,27 @@ static int start_transient_service( pty_forward_set_handler(c.forward, pty_forward_handler, &c); } - if (arg_wait) { - _cleanup_free_ char *path = NULL; - const char *mt; - path = unit_dbus_path_from_name(service); - if (!path) - return log_oom(); + path = unit_dbus_path_from_name(service); + if (!path) + return log_oom(); - mt = strjoina("type='signal'," - "sender='org.freedesktop.systemd1'," - "path='", path, "'," - "interface='org.freedesktop.DBus.Properties'," - "member='PropertiesChanged'"); - r = sd_bus_add_match(bus, &c.match, mt, on_properties_changed, &c); - if (r < 0) - return log_error_errno(r, "Failed to add properties changed signal."); + mt = strjoina("type='signal'," + "sender='org.freedesktop.systemd1'," + "path='", path, "'," + "interface='org.freedesktop.DBus.Properties'," + "member='PropertiesChanged'"); + r = sd_bus_add_match(bus, &c.match, mt, on_properties_changed, &c); + if (r < 0) + return log_error_errno(r, "Failed to add properties changed signal."); - r = sd_bus_attach_event(bus, c.event, 0); - if (r < 0) - return log_error_errno(r, "Failed to attach bus to event loop."); + r = sd_bus_attach_event(bus, c.event, 0); + if (r < 0) + return log_error_errno(r, "Failed to attach bus to event loop."); - r = run_context_update(&c, path); - if (r < 0) - return r; - } + r = run_context_update(&c, path); + if (r < 0) + return r; r = sd_event_loop(c.event); if (r < 0) @@ -1058,7 +1058,7 @@ static int start_transient_service( fputc('\n', stdout); } - if (!arg_quiet) { + if (arg_wait && !arg_quiet) { /* Explicitly destroy the PTY forwarder, so that the PTY device is usable again, in its * original settings (i.e. proper line breaks), so that we can show the summary in a pretty @@ -1439,7 +1439,7 @@ int main(int argc, char* argv[]) { /* If --wait is used connect via the bus, unconditionally, as ref/unref is not supported via the limited direct * connection */ - if (arg_wait) + if (arg_wait || arg_pty) r = bus_connect_transport(arg_transport, arg_host, arg_user, &bus); else r = bus_connect_transport_systemd(arg_transport, arg_host, arg_user, &bus); diff --git a/src/shared/ptyfwd.c b/src/shared/ptyfwd.c index 3a02ae98dc..59b541d519 100644 --- a/src/shared/ptyfwd.c +++ b/src/shared/ptyfwd.c @@ -69,6 +69,7 @@ struct PTYForward { bool read_from_master:1; bool done:1; + bool drain:1; bool last_char_set:1; char last_char; @@ -302,6 +303,11 @@ static int shovel(PTYForward *f) { return pty_forward_done(f, 0); } + /* If we were asked to drain, and there's nothing more to handle from the master, then call the callback + * too. */ + if (f->drain && f->out_buffer_full == 0 && !f->master_readable) + return pty_forward_done(f, 0); + return 0; } @@ -528,3 +534,18 @@ void pty_forward_set_handler(PTYForward *f, PTYForwardHandler cb, void *userdata f->handler = cb; f->userdata = userdata; } + +bool pty_forward_drain(PTYForward *f) { + assert(f); + + /* Starts draining the forwarder. Specifically: + * + * - Returns true if there are no unprocessed bytes from the pty, false otherwise + * + * - Makes sure the handler function is called the next time the number of unprocessed bytes hits zero + */ + + f->drain = true; + + return f->out_buffer_full == 0 && !f->master_readable; +} diff --git a/src/shared/ptyfwd.h b/src/shared/ptyfwd.h index bd5d5fec0d..3fad1d3b26 100644 --- a/src/shared/ptyfwd.h +++ b/src/shared/ptyfwd.h @@ -51,4 +51,6 @@ bool pty_forward_is_done(PTYForward *f); void pty_forward_set_handler(PTYForward *f, PTYForwardHandler handler, void *userdata); +bool pty_forward_drain(PTYForward *f); + DEFINE_TRIVIAL_CLEANUP_FUNC(PTYForward*, pty_forward_free);