From cfc4529b01a8445e3a1d5383d390150427a8b0bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 10:06:55 +0000 Subject: [PATCH 01/22] networkd: unbork rule serialization We'd write something like RULE=from=250.250.240.0/24 to=(null)/0 tos=0 fwmark=0/0 table=12 RULE=from=250.250.250.0/24 to=(null)/0 tos=0 fwmark=0/0 table=11 --- src/network/networkd-manager.c | 35 +++++++++++++++++----- src/network/networkd-routing-policy-rule.c | 3 -- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 93a2571ec2..9ffbe39958 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -1005,7 +1005,6 @@ static void print_string_set(FILE *f, const char *field, OrderedSet *s) { static int manager_save(Manager *m) { _cleanup_ordered_set_free_free_ OrderedSet *dns = NULL, *ntp = NULL, *search_domains = NULL, *route_domains = NULL; RoutingPolicyRule *rule = NULL; - bool space = false; Link *link; Iterator i; _cleanup_free_ char *temp_path = NULL; @@ -1132,28 +1131,48 @@ static int manager_save(Manager *m) { SET_FOREACH(rule, m->rules, i) { _cleanup_free_ char *from_str = NULL, *to_str = NULL; + bool space = false; + fputs("RULE=", f); if (!in_addr_is_null(rule->family, &rule->from)) { r = in_addr_to_string(rule->family, &rule->from, &from_str); if (r < 0) goto fail; + + fprintf(f, "from=%s/%hhu", + from_str, rule->from_prefixlen); + space = true; } if (!in_addr_is_null(rule->family, &rule->to)) { r = in_addr_to_string(rule->family, &rule->to, &to_str); if (r < 0) goto fail; + + fprintf(f, "%sto=%s/%hhu", + space ? " " : "", + to_str, rule->to_prefixlen); + space = true; } - fprintf(f, "from=%s%s/%hhu to=%s%s/%hhu tos=%hhu fwmark=%"PRIu32"/%"PRIu32" table=%"PRIu32, - space ? " " : "", from_str, rule->from_prefixlen, - space ? " " : "", to_str, rule->to_prefixlen, - rule->tos, - rule->fwmark, rule->fwmask, - rule->table); + if (rule->tos != 0) { + fprintf(f, "%stos=%hhu", + space ? " " : "", + rule->tos); + space = true; + } - fputc('\n', f); + if (rule->fwmark != 0) { + fprintf(f, "%sfwmark=%"PRIu32"/%"PRIu32, + space ? " " : "", + rule->fwmark, rule->fwmask); + space = true; + } + + fprintf(f, "%stable=%"PRIu32 "\n", + space ? " " : "", + rule->table); } r = fflush_and_check(f); diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index 221ab4cb85..775b85e338 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -885,9 +885,6 @@ int routing_policy_rule_load(Manager *m) { if (!p) continue; - p = strchr(*i, '='); - p++; - r = routing_policy_rule_new(&rule); if (r < 0) return r; From 458d8ae3029647d85002eff6b3ef8d6bd80f2eef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 10:22:51 +0000 Subject: [PATCH 02/22] networkd: move rule loading to a separate function No functional change. --- src/network/networkd-manager.c | 51 ++-------------- src/network/networkd-routing-policy-rule.c | 69 ++++++++++++++++++++-- src/network/networkd-routing-policy-rule.h | 3 +- 3 files changed, 69 insertions(+), 54 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 9ffbe39958..852d690ef5 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -1004,7 +1004,6 @@ static void print_string_set(FILE *f, const char *field, OrderedSet *s) { static int manager_save(Manager *m) { _cleanup_ordered_set_free_free_ OrderedSet *dns = NULL, *ntp = NULL, *search_domains = NULL, *route_domains = NULL; - RoutingPolicyRule *rule = NULL; Link *link; Iterator i; _cleanup_free_ char *temp_path = NULL; @@ -1129,51 +1128,9 @@ static int manager_save(Manager *m) { print_string_set(f, "DOMAINS=", search_domains); print_string_set(f, "ROUTE_DOMAINS=", route_domains); - SET_FOREACH(rule, m->rules, i) { - _cleanup_free_ char *from_str = NULL, *to_str = NULL; - bool space = false; - - fputs("RULE=", f); - - if (!in_addr_is_null(rule->family, &rule->from)) { - r = in_addr_to_string(rule->family, &rule->from, &from_str); - if (r < 0) - goto fail; - - fprintf(f, "from=%s/%hhu", - from_str, rule->from_prefixlen); - space = true; - } - - if (!in_addr_is_null(rule->family, &rule->to)) { - r = in_addr_to_string(rule->family, &rule->to, &to_str); - if (r < 0) - goto fail; - - fprintf(f, "%sto=%s/%hhu", - space ? " " : "", - to_str, rule->to_prefixlen); - space = true; - } - - if (rule->tos != 0) { - fprintf(f, "%stos=%hhu", - space ? " " : "", - rule->tos); - space = true; - } - - if (rule->fwmark != 0) { - fprintf(f, "%sfwmark=%"PRIu32"/%"PRIu32, - space ? " " : "", - rule->fwmark, rule->fwmask); - space = true; - } - - fprintf(f, "%stable=%"PRIu32 "\n", - space ? " " : "", - rule->table); - } + r = routing_policy_serialize_rules(m->rules, f); + if (r < 0) + goto fail; r = fflush_and_check(f); if (r < 0) @@ -1260,7 +1217,7 @@ int manager_new(Manager **ret, sd_event *event) { m->duid.type = DUID_TYPE_EN; - (void) routing_policy_rule_load(m); + (void) routing_policy_load_rules(m->state_file, &m->rules_saved); *ret = m; m = NULL; diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index 775b85e338..c8eba2298d 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -836,7 +836,7 @@ int config_parse_routing_policy_rule_device( return 0; } -static int routing_policy_rule_read_full_file(char *state_file, char **ret) { +static int routing_policy_rule_read_full_file(const char *state_file, char **ret) { _cleanup_free_ char *s = NULL; size_t size; int r; @@ -857,16 +857,73 @@ static int routing_policy_rule_read_full_file(char *state_file, char **ret) { return size; } -int routing_policy_rule_load(Manager *m) { +int routing_policy_serialize_rules(Set *rules, FILE *f) { + RoutingPolicyRule *rule = NULL; + Iterator i; + int r; + + assert(f); + + SET_FOREACH(rule, rules, i) { + _cleanup_free_ char *from_str = NULL, *to_str = NULL; + bool space = false; + + fputs("RULE=", f); + + if (!in_addr_is_null(rule->family, &rule->from)) { + r = in_addr_to_string(rule->family, &rule->from, &from_str); + if (r < 0) + return r; + + fprintf(f, "from=%s/%hhu", + from_str, rule->from_prefixlen); + space = true; + } + + if (!in_addr_is_null(rule->family, &rule->to)) { + r = in_addr_to_string(rule->family, &rule->to, &to_str); + if (r < 0) + return r; + + fprintf(f, "%sto=%s/%hhu", + space ? " " : "", + to_str, rule->to_prefixlen); + space = true; + } + + if (rule->tos != 0) { + fprintf(f, "%stos=%hhu", + space ? " " : "", + rule->tos); + space = true; + } + + if (rule->fwmark != 0) { + fprintf(f, "%sfwmark=%"PRIu32"/%"PRIu32, + space ? " " : "", + rule->fwmark, rule->fwmask); + space = true; + } + + fprintf(f, "%stable=%"PRIu32 "\n", + space ? " " : "", + rule->table); + } + + return 0; +} + +int routing_policy_load_rules(const char *state_file, Set **rules) { _cleanup_strv_free_ char **l = NULL; _cleanup_free_ char *data = NULL; const char *p; char **i; int r; - assert(m); + assert(state_file); + assert(rules); - r = routing_policy_rule_read_full_file(m->state_file, &data); + r = routing_policy_rule_read_full_file(state_file, &data); if (r <= 0) return r; @@ -874,7 +931,7 @@ int routing_policy_rule_load(Manager *m) { if (!l) return -ENOMEM; - r = set_ensure_allocated(&m->rules_saved, &routing_policy_rule_hash_ops); + r = set_ensure_allocated(rules, &routing_policy_rule_hash_ops); if (r < 0) return r; @@ -957,7 +1014,7 @@ int routing_policy_rule_load(Manager *m) { } } - r = set_put(m->rules_saved, rule); + r = set_put(*rules, rule); if (r < 0) { log_warning_errno(r, "Failed to add RPDB rule to saved DB, ignoring: %s", p); continue; diff --git a/src/network/networkd-routing-policy-rule.h b/src/network/networkd-routing-policy-rule.h index 01a7107366..70a861723b 100644 --- a/src/network/networkd-routing-policy-rule.h +++ b/src/network/networkd-routing-policy-rule.h @@ -77,7 +77,8 @@ int routing_policy_rule_add_foreign(Manager *m, int family, const union in_addr_ int routing_policy_rule_get(Manager *m, int family, const union in_addr_union *from, uint8_t from_prefixlen, const union in_addr_union *to, uint8_t to_prefixlen, uint8_t tos, uint32_t fwmark, uint32_t table, char *iif, char *oif, RoutingPolicyRule **ret); int routing_policy_rule_make_local(Manager *m, RoutingPolicyRule *rule); -int routing_policy_rule_load(Manager *m); +int routing_policy_serialize_rules(Set *rules, FILE *f); +int routing_policy_load_rules(const char *state_file, Set **rules); void routing_policy_rule_purge(Manager *m, Link *link); int config_parse_routing_policy_rule_tos(const char *unit, const char *filename, unsigned line, const char *section, unsigned section_line, const char *lvalue, int ltype, const char *rvalue, void *data,void *userdata); From e4aca57df851af6c1bd132dcd7a5d8dd112fe3dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 11:21:19 +0000 Subject: [PATCH 03/22] networkd: parse the rvalue, not the option name :( --- src/network/networkd-routing-policy-rule.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index c8eba2298d..09c1015f2c 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -996,7 +996,7 @@ int routing_policy_load_rules(const char *state_file, Set **rules) { } } else if (streq(a, "fwmark")) { - r = parse_fwmark_fwmask(a, &rule->fwmark, &rule->fwmask); + r = parse_fwmark_fwmask(b, &rule->fwmark, &rule->fwmask); if (r < 0) { log_error_errno(r, "Failed to parse RPDB rule firewall mark or mask, ignoring: %s", a); continue; From 6f349a4f9c4f78a6418bff93fe676d9195dc7aa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 11:23:31 +0000 Subject: [PATCH 04/22] test-network: use automatic cleanup --- src/network/test-network.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/network/test-network.c b/src/network/test-network.c index ff0f272ef6..a6b273a290 100644 --- a/src/network/test-network.c +++ b/src/network/test-network.c @@ -22,6 +22,7 @@ #include "dhcp-lease-internal.h" #include "network-internal.h" #include "networkd-manager.h" +#include "udev-util.h" static void test_deserialize_in_addr(void) { _cleanup_free_ struct in_addr *addresses = NULL; @@ -187,25 +188,22 @@ static void test_address_equality(void) { int main(void) { _cleanup_manager_free_ Manager *manager = NULL; - sd_event *event; - struct udev *udev; - struct udev_device *loopback; + _cleanup_(sd_event_unrefp) sd_event *event = NULL; + _cleanup_udev_unref_ struct udev *udev = NULL; + _cleanup_udev_device_unref_ struct udev_device *loopback = NULL; int r; test_deserialize_in_addr(); test_deserialize_dhcp_routes(); test_address_equality(); - r = sd_event_default(&event); - assert_se(r >= 0); + assert_se(sd_event_default(&event) >= 0); assert_se(manager_new(&manager, event) >= 0); r = test_load_config(manager); - if (r == -EPERM) { - sd_event_unref(event); + if (r == -EPERM) return EXIT_TEST_SKIP; - } udev = udev_new(); assert_se(udev); @@ -217,8 +215,4 @@ int main(void) { test_network_get(manager, loopback); assert_se(manager_rtnl_enumerate_links(manager) >= 0); - - udev_device_unref(loopback); - udev_unref(udev); - sd_event_unref(event); } From 7379f3de7f5863f18c768176504203cb58c7e6ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 11:38:24 +0000 Subject: [PATCH 05/22] Add test-routing-policy-rule --- src/network/meson.build | 6 ++ src/network/test-routing-policy-rule.c | 93 ++++++++++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 src/network/test-routing-policy-rule.c diff --git a/src/network/meson.build b/src/network/meson.build index ed68faca0c..f97484eb26 100644 --- a/src/network/meson.build +++ b/src/network/meson.build @@ -156,6 +156,12 @@ if conf.get('ENABLE_NETWORKD') == 1 libshared], [threads]], + [['src/network/test-routing-policy-rule.c'], + [libnetworkd_core, + libsystemd_network, + libudev], + []], + [['src/network/test-network-tables.c', 'src/network/test-network-tables.c', test_tables_h], diff --git a/src/network/test-routing-policy-rule.c b/src/network/test-routing-policy-rule.c new file mode 100644 index 0000000000..a4c568e325 --- /dev/null +++ b/src/network/test-routing-policy-rule.c @@ -0,0 +1,93 @@ +/*** + SPDX-License-Identifier: LGPL-2.1+ + + This file is part of systemd. + + Copyright 2017 Zbigniew Jędrzejewski-Szmek + + systemd is free software; you can redistribute it and/or modify it + under the terms of the GNU Lesser General Public License as published by + the Free Software Foundation; either version 2.1 of the License, or + (at your option) any later version. + + systemd is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public License + along with systemd; If not, see . +***/ + +#include "fd-util.h" +#include "fileio.h" +#include "log.h" +#include "macro.h" +#include "network-internal.h" +#include "networkd-manager.h" +#include "string-util.h" + +static void test_rule_serialization(const char *title, const char *ruleset, const char *expected) { + char pattern[] = "/tmp/systemd-test-routing-policy-rule.XXXXXX", + pattern2[] = "/tmp/systemd-test-routing-policy-rule.XXXXXX", + pattern3[] = "/tmp/systemd-test-routing-policy-rule.XXXXXX"; + const char *cmd; + int fd, fd2, fd3; + _cleanup_fclose_ FILE *f = NULL, *f2 = NULL, *f3 = NULL; + _cleanup_set_free_free_ Set *rules = NULL; + _cleanup_free_ char *buf = NULL; + size_t buf_size; + + log_info("========== %s ==========", title); + log_info("put:\n%s\n", ruleset); + + assert_se((fd = mkostemp_safe(pattern)) >= 0); + assert_se(f = fdopen(fd, "a+e")); + assert_se(write_string_stream(f, ruleset, 0) == 0); + + assert_se(routing_policy_load_rules(pattern, &rules) == 0); + + assert_se((fd2 = mkostemp_safe(pattern2)) >= 0); + assert_se(f2 = fdopen(fd2, "a+e")); + + assert_se(routing_policy_serialize_rules(rules, f2) == 0); + assert_se(fflush_and_check(f2) == 0); + + assert_se(read_full_file(pattern2, &buf, &buf_size) == 0); + + log_info("got:\n%s", buf); + + assert_se((fd3 = mkostemp_safe(pattern3)) >= 0); + assert_se(f3 = fdopen(fd3, "we")); + assert_se(write_string_stream(f3, expected ?: ruleset, 0) == 0); + + cmd = strjoina("diff -u ", pattern3, " ", pattern2); + log_info("$ %s", cmd); + assert_se(system(cmd) == 0); +} + +int main(int argc, char **argv) { + _cleanup_free_ char *p = NULL; + + log_set_max_level(LOG_DEBUG); + log_parse_environment(); + log_open(); + + test_rule_serialization("basic parsing", + "RULE=from=1.2.3.4/32 to=2.3.4.5/32 tos=5 fwmark=1/2 table=10", NULL); + + test_rule_serialization("ignored values", + "RULE=something=to=ignore from=1.2.3.4/32 from=1.2.3.4/32" + " \t to=2.3.4.5/24 to=2.3.4.5/32 tos=5 fwmark=2 fwmark=1 table=10 table=20", + "RULE=from=1.2.3.4/32" + " to=2.3.4.5/32 tos=5 fwmark=1/0 table=20"); + + test_rule_serialization("ipv6", + "RULE=from=1::2/64 to=2::3/64 table=6", NULL); + + assert_se(asprintf(&p, "RULE=from=1::2/64 to=2::3/64 table=%d", RT_TABLE_MAIN) >= 0); + test_rule_serialization("default table", + "RULE=from=1::2/64 to=2::3/64", p); + + return 0; +} From 9491f55f74a1c61a8c6c79c3ef16a32f2722e232 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 11:51:08 +0000 Subject: [PATCH 06/22] networkd: fix serialization of {Incoming,Outgoing}Interface Let's just say that the code wasn't fully functional ;( Since we only had the parser for serialization, and not the writer, we are free to change the format. So while at it, let's use shorter names in the serialization format that match the surrounding style. --- src/network/networkd-routing-policy-rule.c | 22 ++++++++++++++++++---- src/network/test-routing-policy-rule.c | 7 +++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index 09c1015f2c..20f748944e 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -905,6 +905,20 @@ int routing_policy_serialize_rules(Set *rules, FILE *f) { space = true; } + if (rule->iif) { + fprintf(f, "%siif=%s", + space ? " " : "", + rule->iif); + space = true; + } + + if (rule->oif) { + fprintf(f, "%soif=%s", + space ? " " : "", + rule->oif); + space = true; + } + fprintf(f, "%stable=%"PRIu32 "\n", space ? " " : "", rule->table); @@ -1001,14 +1015,14 @@ int routing_policy_load_rules(const char *state_file, Set **rules) { log_error_errno(r, "Failed to parse RPDB rule firewall mark or mask, ignoring: %s", a); continue; } - } else if (streq(a, "IncomingInterface")) { + } else if (streq(a, "iif")) { - rule->iif = strdup(a); + rule->iif = strdup(b); if (!rule->iif) return log_oom(); - } else if (streq(a, "OutgoingInterface")) { + } else if (streq(a, "oif")) { - rule->oif = strdup(a); + rule->oif = strdup(b); if (!rule->oif) return log_oom(); } diff --git a/src/network/test-routing-policy-rule.c b/src/network/test-routing-policy-rule.c index a4c568e325..c619dbc159 100644 --- a/src/network/test-routing-policy-rule.c +++ b/src/network/test-routing-policy-rule.c @@ -89,5 +89,12 @@ int main(int argc, char **argv) { test_rule_serialization("default table", "RULE=from=1::2/64 to=2::3/64", p); + test_rule_serialization("incoming interface", + "RULE=from=1::2/64 to=2::3/64 table=1 iif=lo", + "RULE=from=1::2/64 to=2::3/64 iif=lo table=1"); + + test_rule_serialization("outgoing interface", + "RULE=from=1::2/64 to=2::3/64 oif=eth0 table=1", NULL); + return 0; } From 93f9da6e0a9e701e4532056754c2d867e86d8dd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 12:01:41 +0000 Subject: [PATCH 07/22] networkd: plug some memleaks in rule serialization/deserialization This fixes the (mostly theoretical, since we're only parsing data that we write ourselves) memleak when iif or oif is deserialized multiple times. Unfortunately it does not fix the memleak when rule is freed, but that'll require a bigger effort. --- src/network/networkd-routing-policy-rule.c | 7 +++---- src/network/test-routing-policy-rule.c | 4 ++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-routing-policy-rule.c b/src/network/networkd-routing-policy-rule.c index 20f748944e..f4b3b7180b 100644 --- a/src/network/networkd-routing-policy-rule.c +++ b/src/network/networkd-routing-policy-rule.c @@ -1017,13 +1017,12 @@ int routing_policy_load_rules(const char *state_file, Set **rules) { } } else if (streq(a, "iif")) { - rule->iif = strdup(b); - if (!rule->iif) + if (free_and_strdup(&rule->iif, b) < 0) return log_oom(); + } else if (streq(a, "oif")) { - rule->oif = strdup(b); - if (!rule->oif) + if (free_and_strdup(&rule->oif, b) < 0) return log_oom(); } } diff --git a/src/network/test-routing-policy-rule.c b/src/network/test-routing-policy-rule.c index c619dbc159..fc6eb5626d 100644 --- a/src/network/test-routing-policy-rule.c +++ b/src/network/test-routing-policy-rule.c @@ -96,5 +96,9 @@ int main(int argc, char **argv) { test_rule_serialization("outgoing interface", "RULE=from=1::2/64 to=2::3/64 oif=eth0 table=1", NULL); + test_rule_serialization("freeing interface names", + "RULE=from=1::2/64 to=2::3/64 iif=e0 iif=e1 oif=e0 oif=e1 table=1", + "RULE=from=1::2/64 to=2::3/64 iif=e1 oif=e1 table=1"); + return 0; } From bbd1590085a2042c3dd452085babe33e1cfda04f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 12:08:37 +0000 Subject: [PATCH 08/22] networkd: fix reversed arguments Found by coverity. Also drop unused return value while at it. --- src/network/networkd-manager.c | 2 +- src/network/networkd-route.c | 4 +--- src/network/networkd-route.h | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 852d690ef5..ad3c442c8e 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -471,7 +471,7 @@ int manager_rtnl_process_route(sd_netlink *rtnl, sd_netlink_message *message, vo return 0; } - route_update(route, &src, src_prefixlen, &gw, &prefsrc, scope, rt_type, protocol); + route_update(route, &src, src_prefixlen, &gw, &prefsrc, scope, protocol, rt_type); break; diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 6139ad3669..b0ad707811 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -368,7 +368,7 @@ int route_add( return 0; } -int route_update(Route *route, +void route_update(Route *route, const union in_addr_union *src, unsigned char src_prefixlen, const union in_addr_union *gw, @@ -389,8 +389,6 @@ int route_update(Route *route, route->scope = scope; route->protocol = protocol; route->type = type; - - return 0; } int route_remove(Route *route, Link *link, diff --git a/src/network/networkd-route.h b/src/network/networkd-route.h index ff03a39c0f..cfb85cbd6d 100644 --- a/src/network/networkd-route.h +++ b/src/network/networkd-route.h @@ -64,7 +64,7 @@ int route_remove(Route *route, Link *link, sd_netlink_message_handler_t callback int route_get(Link *link, int family, const union in_addr_union *dst, unsigned char dst_prefixlen, unsigned char tos, uint32_t priority, uint32_t table, Route **ret); int route_add(Link *link, int family, const union in_addr_union *dst, unsigned char dst_prefixlen, unsigned char tos, uint32_t priority, uint32_t table, Route **ret); int route_add_foreign(Link *link, int family, const union in_addr_union *dst, unsigned char dst_prefixlen, unsigned char tos, uint32_t priority, uint32_t table, Route **ret); -int route_update(Route *route, const union in_addr_union *src, unsigned char src_prefixlen, const union in_addr_union *gw, const union in_addr_union *prefsrc, unsigned char scope, unsigned char protocol, unsigned char type); +void route_update(Route *route, const union in_addr_union *src, unsigned char src_prefixlen, const union in_addr_union *gw, const union in_addr_union *prefsrc, unsigned char scope, unsigned char protocol, unsigned char type); int route_expire_handler(sd_event_source *s, uint64_t usec, void *userdata); From bdd73ac5c221d393cb9c560c5b212fffeda4f296 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 12:16:09 +0000 Subject: [PATCH 09/22] dissect-image: simplify check for passphrase presence m->encrypted is set when fstype=="crypto_LUKS", but this is not obvious when reading decrypt_partition(). Just check if passphrase is set before using it. --- src/shared/dissect-image.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/shared/dissect-image.c b/src/shared/dissect-image.c index 75e3431f28..219ba8cb0b 100644 --- a/src/shared/dissect-image.c +++ b/src/shared/dissect-image.c @@ -861,6 +861,9 @@ static int decrypt_partition( if (!streq(m->fstype, "crypto_LUKS")) return 0; + if (!passphrase) + return -ENOKEY; + r = make_dm_name_and_node(m->node, "-decrypted", &name, &node); if (r < 0) return r; @@ -1006,9 +1009,6 @@ int dissected_image_decrypt( } #if HAVE_LIBCRYPTSETUP - if (m->encrypted && !passphrase) - return -ENOKEY; - d = new0(DecryptedImage, 1); if (!d) return -ENOMEM; From bceda88b5234a6d2ce53073918bc5ee4ac7a869f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 12:22:56 +0000 Subject: [PATCH 10/22] boot/efi: fix NULL dereference The comment above makes the intent of the code pretty clear: "use security2_protocol == NULL as indicator". So revert the condition in the check and fix the logic in the comment while at it. The question is how this could have ever worked: if BS->LocateProtocol (which is supposedly optional) ever failed, we'd crash here. Strange. Found by coverity. --- src/boot/efi/shim.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/boot/efi/shim.c b/src/boot/efi/shim.c index 6b83af1f82..6da9ee889f 100644 --- a/src/boot/efi/shim.c +++ b/src/boot/efi/shim.c @@ -208,9 +208,9 @@ EFI_STATUS security_policy_install(void) { return EFI_ALREADY_STARTED; /* - * Don't bother with status here. The call is allowed - * to fail, since SECURITY2 was introduced in PI 1.2.1 - * If it fails, use security2_protocol == NULL as indicator + * Don't bother with status here. The call is allowed + * to fail, since SECURITY2 was introduced in PI 1.2.1. + * Use security2_protocol == NULL as indicator. */ uefi_call_wrapper(BS->LocateProtocol, 3, (EFI_GUID*) &security2_protocol_guid, NULL, (VOID**) &security2_protocol); @@ -219,14 +219,14 @@ EFI_STATUS security_policy_install(void) { if (status != EFI_SUCCESS) return status; - if (!security2_protocol) { + esfas = security_protocol->FileAuthenticationState; + security_protocol->FileAuthenticationState = security_policy_authentication; + + if (security2_protocol) { es2fa = security2_protocol->FileAuthentication; security2_protocol->FileAuthentication = security2_policy_authentication; } - esfas = security_protocol->FileAuthenticationState; - security_protocol->FileAuthenticationState = security_policy_authentication; - return EFI_SUCCESS; } From 10a6e01eaa9e340c4edafa771c001986bdcdcfc6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 12:36:21 +0000 Subject: [PATCH 11/22] boot/shim: fix char ** vs char * mismatch This code cannot have ever worked ;( Found by coverity, obvious when the type is not obfuscated to void*. --- src/boot/efi/shim.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/boot/efi/shim.c b/src/boot/efi/shim.c index 6da9ee889f..d66b39be23 100644 --- a/src/boot/efi/shim.c +++ b/src/boot/efi/shim.c @@ -162,7 +162,7 @@ static EFIAPI EFI_STATUS security_policy_authentication (const EFI_SECURITY_PROT EFI_DEVICE_PATH *dev_path; EFI_HANDLE h; EFI_FILE *root; - VOID *file_buffer = NULL; + CHAR8 *file_buffer = NULL; UINTN file_size; CHAR16 *dev_path_str; @@ -182,7 +182,7 @@ static EFIAPI EFI_STATUS security_policy_authentication (const EFI_SECURITY_PROT dev_path_str = DevicePathToStr(dev_path); FreePool(dev_path); - file_size = file_read(root, dev_path_str, 0, 0, file_buffer); + file_size = file_read(root, dev_path_str, 0, 0, &file_buffer); FreePool(dev_path_str); uefi_call_wrapper(root->Close, 1, root); From ef51180b625c36a6ae42eea6d387e32760c281af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 12:47:52 +0000 Subject: [PATCH 12/22] test-ndisc-ra: add assert_se() to make coverity happy --- src/libsystemd-network/test-ndisc-ra.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsystemd-network/test-ndisc-ra.c b/src/libsystemd-network/test-ndisc-ra.c index 5914fa2bf0..c1a8d5a00d 100644 --- a/src/libsystemd-network/test-ndisc-ra.c +++ b/src/libsystemd-network/test-ndisc-ra.c @@ -262,7 +262,7 @@ static int radv_recv(sd_event_source *s, int fd, uint32_t revents, void *userdat unsigned char buf[168]; size_t i; - read(test_fd[0], &buf, sizeof(buf)); + assert_se(read(test_fd[0], &buf, sizeof(buf)) == sizeof(buf)); /* router lifetime must be zero when test is stopped */ if (test_stopped) { From 1e40b7490afaf490f2c97217693102c3fe64a3a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 13:25:08 +0000 Subject: [PATCH 13/22] networkd: remove dead code Found by coverity. --- src/network/networkd-manager.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index ad3c442c8e..478fc090b1 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -151,9 +151,6 @@ int manager_connect_bus(Manager *m) { return 0; } - if (r < 0) - return r; - r = sd_bus_add_match(m->bus, &m->prepare_for_sleep_slot, "type='signal'," "sender='org.freedesktop.login1'," From 8d44f5a63746d28b1e6db6f57a8005ba84a8869c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 13:31:46 +0000 Subject: [PATCH 14/22] shim: fix logically dead code The alternate verification path could never be taken. Found by coverity. --- src/boot/efi/shim.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/boot/efi/shim.c b/src/boot/efi/shim.c index d66b39be23..6d7d814c5c 100644 --- a/src/boot/efi/shim.c +++ b/src/boot/efi/shim.c @@ -62,10 +62,7 @@ static BOOLEAN shim_validate(VOID *data, UINT32 size) { if (!shim_lock) return FALSE; - if (shim_lock->shim_verify(data, size) == EFI_SUCCESS) - return TRUE; - - return FALSE; + return shim_lock->shim_verify(data, size) == EFI_SUCCESS; } BOOLEAN secure_boot_enabled(void) { @@ -188,12 +185,10 @@ static EFIAPI EFI_STATUS security_policy_authentication (const EFI_SECURITY_PROT if (shim_validate(file_buffer, file_size)) status = EFI_SUCCESS; - - FreePool(file_buffer); - - /* Try using the platform's native policy.... */ - if (status != EFI_SUCCESS) + else + /* Try using the platform's native policy.... */ status = uefi_call_wrapper(esfas, 3, this, authentication_status, device_path_const); + FreePool(file_buffer); return status; } From 1a96c8e1ccb06f87b6bfaff4639390ecd00af588 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 13:54:55 +0000 Subject: [PATCH 15/22] sd-resolve: check that name fits in the specified packet length Coverity complained that we didn't check if the data is long enough. --- src/libsystemd/sd-resolve/sd-resolve.c | 44 ++++++++++++++++---------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/libsystemd/sd-resolve/sd-resolve.c b/src/libsystemd/sd-resolve/sd-resolve.c index 986768e7b3..e62afd9bfb 100644 --- a/src/libsystemd/sd-resolve/sd-resolve.c +++ b/src/libsystemd/sd-resolve/sd-resolve.c @@ -33,6 +33,7 @@ #include "sd-resolve.h" #include "alloc-util.h" +#include "dns-domain.h" #include "fd-util.h" #include "io-util.h" #include "list.h" @@ -812,25 +813,36 @@ static int handle_response(sd_resolve *resolve, const Packet *packet, size_t len assert(length >= sizeof(NameInfoResponse)); assert(q->type == REQUEST_NAMEINFO); - q->ret = ni_resp->ret; - q->_errno = ni_resp->_errno; - q->_h_errno = ni_resp->_h_errno; + if (ni_resp->hostlen > DNS_HOSTNAME_MAX || + ni_resp->servlen > DNS_HOSTNAME_MAX || + sizeof(NameInfoResponse) + ni_resp->hostlen + ni_resp->servlen > length + 2) { + q->ret = EAI_SYSTEM; + q->_errno = -EIO; + q->_h_errno = 0; - if (ni_resp->hostlen > 0) { - q->host = strndup((const char*) ni_resp + sizeof(NameInfoResponse), ni_resp->hostlen-1); - if (!q->host) { - q->ret = EAI_MEMORY; - q->_errno = ENOMEM; - q->_h_errno = 0; + } else { + q->ret = ni_resp->ret; + q->_errno = ni_resp->_errno; + q->_h_errno = ni_resp->_h_errno; + + if (ni_resp->hostlen > 0) { + q->host = strndup((const char*) ni_resp + sizeof(NameInfoResponse), + ni_resp->hostlen-1); + if (!q->host) { + q->ret = EAI_MEMORY; + q->_errno = ENOMEM; + q->_h_errno = 0; + } } - } - if (ni_resp->servlen > 0) { - q->serv = strndup((const char*) ni_resp + sizeof(NameInfoResponse) + ni_resp->hostlen, ni_resp->servlen-1); - if (!q->serv) { - q->ret = EAI_MEMORY; - q->_errno = ENOMEM; - q->_h_errno = 0; + if (ni_resp->servlen > 0) { + q->serv = strndup((const char*) ni_resp + sizeof(NameInfoResponse) + ni_resp->hostlen, + ni_resp->servlen-1); + if (!q->serv) { + q->ret = EAI_MEMORY; + q->_errno = ENOMEM; + q->_h_errno = 0; + } } } From 595c66a3db50b23d25aee24f253c2a9009344956 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 16:13:04 +0000 Subject: [PATCH 16/22] core/umount: use _cleanup_ in one more place --- src/core/umount.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/core/umount.c b/src/core/umount.c index cd195e12af..cd58a9cf6d 100644 --- a/src/core/umount.c +++ b/src/core/umount.c @@ -159,7 +159,7 @@ static int swap_list_get(MountPoint **head) { for (i = 2;; i++) { MountPoint *swap; - char *dev = NULL, *d; + _cleanup_free_ char *dev = NULL, *d = NULL; int k; k = fscanf(proc_swaps, @@ -175,27 +175,21 @@ static int swap_list_get(MountPoint **head) { break; log_warning("Failed to parse /proc/swaps:%u.", i); - free(dev); continue; } - if (endswith(dev, " (deleted)")) { - free(dev); + if (endswith(dev, " (deleted)")) continue; - } r = cunescape(dev, UNESCAPE_RELAX, &d); - free(dev); if (r < 0) return r; swap = new0(MountPoint, 1); - if (!swap) { - free(d); + if (!swap) return -ENOMEM; - } - swap->path = d; + free_and_replace(swap->path, d); LIST_PREPEND(mount_point, *head, swap); } From b0fe73d740e5f813fada54d66c73532514bf0ea6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 21:48:46 +0000 Subject: [PATCH 17/22] udevadm-monitor: add some assert_se's to make coverity happy We do that elsewhere and it's nice to be consistent. --- src/udev/udevadm-monitor.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/udev/udevadm-monitor.c b/src/udev/udevadm-monitor.c index a369dc2ac2..c8b85fb0f4 100644 --- a/src/udev/udevadm-monitor.c +++ b/src/udev/udevadm-monitor.c @@ -145,12 +145,12 @@ static int adm_monitor(struct udev *udev, int argc, char *argv[]) { /* set signal handlers */ act.sa_handler = sig_handler; act.sa_flags = SA_RESTART; - sigaction(SIGINT, &act, NULL); - sigaction(SIGTERM, &act, NULL); - sigemptyset(&mask); - sigaddset(&mask, SIGINT); - sigaddset(&mask, SIGTERM); - sigprocmask(SIG_UNBLOCK, &mask, NULL); + assert_se(sigaction(SIGINT, &act, NULL) == 0); + assert_se(sigaction(SIGTERM, &act, NULL) == 0); + assert_se(sigemptyset(&mask) == 0); + assert_se(sigaddset(&mask, SIGINT) == 0); + assert_se(sigaddset(&mask, SIGTERM) == 0); + assert_se(sigprocmask(SIG_UNBLOCK, &mask, NULL) == 0); /* Callers are expecting to see events as they happen: Line buffering */ setlinebuf(stdout); From ca9b2b4dc1bced24a9359fef6da797f785628433 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 21:51:11 +0000 Subject: [PATCH 18/22] test-fd-util: add assert_se to make coverity happy --- src/test/test-fd-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test-fd-util.c b/src/test/test-fd-util.c index f45fbbe7e3..e5a0e9d51b 100644 --- a/src/test/test-fd-util.c +++ b/src/test/test-fd-util.c @@ -104,7 +104,7 @@ static void test_open_serialization_fd(void) { fd = open_serialization_fd("test"); assert_se(fd >= 0); - write(fd, "test\n", 5); + assert_se(write(fd, "test\n", 5) == 5); } static void test_acquire_data_fd_one(unsigned flags) { From c47a9f909a661890a3857c1c6f6e075d92b028bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 21:52:32 +0000 Subject: [PATCH 19/22] test-process-util: add (void) cast to make coverity happy The intent is for the call to succeed only when privileged, so make that clear. --- src/test/test-process-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index 47733788f3..9b65f103d7 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -425,7 +425,7 @@ static void test_rename_process_multi(void) { /* child */ test_rename_process_now("one", 1); test_rename_process_now("more", 0); /* longer than "one", hence truncated */ - setresuid(99, 99, 99); + (void) setresuid(99, 99, 99); /* change uid when running privileged */ test_rename_process_now("time!", 0); test_rename_process_now("0", 1); /* shorter than "one", should fit */ test_rename_process_one("", -EINVAL); From 7beb3baf16b5707441a1fb02de78223667b79188 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 21:54:38 +0000 Subject: [PATCH 20/22] test-hostname-util: add assert_se's to make coverity happy --- src/test/test-hostname-util.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/test-hostname-util.c b/src/test/test-hostname-util.c index b73468ddbc..329354831b 100644 --- a/src/test/test-hostname-util.c +++ b/src/test/test-hostname-util.c @@ -110,27 +110,27 @@ static void test_read_etc_hostname(void) { close(fd); /* simple hostname */ - write_string_file(path, "foo", WRITE_STRING_FILE_CREATE); + assert_se(write_string_file(path, "foo", WRITE_STRING_FILE_CREATE) == 0); assert_se(read_etc_hostname(path, &hostname) == 0); assert_se(streq(hostname, "foo")); hostname = mfree(hostname); /* with comment */ - write_string_file(path, "# comment\nfoo", WRITE_STRING_FILE_CREATE); + assert_se(write_string_file(path, "# comment\nfoo", WRITE_STRING_FILE_CREATE) == 0); assert_se(read_etc_hostname(path, &hostname) == 0); assert_se(hostname); assert_se(streq(hostname, "foo")); hostname = mfree(hostname); /* with comment and extra whitespace */ - write_string_file(path, "# comment\n\n foo ", WRITE_STRING_FILE_CREATE); + assert_se(write_string_file(path, "# comment\n\n foo ", WRITE_STRING_FILE_CREATE) == 0); assert_se(read_etc_hostname(path, &hostname) == 0); assert_se(hostname); assert_se(streq(hostname, "foo")); hostname = mfree(hostname); /* cleans up name */ - write_string_file(path, "!foo/bar.com", WRITE_STRING_FILE_CREATE); + assert_se(write_string_file(path, "!foo/bar.com", WRITE_STRING_FILE_CREATE) == 0); assert_se(read_etc_hostname(path, &hostname) == 0); assert_se(hostname); assert_se(streq(hostname, "foobar.com")); @@ -138,7 +138,7 @@ static void test_read_etc_hostname(void) { /* no value set */ hostname = (char*) 0x1234; - write_string_file(path, "# nothing here\n", WRITE_STRING_FILE_CREATE); + assert_se(write_string_file(path, "# nothing here\n", WRITE_STRING_FILE_CREATE) == 0); assert_se(read_etc_hostname(path, &hostname) == -ENOENT); assert_se(hostname == (char*) 0x1234); /* does not touch argument on error */ From d387cfdddd7a843cb6538186ae32222b6d7690a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 22:01:48 +0000 Subject: [PATCH 21/22] pid1: clarify that min_max is always initialized read_one_line_file() always returns <= 0, so the code was OK, but let's write the check a bit differently to make it obvious that min_max is always set. --- src/core/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/main.c b/src/core/main.c index d77946194c..b7fb9b3524 100644 --- a/src/core/main.c +++ b/src/core/main.c @@ -1237,7 +1237,7 @@ static int bump_rlimit_nofile(struct rlimit *saved_rlimit) { /* Get current RLIMIT_NOFILE maximum compiled into the kernel. */ r = read_one_line_file("/proc/sys/fs/nr_open", &nr_open); - if (r == 0) + if (r >= 0) r = safe_atoi(nr_open, &min_max); /* If we fail, fallback to the hard-coded kernel limit of 1024 * 1024. */ if (r < 0) From ba77dac291b788c27405a3b765072c3da339d57e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 27 Nov 2017 22:05:19 +0000 Subject: [PATCH 22/22] pid1: improve syntax in error message --- src/core/load-fragment.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 42dcaea4e1..ac58ce8a14 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -2904,7 +2904,7 @@ static int syscall_filter_parse_one( set = syscall_filter_set_find(t); if (!set) { if (warn) - log_syntax(unit, LOG_WARNING, filename, line, 0, "Don't know system call group, ignoring: %s", t); + log_syntax(unit, LOG_WARNING, filename, line, 0, "Unknown system call group, ignoring: %s", t); return 0; } @@ -2924,7 +2924,7 @@ static int syscall_filter_parse_one( } /* If we previously wanted to forbid a syscall and now - * we want to allow it, then remove it from the list + * we want to allow it, then remove it from the list. */ if (!invert == c->syscall_whitelist) { r = hashmap_put(c->syscall_filter, INT_TO_PTR(id + 1), INT_TO_PTR(errno_num));