From 972865bc4ef67557f04fba6b7a92edb70c9d3927 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Sep 2015 16:03:41 +0200 Subject: [PATCH 1/7] logging: coerce negative error values to positive errno Especially systemd, which makes use of the error argument for logging, likes to represent errors as negative numbers. We hence must invert a negative error code to get the real errno. (cherry picked from commit d6370d09e6aa158d4fc7fe1e2ce4c335eed1e017) --- src/nm-logging.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/nm-logging.c b/src/nm-logging.c index fa770344ed..b8433a25dd 100644 --- a/src/nm-logging.c +++ b/src/nm-logging.c @@ -381,8 +381,11 @@ _nm_log_impl (const char *file, return; /* Make sure that %m maps to the specified error */ - if (error != 0) + if (error != 0) { + if (error < 0) + error = -error; errno = error; + } va_start (args, fmt); msg = g_strdup_vprintf (fmt, args); From 436ed50f4ab5ff152fe946f9ffdf3f2f822d0469 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 22 Sep 2015 12:34:37 +0200 Subject: [PATCH 2/7] systemd/adapt: refactor creation of struct sd_event_source (cherry picked from commit 41917a52c085b8699e5b37ef9b574ee265dcd0bf) --- src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c index a6d817278e..7984dcba59 100644 --- a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c +++ b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c @@ -37,6 +37,16 @@ struct sd_event_source { sd_event_time_handler_t time_cb; }; +static struct sd_event_source * +source_new (void) +{ + struct sd_event_source *source; + + source = g_new0 (struct sd_event_source, 1); + source->refcount = 1; + return source; +} + int sd_event_source_set_priority (sd_event_source *s, int64_t priority) { @@ -113,8 +123,7 @@ sd_event_add_io (sd_event *e, sd_event_source **s, int fd, uint32_t events, sd_e if (!channel) return -EINVAL; - source = g_new0 (struct sd_event_source, 1); - source->refcount = 1; + source = source_new (); source->io_cb = callback; source->user_data = userdata; source->channel = channel; @@ -158,8 +167,7 @@ sd_event_add_time(sd_event *e, sd_event_source **s, clockid_t clock, uint64_t us struct sd_event_source *source; uint64_t n = now (clock); - source = g_new0 (struct sd_event_source, 1); - source->refcount = 1; + source = source_new (); source->time_cb = callback; source->user_data = userdata; source->usec = usec; From 8c08014f475c4a60e129d4cd755ff30db5eac4bc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 22 Sep 2015 12:35:42 +0200 Subject: [PATCH 3/7] systemd/adapt: use slice-allocator for struct sd_event_source (cherry picked from commit fb0e87be394c0fdb4ccf2911bd089bbdf0b19113) --- src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c index 7984dcba59..1f6038e9c9 100644 --- a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c +++ b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c @@ -42,7 +42,7 @@ source_new (void) { struct sd_event_source *source; - source = g_new0 (struct sd_event_source, 1); + source = g_slice_new0 (struct sd_event_source); source->refcount = 1; return source; } @@ -72,7 +72,7 @@ sd_event_source_unref (sd_event_source *s) */ g_io_channel_unref (s->channel); } - g_free (s); + g_slice_free (struct sd_event_source, s); } return NULL; } From 55e46923c073fc5a38fcf572da05d55c0fc0b71f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 22 Sep 2015 12:44:14 +0200 Subject: [PATCH 4/7] systemd/adapt: fix potential crash invoking sd_event_source callbacks It is common that the callbacks unref the event source. Hence we must ensure that the @source stays alive until the callback returns. (cherry picked from commit 9901047ae3464999ab5c52ac408f603c73239597) --- src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c | 32 ++++++++++++++------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c index 1f6038e9c9..67ddc8da1b 100644 --- a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c +++ b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c @@ -91,6 +91,7 @@ static gboolean io_ready (GIOChannel *channel, GIOCondition condition, struct sd_event_source *source) { int r, revents = 0; + gboolean result; if (condition & G_IO_IN) revents |= EPOLLIN; @@ -103,13 +104,18 @@ io_ready (GIOChannel *channel, GIOCondition condition, struct sd_event_source *s if (condition & G_IO_HUP) revents |= EPOLLHUP; - r = source->io_cb (source, g_io_channel_unix_get_fd (channel), revents, source->user_data); - if (r < 0) { - source->id = 0; - return G_SOURCE_REMOVE; - } + source->refcount++; - return G_SOURCE_CONTINUE; + r = source->io_cb (source, g_io_channel_unix_get_fd (channel), revents, source->user_data); + if (r < 0 || source->refcount <= 1) { + source->id = 0; + result = G_SOURCE_REMOVE; + } else + result = G_SOURCE_CONTINUE; + + sd_event_source_unref (source); + + return result; } int @@ -151,14 +157,20 @@ static gboolean time_ready (struct sd_event_source *source) { int r; + gboolean result; + + source->refcount++; r = source->time_cb (source, source->usec, source->user_data); - if (r < 0) { + if (r < 0 || source->refcount <= 1) { source->id = 0; - return G_SOURCE_REMOVE; - } + result = G_SOURCE_REMOVE; + } else + result = G_SOURCE_CONTINUE; - return G_SOURCE_CONTINUE; + sd_event_source_unref (source); + + return result; } int From 11237a0a66df5e2486d961050aa03103578c40d2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 22 Sep 2015 13:26:58 +0200 Subject: [PATCH 5/7] systemd/adapt: assert that a @source argument is passed to sd_event_add_time() Systemd supports omitting the output source argument. In this case, the created event source is floating and the reference count is handled properly. We have however no callers that make use of that functionality, so instead of implementing floating references, assert that we don't need it. This isn't a change in behavior, because previously the could would just SEGFAULT if a caller didn't want to take ownership of the created event. (cherry picked from commit 02c51d4231da154b624fe5859a1e34f340eecb05) --- src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c index 67ddc8da1b..1f8f2dcc3e 100644 --- a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c +++ b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c @@ -125,6 +125,10 @@ sd_event_add_io (sd_event *e, sd_event_source **s, int fd, uint32_t events, sd_e GIOChannel *channel; GIOCondition condition = 0; + /* systemd supports floating sd_event_source by omitting the @s argument. + * We don't have such users and don't implement floating references. */ + g_return_val_if_fail (s, -EINVAL); + channel = g_io_channel_unix_new (fd); if (!channel) return -EINVAL; @@ -179,6 +183,10 @@ sd_event_add_time(sd_event *e, sd_event_source **s, clockid_t clock, uint64_t us struct sd_event_source *source; uint64_t n = now (clock); + /* systemd supports floating sd_event_source by omitting the @s argument. + * We don't have such users and don't implement floating references. */ + g_return_val_if_fail (s, -EINVAL); + source = source_new (); source->time_cb = callback; source->user_data = userdata; From 0e6a13bccc6503657781f91960b82212af7317f8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 23 Sep 2015 09:48:00 +0200 Subject: [PATCH 6/7] systemd/adapt: refactor sd_event_source to use a union for holding mutually exclusive fields sd_event_source is either used for sd_event_add_io() or sd_event_add_time(). Depending on the use, different fields of the struct are relevant. Refactor the struct to have a union. This reduces the size of the struct, but more importantly, it makes it clear which fields are used in which context. (cherry picked from commit 2d2d742cf1787a948a20bf7a12acd17fa62e1dc9) --- src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c | 22 +++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c index 1f8f2dcc3e..24b77c528b 100644 --- a/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c +++ b/src/dhcp-manager/systemd-dhcp/nm-sd-adapt.c @@ -31,10 +31,16 @@ struct sd_event_source { gpointer user_data; GIOChannel *channel; - sd_event_io_handler_t io_cb; - uint64_t usec; - sd_event_time_handler_t time_cb; + union { + struct { + sd_event_io_handler_t cb; + } io; + struct { + sd_event_time_handler_t cb; + uint64_t usec; + } time; + }; }; static struct sd_event_source * @@ -106,7 +112,7 @@ io_ready (GIOChannel *channel, GIOCondition condition, struct sd_event_source *s source->refcount++; - r = source->io_cb (source, g_io_channel_unix_get_fd (channel), revents, source->user_data); + r = source->io.cb (source, g_io_channel_unix_get_fd (channel), revents, source->user_data); if (r < 0 || source->refcount <= 1) { source->id = 0; result = G_SOURCE_REMOVE; @@ -134,7 +140,7 @@ sd_event_add_io (sd_event *e, sd_event_source **s, int fd, uint32_t events, sd_e return -EINVAL; source = source_new (); - source->io_cb = callback; + source->io.cb = callback; source->user_data = userdata; source->channel = channel; @@ -165,7 +171,7 @@ time_ready (struct sd_event_source *source) source->refcount++; - r = source->time_cb (source, source->usec, source->user_data); + r = source->time.cb (source, source->time.usec, source->user_data); if (r < 0 || source->refcount <= 1) { source->id = 0; result = G_SOURCE_REMOVE; @@ -188,9 +194,9 @@ sd_event_add_time(sd_event *e, sd_event_source **s, clockid_t clock, uint64_t us g_return_val_if_fail (s, -EINVAL); source = source_new (); - source->time_cb = callback; + source->time.cb = callback; source->user_data = userdata; - source->usec = usec; + source->time.usec = usec; if (usec > 1000) usec = n < usec - 1000 ? usec - n : 1000; From fe70a87f942fa4a5d509787852185a7168739de6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 22 Sep 2015 14:03:32 +0200 Subject: [PATCH 7/7] systemd: avoid potential crash due to uncanceled timers in client_receive_advertise() Got a crash with unknown reason on nm-1-0 branch. It's unclear why, but the reason could be that a lease in client_receive_advertise() was cleared, but not its timers. Backtrace from nm-1-0 branch (note that the systemd code where the crash happend is different, but similar): #0 sd_event_source_unref (s=0xf5c007e8fb894853) at dhcp-manager/systemd-dhcp/nm-sd-adapt.c:53 #1 0x0000555555682340 in client_timeout_t1 (s=, usec=, userdata=0x5555559f5240) at dhcp-manager/systemd-dhcp/src/libsystemd-network/sd-dhcp6-client.c:451 #2 0x00005555556a078f in time_ready (source=0x5555559f3c20) at dhcp-manager/systemd-dhcp/nm-sd-adapt.c:146 #3 0x00007ffff4a481b3 in g_timeout_dispatch () from /lib64/libglib-2.0.so.0 #4 0x00007ffff4a4779a in g_main_context_dispatch () from /lib64/libglib-2.0.so.0 #5 0x00007ffff4a47ae8 in g_main_context_iterate.isra.24 () from /lib64/libglib-2.0.so.0 #6 0x00007ffff4a47dba in g_main_loop_run () from /lib64/libglib-2.0.so.0 #7 0x0000555555597073 in main (argc=1, argv=0x7fffffffe2b8) at main.c:512 Equivalent upstream systemd patch: https://github.com/systemd/systemd/pull/1332 https://github.com/systemd/systemd/commit/f89087272b5561c9a3fc9d6a4e2a09f75f688fa7 https://bugzilla.redhat.com/show_bug.cgi?id=1260727 (cherry picked from commit 401a2eb834bdddd8931161eb86204eb80d26b2c7) --- .../systemd-dhcp/src/libsystemd-network/sd-dhcp6-client.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/dhcp-manager/systemd-dhcp/src/libsystemd-network/sd-dhcp6-client.c b/src/dhcp-manager/systemd-dhcp/src/libsystemd-network/sd-dhcp6-client.c index 6c5a6ab536..8a17a72803 100644 --- a/src/dhcp-manager/systemd-dhcp/src/libsystemd-network/sd-dhcp6-client.c +++ b/src/dhcp-manager/systemd-dhcp/src/libsystemd-network/sd-dhcp6-client.c @@ -828,7 +828,10 @@ static int client_receive_advertise(sd_dhcp6_client *client, r = dhcp6_lease_get_preference(client->lease, &pref_lease); if (!client->lease || r < 0 || pref_advertise > pref_lease) { - sd_dhcp6_lease_unref(client->lease); + if (client->lease) { + dhcp6_lease_clear_timers(&client->lease->ia); + sd_dhcp6_lease_unref(client->lease); + } client->lease = lease; lease = NULL; r = 0;