From e4bed40f40e32906203ef8886f0435524bd0ebb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 5 Sep 2020 16:29:44 +0200 Subject: [PATCH 01/17] resolve: move handler functions higher No functional change, preparation for subsequent refactoring. --- src/resolve/resolved-dns-stub.c | 112 ++++++++++++++++---------------- 1 file changed, 56 insertions(+), 56 deletions(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 59ced33847..8c4e01ad45 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -477,6 +477,62 @@ static int on_dns_stub_packet_extra(sd_event_source *s, int fd, uint32_t revents return on_dns_stub_packet_internal(s, fd, revents, l->manager, l); } +static int on_dns_stub_stream_packet(DnsStream *s) { + _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL; + + assert(s); + + p = dns_stream_take_read_packet(s); + assert(p); + + if (dns_packet_validate_query(p) > 0) { + log_debug("Got DNS stub TCP query packet for id %u", DNS_PACKET_ID(p)); + + dns_stub_process_query(s->manager, s->stub_listener_extra, s, p); + } else + log_debug("Invalid DNS stub TCP packet, ignoring."); + + return 0; +} + +static int on_dns_stub_stream_internal(sd_event_source *s, int fd, uint32_t revents, Manager *m, DnsStubListenerExtra *l) { + DnsStream *stream; + int cfd, r; + + cfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); + if (cfd < 0) { + if (ERRNO_IS_ACCEPT_AGAIN(errno)) + return 0; + + return -errno; + } + + r = dns_stream_new(m, &stream, DNS_STREAM_STUB, DNS_PROTOCOL_DNS, cfd, NULL); + if (r < 0) { + safe_close(cfd); + return r; + } + + stream->stub_listener_extra = l; + stream->on_packet = on_dns_stub_stream_packet; + stream->complete = dns_stub_stream_complete; + + /* We let the reference to the stream dangle here, it will be dropped later by the complete callback. */ + + return 0; +} + +static int on_dns_stub_stream(sd_event_source *s, int fd, uint32_t revents, void *userdata) { + return on_dns_stub_stream_internal(s, fd, revents, userdata, NULL); +} + +static int on_dns_stub_stream_extra(sd_event_source *s, int fd, uint32_t revents, void *userdata) { + DnsStubListenerExtra *l = userdata; + + assert(l); + return on_dns_stub_stream_internal(s, fd, revents, l->manager, l); +} + static int set_dns_stub_common_socket_options(int fd, int family) { int r; @@ -622,62 +678,6 @@ fail: return log_warning_errno(r, "Failed to listen on UDP socket %s: %m", strnull(pretty)); } -static int on_dns_stub_stream_packet(DnsStream *s) { - _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL; - - assert(s); - - p = dns_stream_take_read_packet(s); - assert(p); - - if (dns_packet_validate_query(p) > 0) { - log_debug("Got DNS stub TCP query packet for id %u", DNS_PACKET_ID(p)); - - dns_stub_process_query(s->manager, s->stub_listener_extra, s, p); - } else - log_debug("Invalid DNS stub TCP packet, ignoring."); - - return 0; -} - -static int on_dns_stub_stream_internal(sd_event_source *s, int fd, uint32_t revents, Manager *m, DnsStubListenerExtra *l) { - DnsStream *stream; - int cfd, r; - - cfd = accept4(fd, NULL, NULL, SOCK_NONBLOCK|SOCK_CLOEXEC); - if (cfd < 0) { - if (ERRNO_IS_ACCEPT_AGAIN(errno)) - return 0; - - return -errno; - } - - r = dns_stream_new(m, &stream, DNS_STREAM_STUB, DNS_PROTOCOL_DNS, cfd, NULL); - if (r < 0) { - safe_close(cfd); - return r; - } - - stream->stub_listener_extra = l; - stream->on_packet = on_dns_stub_stream_packet; - stream->complete = dns_stub_stream_complete; - - /* We let the reference to the stream dangle here, it will be dropped later by the complete callback. */ - - return 0; -} - -static int on_dns_stub_stream(sd_event_source *s, int fd, uint32_t revents, void *userdata) { - return on_dns_stub_stream_internal(s, fd, revents, userdata, NULL); -} - -static int on_dns_stub_stream_extra(sd_event_source *s, int fd, uint32_t revents, void *userdata) { - DnsStubListenerExtra *l = userdata; - - assert(l); - return on_dns_stub_stream_internal(s, fd, revents, l->manager, l); -} - static int manager_dns_stub_tcp_fd(Manager *m) { union sockaddr_union sa = { .in.sin_family = AF_INET, From b5febb3f56d17767e5e46e1fd610df2e762dc384 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Sep 2020 16:14:54 +0200 Subject: [PATCH 02/17] resolved: unify the two functions to create extra stubs There is a minor functional change: IPV6_FREEBIND is set of IPv6 sockets, not IP_FREEBIND. This was missed in af8b1384, but I noticed only after the merging the two functions. And a not-so-minor functional chagnge: 7216a3b5dcde36245 changed manager_dns_stub_tcp_fd_extra() to return the fd even if the source was already initialized, but it didn't do the same change for manager_dns_stub_udp_fd_extra(), so it would return 0 in that case. But 0354029bf572489b uses manager_dns_stub_udp_fd_extra() when preparing to call manager_send(), and will pass 0 as the fd in that case. For both socket types fd is now always returned. --- src/resolve/resolved-dns-stub.c | 146 ++++++++++---------------------- 1 file changed, 46 insertions(+), 100 deletions(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 8c4e01ad45..2c59b3a85e 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -15,7 +15,7 @@ * IP and UDP header sizes */ #define ADVERTISE_DATAGRAM_SIZE_MAX (65536U-14U-20U-8U) -static int manager_dns_stub_udp_fd_extra(Manager *m, DnsStubListenerExtra *l); +static int manager_dns_stub_fd_extra(Manager *m, DnsStubListenerExtra *l, int type); static void dns_stub_listener_extra_hash_func(const DnsStubListenerExtra *a, struct siphash *state) { assert(a); @@ -217,7 +217,7 @@ static int dns_stub_send( * is because otherwise the kernel will choose it automatically based on the routing table and will * thus pick 127.0.0.1 rather than 127.0.0.53. */ r = manager_send(m, - manager_dns_stub_udp_fd_extra(m, l), + manager_dns_stub_fd_extra(m, l, SOCK_DGRAM), l ? p->ifindex : LOOPBACK_IFINDEX, /* force loopback iface if this is the main listener stub */ p->family, &p->sender, p->sender_port, &p->destination, reply); @@ -605,19 +605,25 @@ static int manager_dns_stub_udp_fd(Manager *m) { return TAKE_FD(fd); } -static int manager_dns_stub_udp_fd_extra(Manager *m, DnsStubListenerExtra *l) { +static int manager_dns_stub_fd_extra(Manager *m, DnsStubListenerExtra *l, int type) { _cleanup_free_ char *pretty = NULL; _cleanup_close_ int fd = -1; union sockaddr_union sa; int r; assert(m); + assert(IN_SET(type, SOCK_DGRAM, SOCK_STREAM)); - if (!l) - return manager_dns_stub_udp_fd(m); + if (!l) { + if (type == SOCK_DGRAM) + return manager_dns_stub_udp_fd(m); + else + return manager_dns_stub_tcp_fd(m); + } - if (l->udp_event_source) - return 0; + sd_event_source **event_source = type == SOCK_DGRAM ? &l->udp_event_source : &l->tcp_event_source; + if (*event_source) + return sd_event_source_get_io_fd(*event_source); if (l->family == AF_INET) sa = (union sockaddr_union) { @@ -632,40 +638,55 @@ static int manager_dns_stub_udp_fd_extra(Manager *m, DnsStubListenerExtra *l) { .in6.sin6_addr = l->address.in6, }; - fd = socket(l->family, SOCK_DGRAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0); + fd = socket(l->family, type | SOCK_CLOEXEC | SOCK_NONBLOCK, 0); if (fd < 0) { r = -errno; goto fail; } - if (l->family == AF_INET) { - r = setsockopt_int(fd, IPPROTO_IP, IP_FREEBIND, true); - if (r < 0) - goto fail; - } - r = set_dns_stub_common_socket_options(fd, l->family); if (r < 0) goto fail; + /* Do not set IP_TTL for extra DNS stub listners, as the address may not be local and in that case + * people may want ttl > 1. */ + + if (l->family == AF_INET) + r = setsockopt_int(fd, IPPROTO_IP, IP_FREEBIND, true); + else + r = setsockopt_int(fd, IPPROTO_IPV6, IPV6_FREEBIND, true); + if (r < 0) + goto fail; + if (bind(fd, &sa.sa, SOCKADDR_LEN(sa)) < 0) { r = -errno; goto fail; } - r = sd_event_add_io(m->event, &l->udp_event_source, fd, EPOLLIN, on_dns_stub_packet_extra, l); + if (type == SOCK_STREAM && + listen(fd, SOMAXCONN) < 0) { + r = -errno; + goto fail; + } + + r = sd_event_add_io(m->event, event_source, fd, EPOLLIN, + type == SOCK_DGRAM ? on_dns_stub_packet_extra : on_dns_stub_stream_extra, + l); if (r < 0) goto fail; - r = sd_event_source_set_io_fd_own(l->udp_event_source, true); + r = sd_event_source_set_io_fd_own(*event_source, true); if (r < 0) goto fail; - (void) sd_event_source_set_description(l->udp_event_source, "dns-stub-udp-extra"); + (void) sd_event_source_set_description(*event_source, + type == SOCK_DGRAM ? "dns-stub-udp-extra" : "dns-stub-tcp-extra"); if (DEBUG_LOGGING) { (void) in_addr_port_to_string(l->family, &l->address, l->port, &pretty); - log_debug("Listening on UDP socket %s.", strnull(pretty)); + log_debug("Listening on %s socket %s.", + type == SOCK_DGRAM ? "UDP" : "TCP", + strnull(pretty)); } return TAKE_FD(fd); @@ -673,9 +694,11 @@ static int manager_dns_stub_udp_fd_extra(Manager *m, DnsStubListenerExtra *l) { fail: assert(r < 0); (void) in_addr_port_to_string(l->family, &l->address, l->port, &pretty); - if (r == -EADDRINUSE) - return log_warning_errno(r, "Another process is already listening on UDP socket %s: %m", strnull(pretty)); - return log_warning_errno(r, "Failed to listen on UDP socket %s: %m", strnull(pretty)); + return log_warning_errno(r, + r == -EADDRINUSE ? "Another process is already listening on %s socket %s: %m" : + "Failed to listen on %s socket %s: %m", + type == SOCK_DGRAM ? "UDP" : "TCP", + strnull(pretty)); } static int manager_dns_stub_tcp_fd(Manager *m) { @@ -726,83 +749,6 @@ static int manager_dns_stub_tcp_fd(Manager *m) { return TAKE_FD(fd); } -static int manager_dns_stub_tcp_fd_extra(Manager *m, DnsStubListenerExtra *l) { - _cleanup_free_ char *pretty = NULL; - _cleanup_close_ int fd = -1; - union sockaddr_union sa; - int r; - - if (l->tcp_event_source) - return sd_event_source_get_io_fd(l->tcp_event_source);; - - if (l->family == AF_INET) - sa = (union sockaddr_union) { - .in.sin_family = l->family, - .in.sin_port = htobe16(l->port != 0 ? l->port : 53U), - .in.sin_addr = l->address.in, - }; - else - sa = (union sockaddr_union) { - .in6.sin6_family = l->family, - .in6.sin6_port = htobe16(l->port != 0 ? l->port : 53U), - .in6.sin6_addr = l->address.in6, - }; - - fd = socket(l->family, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0); - if (fd < 0) { - r = -errno; - goto fail; - } - - r = set_dns_stub_common_socket_options(fd, l->family); - if (r < 0) - goto fail; - - /* Do not set IP_TTL for extra DNS stub listners, as the address may not be local and in that - * case people may want ttl > 1. */ - - if (l->family == AF_INET) - r = setsockopt_int(fd, IPPROTO_IP, IP_FREEBIND, true); - else - r = setsockopt_int(fd, IPPROTO_IPV6, IPV6_FREEBIND, true); - if (r < 0) - goto fail; - - if (bind(fd, &sa.sa, SOCKADDR_LEN(sa)) < 0) { - r = -errno; - goto fail; - } - - if (listen(fd, SOMAXCONN) < 0) { - r = -errno; - goto fail; - } - - r = sd_event_add_io(m->event, &l->tcp_event_source, fd, EPOLLIN, on_dns_stub_stream_extra, l); - if (r < 0) - goto fail; - - r = sd_event_source_set_io_fd_own(l->tcp_event_source, true); - if (r < 0) - goto fail; - - (void) sd_event_source_set_description(l->tcp_event_source, "dns-stub-tcp-extra"); - - if (DEBUG_LOGGING) { - (void) in_addr_port_to_string(l->family, &l->address, l->port, &pretty); - log_debug("Listening on TCP socket %s.", strnull(pretty)); - } - - return TAKE_FD(fd); - -fail: - assert(r < 0); - (void) in_addr_port_to_string(l->family, &l->address, l->port, &pretty); - if (r == -EADDRINUSE) - return log_warning_errno(r, "Another process is already listening on TCP socket %s: %m", strnull(pretty)); - return log_warning_errno(r, "Failed to listen on TCP socket %s: %m", strnull(pretty)); -} - int manager_dns_stub_start(Manager *m) { const char *t = "UDP"; int r = 0; @@ -846,9 +792,9 @@ int manager_dns_stub_start(Manager *m) { ORDERED_SET_FOREACH(l, m->dns_extra_stub_listeners) { if (FLAGS_SET(l->mode, DNS_STUB_LISTENER_UDP)) - (void) manager_dns_stub_udp_fd_extra(m, l); + (void) manager_dns_stub_fd_extra(m, l, SOCK_DGRAM); if (FLAGS_SET(l->mode, DNS_STUB_LISTENER_TCP)) - (void) manager_dns_stub_tcp_fd_extra(m, l); + (void) manager_dns_stub_fd_extra(m, l, SOCK_STREAM); } } From d491917c9fa816964c0c4f57b8499e8e51776241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 5 Sep 2020 16:45:58 +0200 Subject: [PATCH 03/17] resolved: unify the two functions to create main stubs There is a small functional difference: IP_TTL==1 is now also set for the UDP socket. I assume that it wasn't set by mistake. --- src/resolve/resolved-dns-stub.c | 104 ++++++++++---------------------- 1 file changed, 32 insertions(+), 72 deletions(-) diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 2c59b3a85e..572be26c2d 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -564,19 +564,22 @@ static int set_dns_stub_common_socket_options(int fd, int family) { return 0; } -static int manager_dns_stub_udp_fd(Manager *m) { +static int manager_dns_stub_fd(Manager *m, int type) { union sockaddr_union sa = { .in.sin_family = AF_INET, - .in.sin_port = htobe16(53), .in.sin_addr.s_addr = htobe32(INADDR_DNS_STUB), + .in.sin_port = htobe16(53), }; _cleanup_close_ int fd = -1; int r; - if (m->dns_stub_udp_event_source) - return sd_event_source_get_io_fd(m->dns_stub_udp_event_source); + assert(IN_SET(type, SOCK_DGRAM, SOCK_STREAM)); - fd = socket(AF_INET, SOCK_DGRAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0); + sd_event_source **event_source = type == SOCK_DGRAM ? &m->dns_stub_udp_event_source : &m->dns_stub_tcp_event_source; + if (*event_source) + return sd_event_source_get_io_fd(*event_source); + + fd = socket(AF_INET, type | SOCK_CLOEXEC | SOCK_NONBLOCK, 0); if (fd < 0) return -errno; @@ -589,18 +592,29 @@ static int manager_dns_stub_udp_fd(Manager *m) { if (r < 0) return r; + r = setsockopt_int(fd, IPPROTO_IP, IP_TTL, 1); + if (r < 0) + return r; + if (bind(fd, &sa.sa, sizeof(sa.in)) < 0) return -errno; - r = sd_event_add_io(m->event, &m->dns_stub_udp_event_source, fd, EPOLLIN, on_dns_stub_packet, m); + if (type == SOCK_STREAM && + listen(fd, SOMAXCONN) < 0) + return -errno; + + r = sd_event_add_io(m->event, event_source, fd, EPOLLIN, + type == SOCK_DGRAM ? on_dns_stub_packet : on_dns_stub_stream, + m); if (r < 0) return r; - r = sd_event_source_set_io_fd_own(m->dns_stub_udp_event_source, true); + r = sd_event_source_set_io_fd_own(*event_source, true); if (r < 0) return r; - (void) sd_event_source_set_description(m->dns_stub_udp_event_source, "dns-stub-udp"); + (void) sd_event_source_set_description(*event_source, + type == SOCK_DGRAM ? "dns-stub-udp" : "dns-stub-tcp"); return TAKE_FD(fd); } @@ -614,12 +628,8 @@ static int manager_dns_stub_fd_extra(Manager *m, DnsStubListenerExtra *l, int ty assert(m); assert(IN_SET(type, SOCK_DGRAM, SOCK_STREAM)); - if (!l) { - if (type == SOCK_DGRAM) - return manager_dns_stub_udp_fd(m); - else - return manager_dns_stub_tcp_fd(m); - } + if (!l) + return manager_dns_stub_fd(m, type); sd_event_source **event_source = type == SOCK_DGRAM ? &l->udp_event_source : &l->tcp_event_source; if (*event_source) @@ -701,54 +711,6 @@ fail: strnull(pretty)); } -static int manager_dns_stub_tcp_fd(Manager *m) { - union sockaddr_union sa = { - .in.sin_family = AF_INET, - .in.sin_addr.s_addr = htobe32(INADDR_DNS_STUB), - .in.sin_port = htobe16(53), - }; - _cleanup_close_ int fd = -1; - int r; - - if (m->dns_stub_tcp_event_source) - return sd_event_source_get_io_fd(m->dns_stub_tcp_event_source); - - fd = socket(AF_INET, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0); - if (fd < 0) - return -errno; - - r = set_dns_stub_common_socket_options(fd, AF_INET); - if (r < 0) - return r; - - r = setsockopt_int(fd, IPPROTO_IP, IP_TTL, 1); - if (r < 0) - return r; - - /* Make sure no traffic from outside the local host can leak to onto this socket */ - r = socket_bind_to_ifindex(fd, LOOPBACK_IFINDEX); - if (r < 0) - return r; - - if (bind(fd, &sa.sa, sizeof(sa.in)) < 0) - return -errno; - - if (listen(fd, SOMAXCONN) < 0) - return -errno; - - r = sd_event_add_io(m->event, &m->dns_stub_tcp_event_source, fd, EPOLLIN, on_dns_stub_stream, m); - if (r < 0) - return r; - - r = sd_event_source_set_io_fd_own(m->dns_stub_tcp_event_source, true); - if (r < 0) - return r; - - (void) sd_event_source_set_description(m->dns_stub_tcp_event_source, "dns-stub-tcp"); - - return TAKE_FD(fd); -} - int manager_dns_stub_start(Manager *m) { const char *t = "UDP"; int r = 0; @@ -764,23 +726,21 @@ int manager_dns_stub_start(Manager *m) { "UDP/TCP"); if (FLAGS_SET(m->dns_stub_listener_mode, DNS_STUB_LISTENER_UDP)) - r = manager_dns_stub_udp_fd(m); + r = manager_dns_stub_fd(m, SOCK_DGRAM); if (r >= 0 && FLAGS_SET(m->dns_stub_listener_mode, DNS_STUB_LISTENER_TCP)) { t = "TCP"; - r = manager_dns_stub_tcp_fd(m); + r = manager_dns_stub_fd(m, SOCK_STREAM); } if (IN_SET(r, -EADDRINUSE, -EPERM)) { - if (r == -EADDRINUSE) - log_warning_errno(r, - "Another process is already listening on %s socket 127.0.0.53:53.\n" - "Turning off local DNS stub support.", t); - else - log_warning_errno(r, - "Failed to listen on %s socket 127.0.0.53:53: %m.\n" - "Turning off local DNS stub support.", t); + log_warning_errno(r, + r == -EADDRINUSE ? "Another process is already listening on %s socket 127.0.0.53:53.\n" + "Turning off local DNS stub support." : + "Failed to listen on %s socket 127.0.0.53:53: %m.\n" + "Turning off local DNS stub support.", + t); manager_dns_stub_stop(m); } else if (r < 0) return log_error_errno(r, "Failed to listen on %s socket 127.0.0.53:53: %m", t); From 38c30b35f38298c2fbdcee46bb39aed3baa3019c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 5 Sep 2020 16:55:10 +0200 Subject: [PATCH 04/17] test-in-addr-util: add log headers --- src/test/test-in-addr-util.c | 10 ++++++++-- src/test/test-socket-util.c | 6 ------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/test/test-in-addr-util.c b/src/test/test-in-addr-util.c index 63e9800ec8..a9ecd0627b 100644 --- a/src/test/test-in-addr-util.c +++ b/src/test/test-in-addr-util.c @@ -58,6 +58,8 @@ static void test_in_addr_prefix_from_string_one( } static void test_in_addr_prefix_from_string(void) { + log_info("/* %s */", __func__); + test_in_addr_prefix_from_string_one("", AF_INET, -EINVAL, NULL, 0, -EINVAL, 0, -EINVAL, 0); test_in_addr_prefix_from_string_one("/", AF_INET, -EINVAL, NULL, 0, -EINVAL, 0, -EINVAL, 0); test_in_addr_prefix_from_string_one("/8", AF_INET, -EINVAL, NULL, 0, -EINVAL, 0, -EINVAL, 0); @@ -90,7 +92,7 @@ static void test_in_addr_prefix_to_string_valid(int family, const char *p) { union in_addr_union u; unsigned char l; - log_info("/* %s */", p); + log_info("%s: %s", __func__, p); assert_se(in_addr_prefix_from_string(p, family, &u, &l) >= 0); assert_se(in_addr_prefix_to_string(family, &u, l, &str) >= 0); @@ -102,7 +104,7 @@ static void test_in_addr_prefix_to_string_unoptimized(int family, const char *p) union in_addr_union u1, u2; unsigned char len1, len2; - log_info("/* %s */", p); + log_info("%s: %s", __func__, p); assert_se(in_addr_prefix_from_string(p, family, &u1, &len1) >= 0); assert_se(in_addr_prefix_to_string(family, &u1, len1, &str1) >= 0); @@ -115,6 +117,8 @@ static void test_in_addr_prefix_to_string_unoptimized(int family, const char *p) } static void test_in_addr_prefix_to_string(void) { + log_info("/* %s */", __func__); + test_in_addr_prefix_to_string_valid(AF_INET, "0.0.0.0/32"); test_in_addr_prefix_to_string_valid(AF_INET, "1.2.3.4/0"); test_in_addr_prefix_to_string_valid(AF_INET, "1.2.3.4/24"); @@ -137,6 +141,8 @@ static void test_in_addr_random_prefix(void) { _cleanup_free_ char *str = NULL; union in_addr_union a; + log_info("/* %s */", __func__); + assert_se(in_addr_from_string(AF_INET, "192.168.10.1", &a) >= 0); assert_se(in_addr_random_prefix(AF_INET, &a, 31, 32) >= 0); diff --git a/src/test/test-socket-util.c b/src/test/test-socket-util.c index 0bc713cb53..d0dda37951 100644 --- a/src/test/test-socket-util.c +++ b/src/test/test-socket-util.c @@ -504,17 +504,11 @@ int main(int argc, char *argv[]) { test_setup_logging(LOG_DEBUG); test_ifname_valid(); - test_socket_print_unix(); - test_sockaddr_equal(); - test_sockaddr_un_len(); - test_in_addr_is_multicast(); - test_getpeercred_getpeergroups(); - test_passfd_read(); test_passfd_contents_read(); test_receive_nopassfd(); From 610618ff61a87ae652116d2e0b58fa556b3c66ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Sep 2020 12:58:30 +0200 Subject: [PATCH 05/17] test-socket-netlink: print the proper expected string We would use the return value from the tested function to decide what to print as "expected", which is confusing when something is wrong with the tested function. --- src/test/test-socket-netlink.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/test/test-socket-netlink.c b/src/test/test-socket-netlink.c index f86f123782..f03e455d0e 100644 --- a/src/test/test-socket-netlink.c +++ b/src/test/test-socket-netlink.c @@ -12,11 +12,19 @@ static void test_socket_address_parse_one(const char *in, int ret, int family, c int r; r = socket_address_parse(&a, in); - if (r >= 0) - assert_se(socket_address_print(&a, &out) >= 0); + if (r >= 0) { + r = socket_address_print(&a, &out); + if (r < 0) + log_error_errno(r, "Printing failed for \"%s\": %m", in); + assert(r >= 0); + } - log_info("\"%s\" → %s → \"%s\" (expect \"%s\")", in, - r >= 0 ? "✓" : "✗", empty_to_dash(out), r >= 0 ? expected ?: in : "-"); + log_info("\"%s\" → %s %d → \"%s\" (expect %d / \"%s\")", + in, + r >= 0 ? "✓" : "✗", r, + empty_to_dash(out), + ret, + ret >= 0 ? expected ?: in : "-"); assert_se(r == ret); if (r >= 0) { assert_se(a.sockaddr.sa.sa_family == family); From 25b2d602b91a5b9225e3f7c72d77e51face237ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Sep 2020 10:05:12 +0200 Subject: [PATCH 06/17] shared/socket-netlink: only allow ifindex if explicitly supported Instead of ignoring ifindex if not wanted, refuse it is the caller does not allow it. --- src/shared/socket-netlink.c | 24 ++++++++------ src/test/test-socket-netlink.c | 57 +++++++++++++++++++++++++++++----- 2 files changed, 63 insertions(+), 18 deletions(-) diff --git a/src/shared/socket-netlink.c b/src/shared/socket-netlink.c index 6c121a4c9a..4b52fa3097 100644 --- a/src/shared/socket-netlink.c +++ b/src/shared/socket-netlink.c @@ -345,10 +345,14 @@ int in_addr_port_ifindex_name_from_string_auto( /* This accepts the following: * 192.168.0.1:53#example.com - * [2001:4860:4860::8888]:53%eth0#example.com */ - - /* if ret_port is NULL, then strings with port cannot be specified. - * Also, if ret_server_name is NULL, then server_name cannot be specified. */ + * [2001:4860:4860::8888]:53%eth0#example.com + * + * If ret_port is NULL, then the port cannot be specified. + * If ret_ifindex is NULL, then the interface index cannot be specified. + * If ret_server_name is NULL, then server_name cannot be specified. + * + * ret_family is always AF_INET or AF_INET6. + */ m = strchr(s, '#'); if (m) { @@ -369,15 +373,15 @@ int in_addr_port_ifindex_name_from_string_auto( m = strchr(s, '%'); if (m) { + if (!ret_ifindex) + return -EINVAL; + if (isempty(m + 1)) return -EINVAL; - if (ret_ifindex) { - /* If we shall return the interface index, try to parse it */ - ifindex = resolve_interface(NULL, m + 1); - if (ifindex < 0) - return ifindex; - } + ifindex = resolve_interface(NULL, m + 1); + if (ifindex < 0) + return ifindex; s = buf2 = strndup(s, m - s); if (!buf2) diff --git a/src/test/test-socket-netlink.c b/src/test/test-socket-netlink.c index f03e455d0e..200af19a45 100644 --- a/src/test/test-socket-netlink.c +++ b/src/test/test-socket-netlink.c @@ -293,18 +293,59 @@ static void test_in_addr_ifindex_name_from_string_auto(void) { } static void test_in_addr_port_ifindex_name_from_string_auto_one(const char *str, int family, uint16_t port, int ifindex, const char *server_name) { - _cleanup_free_ char *name = NULL, *x = NULL; union in_addr_union a; uint16_t p; int f, i; + char *fake; - assert_se(in_addr_port_ifindex_name_from_string_auto(str, &f, &a, &p, &i, &name) >= 0); - assert_se(family == f); - assert_se(port == p); - assert_se(ifindex == i); - assert_se(streq_ptr(server_name, name)); - assert_se(in_addr_port_ifindex_name_to_string(f, &a, p, i, name, &x) >= 0); - assert_se(streq(str, x)); + log_info("%s: %s", __func__, str); + + { + _cleanup_free_ char *name = NULL, *x = NULL; + assert_se(in_addr_port_ifindex_name_from_string_auto(str, &f, &a, &p, &i, &name) == 0); + assert_se(family == f); + assert_se(port == p); + assert_se(ifindex == i); + assert_se(streq_ptr(server_name, name)); + assert_se(in_addr_port_ifindex_name_to_string(f, &a, p, i, name, &x) >= 0); + assert_se(streq(str, x)); + } + + if (port > 0) + assert_se(in_addr_port_ifindex_name_from_string_auto(str, &f, &a, NULL, &i, &fake) == -EINVAL); + else { + _cleanup_free_ char *name = NULL, *x = NULL; + assert_se(in_addr_port_ifindex_name_from_string_auto(str, &f, &a, NULL, &i, &name) == 0); + assert_se(family == f); + assert_se(ifindex == i); + assert_se(streq_ptr(server_name, name)); + assert_se(in_addr_port_ifindex_name_to_string(f, &a, 0, i, name, &x) >= 0); + assert_se(streq(str, x)); + } + + if (ifindex > 0) + assert_se(in_addr_port_ifindex_name_from_string_auto(str, &f, &a, &p, NULL, &fake) == -EINVAL); + else { + _cleanup_free_ char *name = NULL, *x = NULL; + assert_se(in_addr_port_ifindex_name_from_string_auto(str, &f, &a, &p, NULL, &name) == 0); + assert_se(family == f); + assert_se(port == p); + assert_se(streq_ptr(server_name, name)); + assert_se(in_addr_port_ifindex_name_to_string(f, &a, p, 0, name, &x) >= 0); + assert_se(streq(str, x)); + } + + if (server_name) + assert_se(in_addr_port_ifindex_name_from_string_auto(str, &f, &a, &p, &i, NULL) == -EINVAL); + else { + _cleanup_free_ char *x = NULL; + assert_se(in_addr_port_ifindex_name_from_string_auto(str, &f, &a, &p, &i, NULL) == 0); + assert_se(family == f); + assert_se(port == p); + assert_se(ifindex == i); + assert_se(in_addr_port_ifindex_name_to_string(f, &a, p, i, NULL, &x) >= 0); + assert_se(streq(str, x)); + } } static void test_in_addr_port_ifindex_name_from_string_auto(void) { From 2313524aa0082b84ae96083cd616120f60a67faa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Sep 2020 14:59:21 +0200 Subject: [PATCH 07/17] basic: convert ifname_valid_full() to take flags and allow numeric interfaces --- src/basic/socket-util.c | 27 +++++++++++++++--------- src/basic/socket-util.h | 9 ++++++-- src/network/networkd-network-gperf.gperf | 2 +- src/test/test-socket-util.c | 18 ++++++++++------ src/udev/net/link-config-gperf.gperf | 3 ++- 5 files changed, 38 insertions(+), 21 deletions(-) diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 4b3a4f206c..1f65015347 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -686,17 +686,19 @@ static const char* const ip_tos_table[] = { DEFINE_STRING_TABLE_LOOKUP_WITH_FALLBACK(ip_tos, int, 0xff); -bool ifname_valid_full(const char *p, bool alternative) { +bool ifname_valid_full(const char *p, IfnameValidFlags flags) { bool numeric = true; /* Checks whether a network interface name is valid. This is inspired by dev_valid_name() in the kernel sources * but slightly stricter, as we only allow non-control, non-space ASCII characters in the interface name. We * also don't permit names that only container numbers, to avoid confusion with numeric interface indexes. */ + assert(!(flags & ~_IFNAME_VALID_ALL)); + if (isempty(p)) return false; - if (alternative) { + if (flags & IFNAME_VALID_ALTERNATIVE) { if (strlen(p) >= ALTIFNAMSIZ) return false; } else { @@ -707,22 +709,27 @@ bool ifname_valid_full(const char *p, bool alternative) { if (dot_or_dot_dot(p)) return false; - while (*p) { - if ((unsigned char) *p >= 127U) + for (const char *t = p; *t; t++) { + if ((unsigned char) *t >= 127U) return false; - if ((unsigned char) *p <= 32U) + if ((unsigned char) *t <= 32U) return false; - if (IN_SET(*p, ':', '/')) + if (IN_SET(*t, ':', '/')) return false; - numeric = numeric && (*p >= '0' && *p <= '9'); - p++; + numeric = numeric && (*t >= '0' && *t <= '9'); } - if (numeric) - return false; + if (numeric) { + if (!(flags & IFNAME_VALID_NUMERIC)) + return false; + + /* Verify that the number is well-formatted and in range. */ + if (parse_ifindex(p) < 0) + return false; + } return true; } diff --git a/src/basic/socket-util.h b/src/basic/socket-util.h index 6a320a976f..fee9055cec 100644 --- a/src/basic/socket-util.h +++ b/src/basic/socket-util.h @@ -130,9 +130,14 @@ static inline int fd_inc_rcvbuf(int fd, size_t n) { int ip_tos_to_string_alloc(int i, char **s); int ip_tos_from_string(const char *s); -bool ifname_valid_full(const char *p, bool alternative); +typedef enum { + IFNAME_VALID_ALTERNATIVE = 1 << 0, + IFNAME_VALID_NUMERIC = 1 << 1, + _IFNAME_VALID_ALL = IFNAME_VALID_ALTERNATIVE | IFNAME_VALID_NUMERIC, +} IfnameValidFlags; +bool ifname_valid_full(const char *p, IfnameValidFlags flags); static inline bool ifname_valid(const char *p) { - return ifname_valid_full(p, false); + return ifname_valid_full(p, 0); } bool address_label_valid(const char *p); diff --git a/src/network/networkd-network-gperf.gperf b/src/network/networkd-network-gperf.gperf index 3f1652b190..d07714f5a1 100644 --- a/src/network/networkd-network-gperf.gperf +++ b/src/network/networkd-network-gperf.gperf @@ -39,7 +39,7 @@ Match.Type, config_parse_match_strv, Match.WLANInterfaceType, config_parse_match_strv, 0, offsetof(Network, match_wlan_iftype) Match.SSID, config_parse_match_strv, 0, offsetof(Network, match_ssid) Match.BSSID, config_parse_hwaddrs, 0, offsetof(Network, match_bssid) -Match.Name, config_parse_match_ifnames, 1, offsetof(Network, match_name) +Match.Name, config_parse_match_ifnames, IFNAME_VALID_ALTERNATIVE, offsetof(Network, match_name) Match.Property, config_parse_match_property, 0, offsetof(Network, match_property) Match.Host, config_parse_net_condition, CONDITION_HOST, offsetof(Network, conditions) Match.Virtualization, config_parse_net_condition, CONDITION_VIRTUALIZATION, offsetof(Network, conditions) diff --git a/src/test/test-socket-util.c b/src/test/test-socket-util.c index d0dda37951..1bb186655d 100644 --- a/src/test/test-socket-util.c +++ b/src/test/test-socket-util.c @@ -26,13 +26,13 @@ assert_cc(SUN_PATH_LEN == 108); static void test_ifname_valid(void) { log_info("/* %s */", __func__); - assert(ifname_valid("foo")); - assert(ifname_valid("eth0")); + assert( ifname_valid("foo")); + assert( ifname_valid("eth0")); assert(!ifname_valid("0")); assert(!ifname_valid("99")); - assert(ifname_valid("a99")); - assert(ifname_valid("99a")); + assert( ifname_valid("a99")); + assert( ifname_valid("99a")); assert(!ifname_valid(NULL)); assert(!ifname_valid("")); @@ -44,9 +44,13 @@ static void test_ifname_valid(void) { assert(ifname_valid("foo.bar")); assert(!ifname_valid("x:y")); - assert(ifname_valid("xxxxxxxxxxxxxxx")); - assert(!ifname_valid("xxxxxxxxxxxxxxxx")); - assert(ifname_valid_full("xxxxxxxxxxxxxxxx", true)); + assert( ifname_valid_full("xxxxxxxxxxxxxxx", 0)); + assert(!ifname_valid_full("xxxxxxxxxxxxxxxx", 0)); + assert( ifname_valid_full("xxxxxxxxxxxxxxxx", IFNAME_VALID_ALTERNATIVE)); + assert( ifname_valid_full("xxxxxxxxxxxxxxxx", IFNAME_VALID_ALTERNATIVE)); + assert(!ifname_valid_full("999", IFNAME_VALID_ALTERNATIVE)); + assert( ifname_valid_full("999", IFNAME_VALID_ALTERNATIVE | IFNAME_VALID_NUMERIC)); + assert(!ifname_valid_full("0", IFNAME_VALID_ALTERNATIVE | IFNAME_VALID_NUMERIC)); } static void test_socket_print_unix_one(const char *in, size_t len_in, const char *expected) { diff --git a/src/udev/net/link-config-gperf.gperf b/src/udev/net/link-config-gperf.gperf index 60a9d21c1d..20f5d7e5a4 100644 --- a/src/udev/net/link-config-gperf.gperf +++ b/src/udev/net/link-config-gperf.gperf @@ -7,6 +7,7 @@ _Pragma("GCC diagnostic ignored \"-Wimplicit-fallthrough\"") #include "ethtool-util.h" #include "link-config.h" #include "network-internal.h" +#include "socket-util.h" %} struct ConfigPerfItem; %null_strings @@ -36,7 +37,7 @@ Link.MACAddressPolicy, config_parse_mac_address_policy, 0, Link.MACAddress, config_parse_hwaddr, 0, offsetof(link_config, mac) Link.NamePolicy, config_parse_name_policy, 0, offsetof(link_config, name_policy) Link.Name, config_parse_ifname, 0, offsetof(link_config, name) -Link.AlternativeName, config_parse_ifnames, 1, offsetof(link_config, alternative_names) +Link.AlternativeName, config_parse_ifnames, IFNAME_VALID_ALTERNATIVE, offsetof(link_config, alternative_names) Link.AlternativeNamesPolicy, config_parse_alternative_names_policy, 0, offsetof(link_config, alternative_names_policy) Link.Alias, config_parse_ifalias, 0, offsetof(link_config, alias) Link.MTUBytes, config_parse_mtu, AF_UNSPEC, offsetof(link_config, mtu) From b16d17a68ae8b44111b08c2689b3dee1d1f90169 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Sep 2020 15:20:31 +0200 Subject: [PATCH 08/17] basic: show interface scope in sockaddr_pretty() If the interface scope is specified, this changes the meaning of the address quite significantly. Let's show the IPv6 scope_id if present. Sadly we don't even have a test for sockaddr_pretty() output :( This will be implicitly tested through socket_address_parse() later on. --- src/basic/socket-util.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 1f65015347..fa51997581 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -399,19 +399,23 @@ int sockaddr_pretty( if (r < 0) return -ENOMEM; } else { - char a[INET6_ADDRSTRLEN]; + char a[INET6_ADDRSTRLEN], ifname[IF_NAMESIZE + 1]; inet_ntop(AF_INET6, &sa->in6.sin6_addr, a, sizeof(a)); + if (sa->in6.sin6_scope_id != 0) + format_ifname_full(sa->in6.sin6_scope_id, ifname, FORMAT_IFNAME_IFINDEX); if (include_port) { r = asprintf(&p, - "[%s]:%u", + "[%s]:%u%s%s", a, - be16toh(sa->in6.sin6_port)); + be16toh(sa->in6.sin6_port), + sa->in6.sin6_scope_id != 0 ? "%" : "", + sa->in6.sin6_scope_id != 0 ? ifname : ""); if (r < 0) return -ENOMEM; } else { - p = strdup(a); + p = sa->in6.sin6_scope_id != 0 ? strjoin(a, "%", ifname) : strdup(a); if (!p) return -ENOMEM; } From c1f848d73c076527b617c132d7dd3cdb1699e2b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Sep 2020 15:38:46 +0200 Subject: [PATCH 09/17] shared: check interface name validity in in_addr_port_ifindex_name_from_string_auto() We don't try to resolve invalid ifnames as all. A different return code is used. This difference will be verified later in test_socket_address_parse() when socket_address_parse() is converted to use in_addr_port_ifindex_name_from_string_auto(). --- src/shared/socket-netlink.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/shared/socket-netlink.c b/src/shared/socket-netlink.c index 4b52fa3097..61db269791 100644 --- a/src/shared/socket-netlink.c +++ b/src/shared/socket-netlink.c @@ -379,6 +379,10 @@ int in_addr_port_ifindex_name_from_string_auto( if (isempty(m + 1)) return -EINVAL; + if (!ifname_valid_full(m + 1, IFNAME_VALID_ALTERNATIVE | IFNAME_VALID_NUMERIC)) + return -EINVAL; /* We want to return -EINVAL for syntactically invalid names, + * and -ENODEV for valid but nonexistent interfaces. */ + ifindex = resolve_interface(NULL, m + 1); if (ifindex < 0) return ifindex; From 222eaaf937d3d39beaea1878e04f20b1689834ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 5 Sep 2020 17:12:06 +0200 Subject: [PATCH 10/17] Get rid of in_addr_port_from_string_auto() again With the commit "shared/socket-netlink: only allow ifindex if explicitly supported" this helper is not necessary anymore. --- src/resolve/resolved-conf.c | 2 +- src/shared/socket-netlink.c | 30 ------------------------------ src/shared/socket-netlink.h | 1 - src/test/test-socket-netlink.c | 31 ------------------------------- 4 files changed, 1 insertion(+), 63 deletions(-) diff --git a/src/resolve/resolved-conf.c b/src/resolve/resolved-conf.c index 184df0337f..08655e115d 100644 --- a/src/resolve/resolved-conf.c +++ b/src/resolve/resolved-conf.c @@ -423,7 +423,7 @@ int config_parse_dns_stub_listener_extra( } } - r = in_addr_port_from_string_auto(p, &stub->family, &stub->address, &stub->port); + r = in_addr_port_ifindex_name_from_string_auto(p, &stub->family, &stub->address, &stub->port, NULL, NULL); if (r < 0) { log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to parse address in %s=%s, ignoring assignment: %m", diff --git a/src/shared/socket-netlink.c b/src/shared/socket-netlink.c index 61db269791..444aae271a 100644 --- a/src/shared/socket-netlink.c +++ b/src/shared/socket-netlink.c @@ -463,36 +463,6 @@ int in_addr_port_ifindex_name_from_string_auto( return r; } -int in_addr_port_from_string_auto( - const char *s, - int *ret_family, - union in_addr_union *ret_address, - uint16_t *ret_port) { - - union in_addr_union addr; - int family, ifindex, r; - uint16_t port; - - assert(s); - - r = in_addr_port_ifindex_name_from_string_auto(s, &family, &addr, &port, &ifindex, NULL); - if (r < 0) - return r; - - /* This does not accept interface specified. */ - if (ifindex != 0) - return -EINVAL; - - if (ret_family) - *ret_family = family; - if (ret_address) - *ret_address = addr; - if (ret_port) - *ret_port = port; - - return r; -} - struct in_addr_full *in_addr_full_free(struct in_addr_full *a) { if (!a) return NULL; diff --git a/src/shared/socket-netlink.h b/src/shared/socket-netlink.h index d019bb31c6..e6cd7d9bf1 100644 --- a/src/shared/socket-netlink.h +++ b/src/shared/socket-netlink.h @@ -33,7 +33,6 @@ static inline int in_addr_ifindex_name_from_string_auto(const char *s, int *fami static inline int in_addr_ifindex_from_string_auto(const char *s, int *family, union in_addr_union *ret, int *ifindex) { return in_addr_ifindex_name_from_string_auto(s, family, ret, ifindex, NULL); } -int in_addr_port_from_string_auto(const char *s, int *ret_family, union in_addr_union *ret_address, uint16_t *ret_port); struct in_addr_full { int family; diff --git a/src/test/test-socket-netlink.c b/src/test/test-socket-netlink.c index 200af19a45..06a08cd9d7 100644 --- a/src/test/test-socket-netlink.c +++ b/src/test/test-socket-netlink.c @@ -365,36 +365,6 @@ static void test_in_addr_port_ifindex_name_from_string_auto(void) { test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53%19#hoge.com", AF_INET6, 53, 19, "hoge.com"); } -static void test_in_addr_port_from_string_auto_one(const char *str, int family, const char *address_string, uint16_t port) { - union in_addr_union a, b; - uint16_t p; - int f; - - assert_se(in_addr_port_from_string_auto(str, &f, &a, &p) >= 0); - assert_se(family == f); - assert_se(port == p); - assert_se(in_addr_from_string(family, address_string, &b) >= 0); - assert_se(in_addr_equal(family, &a, &b) == 1); -} - -static void test_in_addr_port_from_string_auto(void) { - log_info("/* %s */", __func__); - - assert_se(in_addr_port_from_string_auto("192.168.0.1#test.com", NULL, NULL, NULL) < 0); - assert_se(in_addr_port_from_string_auto("192.168.0.1:53#example.com", NULL, NULL, NULL) < 0); - assert_se(in_addr_port_from_string_auto("fe80::18#hoge.com", NULL, NULL, NULL) < 0); - assert_se(in_addr_port_from_string_auto("fe80::18%19", NULL, NULL, NULL) < 0); - assert_se(in_addr_port_from_string_auto("fe80::18%19#hoge.com", NULL, NULL, NULL) < 0); - assert_se(in_addr_port_from_string_auto("[fe80::18]:53#hoge.com", NULL, NULL, NULL) < 0); - assert_se(in_addr_port_from_string_auto("[fe80::18]:53%19", NULL, NULL, NULL) < 0); - assert_se(in_addr_port_from_string_auto("[fe80::18]:53%19#hoge.com", NULL, NULL, NULL) < 0); - - test_in_addr_port_from_string_auto_one("192.168.0.1", AF_INET, "192.168.0.1", 0); - test_in_addr_port_from_string_auto_one("192.168.0.1:53", AF_INET, "192.168.0.1", 53); - test_in_addr_port_from_string_auto_one("fe80::18", AF_INET6, "fe80::18", 0); - test_in_addr_port_from_string_auto_one("[fe80::18]:53", AF_INET6, "fe80::18", 53); -} - int main(int argc, char *argv[]) { test_setup_logging(LOG_DEBUG); @@ -409,7 +379,6 @@ int main(int argc, char *argv[]) { test_in_addr_ifindex_from_string_auto(); test_in_addr_ifindex_name_from_string_auto(); test_in_addr_port_ifindex_name_from_string_auto(); - test_in_addr_port_from_string_auto(); return 0; } From 75af1666d7476ee77f47bcc2e6c415c040326eb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Sep 2020 13:01:13 +0200 Subject: [PATCH 11/17] shared: make socket_address_parse use the generic parser for IPv[46] addresses One special syntax is not supported anymore: "iface:port" would be parsed as an interface name plus numerical port, equivalent to "[::]%iface:port". This was added in 542563babda, but was undocumented, and we had no tests for it. It seems that this actually wasn't doing anything useful, because the kernel only uses the scope identifier for link-local addresses. --- src/shared/socket-netlink.c | 105 ++++++++++++------------------------ 1 file changed, 33 insertions(+), 72 deletions(-) diff --git a/src/shared/socket-netlink.c b/src/shared/socket-netlink.c index 444aae271a..25bc0167a6 100644 --- a/src/shared/socket-netlink.c +++ b/src/shared/socket-netlink.c @@ -66,39 +66,8 @@ int socket_address_parse(SocketAddress *a, const char *s) { .type = SOCK_STREAM, }; - if (*s == '[') { - uint16_t port; - - /* IPv6 in [x:.....:z]:p notation */ - - e = strchr(s+1, ']'); - if (!e) - return -EINVAL; - - n = strndup(s+1, e-s-1); - if (!n) - return -ENOMEM; - - errno = 0; - if (inet_pton(AF_INET6, n, &a->sockaddr.in6.sin6_addr) <= 0) - return errno_or_else(EINVAL); - - e++; - if (*e != ':') - return -EINVAL; - - e++; - r = parse_ip_port(e, &port); - if (r < 0) - return r; - - a->sockaddr.in6.sin6_family = AF_INET6; - a->sockaddr.in6.sin6_port = htobe16(port); - a->size = sizeof(struct sockaddr_in6); - - } else if (*s == '/') { + if (*s == '/') { /* AF_UNIX socket */ - size_t l; l = strlen(s); @@ -158,47 +127,11 @@ int socket_address_parse(SocketAddress *a, const char *s) { } else { uint16_t port; - e = strchr(s, ':'); - if (e) { - r = parse_ip_port(e + 1, &port); - if (r < 0) - return r; - - n = strndup(s, e-s); - if (!n) - return -ENOMEM; - - /* IPv4 in w.x.y.z:p notation? */ - r = inet_pton(AF_INET, n, &a->sockaddr.in.sin_addr); - if (r < 0) - return -errno; - - if (r > 0) { - /* Gotcha, it's a traditional IPv4 address */ - a->sockaddr.in.sin_family = AF_INET; - a->sockaddr.in.sin_port = htobe16(port); - a->size = sizeof(struct sockaddr_in); - } else { - int idx; - - /* Uh, our last resort, an interface name */ - idx = resolve_ifname(NULL, n); - if (idx < 0) - return idx; - - a->sockaddr.in6.sin6_family = AF_INET6; - a->sockaddr.in6.sin6_port = htobe16(port); - a->sockaddr.in6.sin6_scope_id = idx; - a->sockaddr.in6.sin6_addr = in6addr_any; - a->size = sizeof(struct sockaddr_in6); - } - } else { - + r = parse_ip_port(s, &port); + if (r == -ERANGE) + return r; /* Valid port syntax, but the numerical value is wrong for a port. */ + if (r >= 0) { /* Just a port */ - r = parse_ip_port(s, &port); - if (r < 0) - return r; - if (socket_ipv6_is_supported()) { a->sockaddr.in6.sin6_family = AF_INET6; a->sockaddr.in6.sin6_port = htobe16(port); @@ -210,6 +143,34 @@ int socket_address_parse(SocketAddress *a, const char *s) { a->sockaddr.in.sin_addr.s_addr = INADDR_ANY; a->size = sizeof(struct sockaddr_in); } + + } else { + union in_addr_union address; + int family; + + r = in_addr_port_ifindex_name_from_string_auto(s, &family, &address, &port, NULL, NULL); + if (r < 0) + return r; + + if (port == 0) /* No port, no go. */ + return -EINVAL; + + if (family == AF_INET) { + a->sockaddr.in = (struct sockaddr_in) { + .sin_family = AF_INET, + .sin_addr = address.in, + .sin_port = htobe16(port), + }; + a->size = sizeof(struct sockaddr_in); + } else if (family == AF_INET6) { + a->sockaddr.in6 = (struct sockaddr_in6) { + .sin6_family = AF_INET6, + .sin6_addr = address.in6, + .sin6_port = htobe16(port), + }; + a->size = sizeof(struct sockaddr_in6); + } else + assert_not_reached("Family quarrel"); } } From 44ab234734907dbf08fae80a57dae01d9f0b2d0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Sep 2020 23:51:21 +0200 Subject: [PATCH 12/17] shared: don't unconditionally set SOCK_STREAM as type in socket_address_parse() We would set .type to a fake value. All real callers (outside of tests) immediately overwrite .type with a proper value after calling socket_address_parse(). So let's not set it and adjust the few places that relied on it being set to the fake value. socket_address_parse() is modernized to only set the output argument on success. --- src/basic/socket-util.c | 10 ++-- src/shared/socket-netlink.c | 103 +++++++++++++++++++-------------- src/test/test-socket-netlink.c | 9 ++- 3 files changed, 71 insertions(+), 51 deletions(-) diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index fa51997581..85edc83cae 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -68,7 +68,7 @@ int socket_address_verify(const SocketAddress *a, bool strict) { if (a->sockaddr.in.sin_port == 0) return -EINVAL; - if (!IN_SET(a->type, SOCK_STREAM, SOCK_DGRAM)) + if (!IN_SET(a->type, 0, SOCK_STREAM, SOCK_DGRAM)) return -EINVAL; return 0; @@ -80,7 +80,7 @@ int socket_address_verify(const SocketAddress *a, bool strict) { if (a->sockaddr.in6.sin6_port == 0) return -EINVAL; - if (!IN_SET(a->type, SOCK_STREAM, SOCK_DGRAM)) + if (!IN_SET(a->type, 0, SOCK_STREAM, SOCK_DGRAM)) return -EINVAL; return 0; @@ -114,7 +114,7 @@ int socket_address_verify(const SocketAddress *a, bool strict) { } } - if (!IN_SET(a->type, SOCK_STREAM, SOCK_DGRAM, SOCK_SEQPACKET)) + if (!IN_SET(a->type, 0, SOCK_STREAM, SOCK_DGRAM, SOCK_SEQPACKET)) return -EINVAL; return 0; @@ -124,7 +124,7 @@ int socket_address_verify(const SocketAddress *a, bool strict) { if (a->size != sizeof(struct sockaddr_nl)) return -EINVAL; - if (!IN_SET(a->type, SOCK_RAW, SOCK_DGRAM)) + if (!IN_SET(a->type, 0, SOCK_RAW, SOCK_DGRAM)) return -EINVAL; return 0; @@ -133,7 +133,7 @@ int socket_address_verify(const SocketAddress *a, bool strict) { if (a->size != sizeof(struct sockaddr_vm)) return -EINVAL; - if (!IN_SET(a->type, SOCK_STREAM, SOCK_DGRAM)) + if (!IN_SET(a->type, 0, SOCK_STREAM, SOCK_DGRAM)) return -EINVAL; return 0; diff --git a/src/shared/socket-netlink.c b/src/shared/socket-netlink.c index 25bc0167a6..198892b007 100644 --- a/src/shared/socket-netlink.c +++ b/src/shared/socket-netlink.c @@ -62,29 +62,25 @@ int socket_address_parse(SocketAddress *a, const char *s) { assert(a); assert(s); - *a = (SocketAddress) { - .type = SOCK_STREAM, - }; - if (*s == '/') { /* AF_UNIX socket */ - size_t l; - l = strlen(s); + size_t l = strlen(s); if (l >= sizeof(a->sockaddr.un.sun_path)) /* Note that we refuse non-NUL-terminated sockets when * parsing (the kernel itself is less strict here in what it * accepts) */ return -EINVAL; - a->sockaddr.un.sun_family = AF_UNIX; + *a = (SocketAddress) { + .sockaddr.un.sun_family = AF_UNIX, + .size = offsetof(struct sockaddr_un, sun_path) + l + 1, + }; memcpy(a->sockaddr.un.sun_path, s, l); - a->size = offsetof(struct sockaddr_un, sun_path) + l + 1; } else if (*s == '@') { /* Abstract AF_UNIX socket */ - size_t l; - l = strlen(s+1); + size_t l = strlen(s+1); if (l >= sizeof(a->sockaddr.un.sun_path) - 1) /* Note that we refuse non-NUL-terminated sockets here * when parsing, even though abstract namespace sockets * explicitly allow embedded NUL bytes and don't consider @@ -92,14 +88,16 @@ int socket_address_parse(SocketAddress *a, const char *s) { * sockets. */ return -EINVAL; - a->sockaddr.un.sun_family = AF_UNIX; + *a = (SocketAddress) { + .sockaddr.un.sun_family = AF_UNIX, + .size = offsetof(struct sockaddr_un, sun_path) + 1 + l, + }; memcpy(a->sockaddr.un.sun_path+1, s+1, l); - a->size = offsetof(struct sockaddr_un, sun_path) + 1 + l; } else if (startswith(s, "vsock:")) { /* AF_VSOCK socket in vsock:cid:port notation */ const char *cid_start = s + STRLEN("vsock:"); - unsigned port; + unsigned port, cid; e = strchr(cid_start, ':'); if (!e) @@ -113,16 +111,22 @@ int socket_address_parse(SocketAddress *a, const char *s) { if (!n) return -ENOMEM; - if (!isempty(n)) { - r = safe_atou(n, &a->sockaddr.vm.svm_cid); + if (isempty(n)) + cid = VMADDR_CID_ANY; + else { + r = safe_atou(n, &cid); if (r < 0) return r; - } else - a->sockaddr.vm.svm_cid = VMADDR_CID_ANY; + } - a->sockaddr.vm.svm_family = AF_VSOCK; - a->sockaddr.vm.svm_port = port; - a->size = sizeof(struct sockaddr_vm); + *a = (SocketAddress) { + .sockaddr.vm = { + .svm_cid = cid, + .svm_family = AF_VSOCK, + .svm_port = port, + }, + .size = sizeof(struct sockaddr_vm), + }; } else { uint16_t port; @@ -132,17 +136,24 @@ int socket_address_parse(SocketAddress *a, const char *s) { return r; /* Valid port syntax, but the numerical value is wrong for a port. */ if (r >= 0) { /* Just a port */ - if (socket_ipv6_is_supported()) { - a->sockaddr.in6.sin6_family = AF_INET6; - a->sockaddr.in6.sin6_port = htobe16(port); - a->sockaddr.in6.sin6_addr = in6addr_any; - a->size = sizeof(struct sockaddr_in6); - } else { - a->sockaddr.in.sin_family = AF_INET; - a->sockaddr.in.sin_port = htobe16(port); - a->sockaddr.in.sin_addr.s_addr = INADDR_ANY; - a->size = sizeof(struct sockaddr_in); - } + if (socket_ipv6_is_supported()) + *a = (SocketAddress) { + .sockaddr.in6 = { + .sin6_family = AF_INET6, + .sin6_port = htobe16(port), + .sin6_addr = in6addr_any, + }, + .size = sizeof(struct sockaddr_in6), + }; + else + *a = (SocketAddress) { + .sockaddr.in = { + .sin_family = AF_INET, + .sin_port = htobe16(port), + .sin_addr.s_addr = INADDR_ANY, + }, + .size = sizeof(struct sockaddr_in), + }; } else { union in_addr_union address; @@ -155,21 +166,25 @@ int socket_address_parse(SocketAddress *a, const char *s) { if (port == 0) /* No port, no go. */ return -EINVAL; - if (family == AF_INET) { - a->sockaddr.in = (struct sockaddr_in) { - .sin_family = AF_INET, - .sin_addr = address.in, - .sin_port = htobe16(port), + if (family == AF_INET) + *a = (SocketAddress) { + .sockaddr.in = { + .sin_family = AF_INET, + .sin_addr = address.in, + .sin_port = htobe16(port), + }, + .size = sizeof(struct sockaddr_in), }; - a->size = sizeof(struct sockaddr_in); - } else if (family == AF_INET6) { - a->sockaddr.in6 = (struct sockaddr_in6) { - .sin6_family = AF_INET6, - .sin6_addr = address.in6, - .sin6_port = htobe16(port), + else if (family == AF_INET6) + *a = (SocketAddress) { + .sockaddr.in6 = { + .sin6_family = AF_INET6, + .sin6_addr = address.in6, + .sin6_port = htobe16(port), + }, + .size = sizeof(struct sockaddr_in6), }; - a->size = sizeof(struct sockaddr_in6); - } else + else assert_not_reached("Family quarrel"); } } diff --git a/src/test/test-socket-netlink.c b/src/test/test-socket-netlink.c index 06a08cd9d7..b87cb7b126 100644 --- a/src/test/test-socket-netlink.c +++ b/src/test/test-socket-netlink.c @@ -17,6 +17,7 @@ static void test_socket_address_parse_one(const char *in, int ret, int family, c if (r < 0) log_error_errno(r, "Printing failed for \"%s\": %m", in); assert(r >= 0); + assert_se(a.type == 0); } log_info("\"%s\" → %s %d → \"%s\" (expect %d / \"%s\")", @@ -206,9 +207,13 @@ static void test_socket_address_is(void) { log_info("/* %s */", __func__); assert_se(socket_address_parse(&a, "192.168.1.1:8888") >= 0); - assert_se(socket_address_is(&a, "192.168.1.1:8888", SOCK_STREAM)); + assert_se( socket_address_is(&a, "192.168.1.1:8888", 0 /* unspecified yet */)); + assert_se(!socket_address_is(&a, "route", 0)); assert_se(!socket_address_is(&a, "route", SOCK_STREAM)); assert_se(!socket_address_is(&a, "192.168.1.1:8888", SOCK_RAW)); + assert_se(!socket_address_is(&a, "192.168.1.1:8888", SOCK_STREAM)); + a.type = SOCK_STREAM; + assert_se( socket_address_is(&a, "192.168.1.1:8888", SOCK_STREAM)); } static void test_socket_address_is_netlink(void) { @@ -217,7 +222,7 @@ static void test_socket_address_is_netlink(void) { log_info("/* %s */", __func__); assert_se(socket_address_parse_netlink(&a, "route 10") >= 0); - assert_se(socket_address_is_netlink(&a, "route 10")); + assert_se( socket_address_is_netlink(&a, "route 10")); assert_se(!socket_address_is_netlink(&a, "192.168.1.1:8888")); assert_se(!socket_address_is_netlink(&a, "route 1")); } From c4c6ee3a959f0f082f232790677709e01b862fa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 5 Sep 2020 21:14:36 +0200 Subject: [PATCH 13/17] resolved: drop duplicated check The same conditional appears a few lines down. --- src/resolve/resolved-conf.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/src/resolve/resolved-conf.c b/src/resolve/resolved-conf.c index 08655e115d..2d85cc4ecd 100644 --- a/src/resolve/resolved-conf.c +++ b/src/resolve/resolved-conf.c @@ -35,9 +35,6 @@ static int manager_add_dns_server_by_string(Manager *m, DnsServerType type, cons if (r < 0) return r; - if (IN_SET(port, 53, 853)) - port = 0; - /* Silently filter out 0.0.0.0 and 127.0.0.53 (our own stub DNS listener) */ if (!dns_server_address_valid(family, &address)) return 0; @@ -50,12 +47,8 @@ static int manager_add_dns_server_by_string(Manager *m, DnsServerType type, cons /* Filter out duplicates */ s = dns_server_find(manager_get_first_dns_server(m, type), family, &address, port, ifindex, server_name); if (s) { - /* - * Drop the marker. This is used to find the servers - * that ceased to exist, see - * manager_mark_dns_servers() and - * manager_flush_marked_dns_servers(). - */ + /* Drop the marker. This is used to find the servers that ceased to exist, see + * manager_mark_dns_servers() and manager_flush_marked_dns_servers(). */ dns_server_move_back_and_unmark(s); return 0; } From a07ab56a4975f1159fd466530f150bfd2230f83c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Sep 2020 15:33:25 +0200 Subject: [PATCH 14/17] Allow interface scopes to be specified in ListenStream= Closes #12624. The formatting in systemd.socket.xml is updated a bit. Currently in_addr_port_ifindex_name_to_string() always prints the ifindex numerically. This is not super useful since the interface numbers are semi-random. Should we use interface names in preference? --- man/systemd.socket.xml | 30 ++++++++++---------- src/shared/socket-netlink.c | 5 ++-- src/test/test-socket-netlink.c | 50 ++++++++++++++++++++++------------ 3 files changed, 52 insertions(+), 33 deletions(-) diff --git a/man/systemd.socket.xml b/man/systemd.socket.xml index 1bcbef2033..253da8a6d4 100644 --- a/man/systemd.socket.xml +++ b/man/systemd.socket.xml @@ -200,22 +200,24 @@ If the address string is a string in the format - v.w.x.y:z, it is read as IPv4 specifier for listening on an - address v.w.x.y on a port z. - - If the address string is a string in the format [x]:y, - it is read as IPv6 address x on a port y. Note that this might - make the service available via IPv4, too, depending on the - BindIPv6Only= setting (see below). - + v.w.x.y:z, it is interpeted + as IPv4 address v.w.x.y and port z. If the address string is a string in the format - vsock:x:y, it is read as CID x on - a port y address in the - AF_VSOCK family. The CID is a unique 32-bit - integer identifier in AF_VSOCK analogous to an IP - address. Specifying the CID is optional, and may be set to the empty - string. + [x]:y, it is interpreted as + IPv6 address x and port y. An optional + interface scope (interface name or number) may be specifed after a % symbol: + [x]:y%dev. + Interface scopes are only useful with link-local addresses, because the kernel ignores them in other + cases. Note that if an address is specified as IPv6, it might still make the service available via + IPv4 too, depending on the BindIPv6Only= setting (see below). + + If the address string is a string in the format + vsock:x:y, it is read as CID + x on a port y address in the + AF_VSOCK family. The CID is a unique 32-bit integer identifier in + AF_VSOCK analogous to an IP address. Specifying the CID is optional, and may be + set to the empty string. Note that SOCK_SEQPACKET (i.e. ListenSequentialPacket=) is only available diff --git a/src/shared/socket-netlink.c b/src/shared/socket-netlink.c index 198892b007..0ecbf7dee0 100644 --- a/src/shared/socket-netlink.c +++ b/src/shared/socket-netlink.c @@ -157,9 +157,9 @@ int socket_address_parse(SocketAddress *a, const char *s) { } else { union in_addr_union address; - int family; + int family, ifindex; - r = in_addr_port_ifindex_name_from_string_auto(s, &family, &address, &port, NULL, NULL); + r = in_addr_port_ifindex_name_from_string_auto(s, &family, &address, &port, &ifindex, NULL); if (r < 0) return r; @@ -181,6 +181,7 @@ int socket_address_parse(SocketAddress *a, const char *s) { .sin6_family = AF_INET6, .sin6_addr = address.in6, .sin6_port = htobe16(port), + .sin6_scope_id = ifindex, }, .size = sizeof(struct sockaddr_in6), }; diff --git a/src/test/test-socket-netlink.c b/src/test/test-socket-netlink.c index b87cb7b126..ac191851fa 100644 --- a/src/test/test-socket-netlink.c +++ b/src/test/test-socket-netlink.c @@ -59,11 +59,21 @@ static void test_socket_address_parse(void) { test_socket_address_parse_one("[::1]:0", -EINVAL, 0, NULL); test_socket_address_parse_one("[::1]:65536", -ERANGE, 0, NULL); test_socket_address_parse_one("[a:b:1]:8888", -EINVAL, 0, NULL); + test_socket_address_parse_one("[::1]%lo:1234", -EINVAL, 0, NULL); + test_socket_address_parse_one("[::1]%lo:0", -EINVAL, 0, NULL); + test_socket_address_parse_one("[::1]%lo", -EINVAL, 0, NULL); + test_socket_address_parse_one("[::1]%lo%lo:1234", -EINVAL, 0, NULL); + test_socket_address_parse_one("[::1]% lo:1234", -EINVAL, 0, NULL); test_socket_address_parse_one("8888", 0, default_family, "[::]:8888"); test_socket_address_parse_one("[2001:0db8:0000:85a3:0000:0000:ac1f:8001]:8888", 0, AF_INET6, "[2001:db8:0:85a3::ac1f:8001]:8888"); test_socket_address_parse_one("[::1]:8888", 0, AF_INET6, NULL); + test_socket_address_parse_one("[::1]:1234%lo", 0, AF_INET6, NULL); + test_socket_address_parse_one("[::1]:0%lo", -EINVAL, 0, NULL); + test_socket_address_parse_one("[::1]%lo", -EINVAL, 0, NULL); + test_socket_address_parse_one("[::1]:1234%lo%lo", -ENODEV, 0, NULL); + test_socket_address_parse_one("[::1]:1234%xxxxasdf", -ENODEV, 0, NULL); test_socket_address_parse_one("192.168.1.254:8888", 0, AF_INET, NULL); test_socket_address_parse_one("/foo/bar", 0, AF_UNIX, NULL); test_socket_address_parse_one("/", 0, AF_UNIX, NULL); @@ -297,7 +307,8 @@ static void test_in_addr_ifindex_name_from_string_auto(void) { test_in_addr_ifindex_name_from_string_auto_one("fe80::18%19#another.test.com", "another.test.com"); } -static void test_in_addr_port_ifindex_name_from_string_auto_one(const char *str, int family, uint16_t port, int ifindex, const char *server_name) { +static void test_in_addr_port_ifindex_name_from_string_auto_one(const char *str, int family, uint16_t port, int ifindex, + const char *server_name, const char *str_repr) { union in_addr_union a; uint16_t p; int f, i; @@ -313,7 +324,7 @@ static void test_in_addr_port_ifindex_name_from_string_auto_one(const char *str, assert_se(ifindex == i); assert_se(streq_ptr(server_name, name)); assert_se(in_addr_port_ifindex_name_to_string(f, &a, p, i, name, &x) >= 0); - assert_se(streq(str, x)); + assert_se(streq(str_repr ?: str, x)); } if (port > 0) @@ -325,7 +336,7 @@ static void test_in_addr_port_ifindex_name_from_string_auto_one(const char *str, assert_se(ifindex == i); assert_se(streq_ptr(server_name, name)); assert_se(in_addr_port_ifindex_name_to_string(f, &a, 0, i, name, &x) >= 0); - assert_se(streq(str, x)); + assert_se(streq(str_repr ?: str, x)); } if (ifindex > 0) @@ -337,7 +348,7 @@ static void test_in_addr_port_ifindex_name_from_string_auto_one(const char *str, assert_se(port == p); assert_se(streq_ptr(server_name, name)); assert_se(in_addr_port_ifindex_name_to_string(f, &a, p, 0, name, &x) >= 0); - assert_se(streq(str, x)); + assert_se(streq(str_repr ?: str, x)); } if (server_name) @@ -349,25 +360,30 @@ static void test_in_addr_port_ifindex_name_from_string_auto_one(const char *str, assert_se(port == p); assert_se(ifindex == i); assert_se(in_addr_port_ifindex_name_to_string(f, &a, p, i, NULL, &x) >= 0); - assert_se(streq(str, x)); + assert_se(streq(str_repr ?: str, x)); } } static void test_in_addr_port_ifindex_name_from_string_auto(void) { log_info("/* %s */", __func__); - test_in_addr_port_ifindex_name_from_string_auto_one("192.168.0.1", AF_INET, 0, 0, NULL); - test_in_addr_port_ifindex_name_from_string_auto_one("192.168.0.1#test.com", AF_INET, 0, 0, "test.com"); - test_in_addr_port_ifindex_name_from_string_auto_one("192.168.0.1:53", AF_INET, 53, 0, NULL); - test_in_addr_port_ifindex_name_from_string_auto_one("192.168.0.1:53#example.com", AF_INET, 53, 0, "example.com"); - test_in_addr_port_ifindex_name_from_string_auto_one("fe80::18", AF_INET6, 0, 0, NULL); - test_in_addr_port_ifindex_name_from_string_auto_one("fe80::18#hoge.com", AF_INET6, 0, 0, "hoge.com"); - test_in_addr_port_ifindex_name_from_string_auto_one("fe80::18%19", AF_INET6, 0, 19, NULL); - test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53", AF_INET6, 53, 0, NULL); - test_in_addr_port_ifindex_name_from_string_auto_one("fe80::18%19#hoge.com", AF_INET6, 0, 19, "hoge.com"); - test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53#hoge.com", AF_INET6, 53, 0, "hoge.com"); - test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53%19", AF_INET6, 53, 19, NULL); - test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53%19#hoge.com", AF_INET6, 53, 19, "hoge.com"); + test_in_addr_port_ifindex_name_from_string_auto_one("192.168.0.1", AF_INET, 0, 0, NULL, NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("192.168.0.1#test.com", AF_INET, 0, 0, "test.com", NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("192.168.0.1:53", AF_INET, 53, 0, NULL, NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("192.168.0.1:53#example.com", AF_INET, 53, 0, "example.com", NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("fe80::18", AF_INET6, 0, 0, NULL, NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("fe80::18#hoge.com", AF_INET6, 0, 0, "hoge.com", NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("fe80::18%19", AF_INET6, 0, 19, NULL, NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("fe80::18%lo", AF_INET6, 0, 1, NULL, "fe80::18%1"); + test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53", AF_INET6, 53, 0, NULL, NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53%19", AF_INET6, 53, 19, NULL, NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53%lo", AF_INET6, 53, 1, NULL, "[fe80::18]:53%1"); + test_in_addr_port_ifindex_name_from_string_auto_one("fe80::18%19#hoge.com", AF_INET6, 0, 19, "hoge.com", NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53#hoge.com", AF_INET6, 53, 0, "hoge.com", NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53%19", AF_INET6, 53, 19, NULL, NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53%19#hoge.com", AF_INET6, 53, 19, "hoge.com", NULL); + test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53%lo", AF_INET6, 53, 1, NULL, "[fe80::18]:53%1"); + test_in_addr_port_ifindex_name_from_string_auto_one("[fe80::18]:53%lo#hoge.com", AF_INET6, 53, 1, "hoge.com", "[fe80::18]:53%1#hoge.com"); } int main(int argc, char *argv[]) { From a73569f180dbb8690ae3b350b0609ac0f9705c55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 5 Sep 2020 21:25:11 +0200 Subject: [PATCH 15/17] shared/socket-netlink: set output in socket_address_parse_netlink() only on success --- src/shared/socket-netlink.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/shared/socket-netlink.c b/src/shared/socket-netlink.c index 0ecbf7dee0..76b68dfa1c 100644 --- a/src/shared/socket-netlink.c +++ b/src/shared/socket-netlink.c @@ -220,10 +220,6 @@ int socket_address_parse_netlink(SocketAddress *a, const char *s) { assert(a); assert(s); - *a = (SocketAddress) { - .type = SOCK_RAW, - }; - r = extract_first_word(&s, &word, NULL, 0); if (r < 0) return r; @@ -240,12 +236,13 @@ int socket_address_parse_netlink(SocketAddress *a, const char *s) { return r; } - a->sockaddr.nl.nl_family = AF_NETLINK; - a->sockaddr.nl.nl_groups = group; - - a->type = SOCK_RAW; - a->size = sizeof(struct sockaddr_nl); - a->protocol = family; + *a = (SocketAddress) { + .type = SOCK_RAW, + .sockaddr.nl.nl_family = AF_NETLINK, + .sockaddr.nl.nl_groups = group, + .protocol = family, + .size = sizeof(struct sockaddr_nl), + }; return 0; } From 08224f38acdf5e391dda43e4e004e4ddc5c00302 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 9 Sep 2020 23:49:51 +0200 Subject: [PATCH 16/17] Use sockaddr_un_set_path() in socket_address_parse() Two functional changes: - "/" is now refused. The test is adjusted. - The trailing NUL is *not* included in the returned size for abstract size. The comments in sockaddr_un_set_path() indicate that this is the right thing to do, and the code in socket_address_parse() wasn't doing that. --- src/shared/socket-netlink.c | 33 +++++++-------------------------- src/test/test-socket-netlink.c | 2 +- 2 files changed, 8 insertions(+), 27 deletions(-) diff --git a/src/shared/socket-netlink.c b/src/shared/socket-netlink.c index 76b68dfa1c..23970dbe55 100644 --- a/src/shared/socket-netlink.c +++ b/src/shared/socket-netlink.c @@ -62,37 +62,18 @@ int socket_address_parse(SocketAddress *a, const char *s) { assert(a); assert(s); - if (*s == '/') { + if (IN_SET(*s, '/', '@')) { /* AF_UNIX socket */ + struct sockaddr_un un; - size_t l = strlen(s); - if (l >= sizeof(a->sockaddr.un.sun_path)) /* Note that we refuse non-NUL-terminated sockets when - * parsing (the kernel itself is less strict here in what it - * accepts) */ - return -EINVAL; + r = sockaddr_un_set_path(&un, s); + if (r < 0) + return r; *a = (SocketAddress) { - .sockaddr.un.sun_family = AF_UNIX, - .size = offsetof(struct sockaddr_un, sun_path) + l + 1, + .sockaddr.un = un, + .size = r, }; - memcpy(a->sockaddr.un.sun_path, s, l); - - } else if (*s == '@') { - /* Abstract AF_UNIX socket */ - - size_t l = strlen(s+1); - if (l >= sizeof(a->sockaddr.un.sun_path) - 1) /* Note that we refuse non-NUL-terminated sockets here - * when parsing, even though abstract namespace sockets - * explicitly allow embedded NUL bytes and don't consider - * them special. But it's simply annoying to debug such - * sockets. */ - return -EINVAL; - - *a = (SocketAddress) { - .sockaddr.un.sun_family = AF_UNIX, - .size = offsetof(struct sockaddr_un, sun_path) + 1 + l, - }; - memcpy(a->sockaddr.un.sun_path+1, s+1, l); } else if (startswith(s, "vsock:")) { /* AF_VSOCK socket in vsock:cid:port notation */ diff --git a/src/test/test-socket-netlink.c b/src/test/test-socket-netlink.c index ac191851fa..afb7038621 100644 --- a/src/test/test-socket-netlink.c +++ b/src/test/test-socket-netlink.c @@ -76,7 +76,7 @@ static void test_socket_address_parse(void) { test_socket_address_parse_one("[::1]:1234%xxxxasdf", -ENODEV, 0, NULL); test_socket_address_parse_one("192.168.1.254:8888", 0, AF_INET, NULL); test_socket_address_parse_one("/foo/bar", 0, AF_UNIX, NULL); - test_socket_address_parse_one("/", 0, AF_UNIX, NULL); + test_socket_address_parse_one("/", -EINVAL, 0, NULL); test_socket_address_parse_one("@abstract", 0, AF_UNIX, NULL); { From c097bf1f50af525fd5cfb0ae7ee9d62fbcb1ca58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 9 Sep 2020 23:57:59 +0200 Subject: [PATCH 17/17] Minor simplification in sockaddr_un_set_path() --- src/basic/socket-util.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 85edc83cae..7a3299672a 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -1118,12 +1118,10 @@ int sockaddr_un_set_path(struct sockaddr_un *ret, const char *path) { * reference paths in the abstract namespace that include NUL bytes in the name. */ l = strlen(path); - if (l == 0) + if (l < 2) return -EINVAL; if (!IN_SET(path[0], '/', '@')) return -EINVAL; - if (path[1] == 0) - return -EINVAL; /* Don't allow paths larger than the space in sockaddr_un. Note that we are a tiny bit more restrictive than * the kernel is: we insist on NUL termination (both for abstract namespace and regular file system socket