From 6978efcffb1aaab67444afb9f0369582ec88996a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 May 2018 08:50:00 +0200 Subject: [PATCH 1/8] core/mount-setup: remove part of check which is always true f1470e424b2b5337e3c383d68dc5a26af1ff4ce6 removed one check, but missed a similar one a few lines down. CID #1390949. --- src/core/mount-setup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/mount-setup.c b/src/core/mount-setup.c index 79300332a8..e9d69728fa 100644 --- a/src/core/mount-setup.c +++ b/src/core/mount-setup.c @@ -317,7 +317,7 @@ int mount_cgroup_controllers(char ***join_controllers) { if (r < 0) return r; - if (r > 0 && k && *k) { + if (r > 0 && *k) { char **i; for (i = *k; *i; i++) { From dcd6361ea1dfaa6810b480e0bc53674e5b2c822d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 May 2018 09:12:12 +0200 Subject: [PATCH 2/8] tree-wide: do not wrap assert_se in extra parentheses We were inconsitently using them in some cases, but in majority not. Using assignment in assert_se is very common, not an exception like in 'if', so let's drop the extra parens everywhere. --- src/basic/path-util.c | 2 +- src/libsystemd/sd-bus/test-bus-server.c | 3 ++- src/test/test-copy.c | 12 ++++++------ src/test/test-dlopen.c | 2 +- src/test/test-fileio.c | 2 +- src/test/test-hexdecoct.c | 4 ++-- src/test/test-nss.c | 2 +- src/test/test-unit-file.c | 4 ++-- 8 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/basic/path-util.c b/src/basic/path-util.c index 2ecb143014..92897239cc 100644 --- a/src/basic/path-util.c +++ b/src/basic/path-util.c @@ -918,7 +918,7 @@ int systemd_installation_has_version(const char *root, unsigned minimal_version) if (r < 0) return r; - assert_se((c = endswith(path, "*.so"))); + assert_se(c = endswith(path, "*.so")); *c = '\0'; /* truncate the glob part */ STRV_FOREACH(name, names) { diff --git a/src/libsystemd/sd-bus/test-bus-server.c b/src/libsystemd/sd-bus/test-bus-server.c index f862dc06e8..373d596437 100644 --- a/src/libsystemd/sd-bus/test-bus-server.c +++ b/src/libsystemd/sd-bus/test-bus-server.c @@ -68,7 +68,8 @@ static void *server(void *p) { if (sd_bus_message_is_method_call(m, "org.freedesktop.systemd.test", "Exit")) { - assert_se((sd_bus_can_send(bus, 'h') >= 1) == (c->server_negotiate_unix_fds && c->client_negotiate_unix_fds)); + assert_se((sd_bus_can_send(bus, 'h') >= 1) == + (c->server_negotiate_unix_fds && c->client_negotiate_unix_fds)); r = sd_bus_message_new_method_return(m, &reply); if (r < 0) { diff --git a/src/test/test-copy.c b/src/test/test-copy.c index 9d6a6540c6..6b5d1e152d 100644 --- a/src/test/test-copy.c +++ b/src/test/test-copy.c @@ -95,7 +95,7 @@ static void test_copy_tree(void) { STRV_FOREACH(p, files) { _cleanup_free_ char *f; - assert_se((f = strappend(original_dir, *p))); + assert_se(f = strappend(original_dir, *p)); assert_se(mkdir_parents(f, 0755) >= 0); assert_se(write_string_file(f, "file", WRITE_STRING_FILE_CREATE) == 0); @@ -104,8 +104,8 @@ static void test_copy_tree(void) { STRV_FOREACH_PAIR(link, p, links) { _cleanup_free_ char *f, *l; - assert_se((f = strappend(original_dir, *p))); - assert_se((l = strappend(original_dir, *link))); + assert_se(f = strappend(original_dir, *p)); + assert_se(l = strappend(original_dir, *link)); assert_se(mkdir_parents(l, 0755) >= 0); assert_se(symlink(f, l) == 0); @@ -120,7 +120,7 @@ static void test_copy_tree(void) { _cleanup_free_ char *buf = NULL, *f; size_t sz = 0; - assert_se((f = strappend(copy_dir, *p))); + assert_se(f = strappend(copy_dir, *p)); assert_se(access(f, F_OK) == 0); assert_se(read_full_file(f, &buf, &sz) == 0); @@ -130,8 +130,8 @@ static void test_copy_tree(void) { STRV_FOREACH_PAIR(link, p, links) { _cleanup_free_ char *target = NULL, *f, *l; - assert_se((f = strjoin(original_dir, *p))); - assert_se((l = strjoin(copy_dir, *link))); + assert_se(f = strjoin(original_dir, *p)); + assert_se(l = strjoin(copy_dir, *link)); assert_se(chase_symlinks(l, NULL, 0, &target) == 1); assert_se(path_equal(f, target)); diff --git a/src/test/test-dlopen.c b/src/test/test-dlopen.c index 151d2d72b2..febfc60e00 100644 --- a/src/test/test-dlopen.c +++ b/src/test/test-dlopen.c @@ -13,7 +13,7 @@ int main(int argc, char **argv) { void *handle; - assert_se((handle = dlopen(argv[1], RTLD_NOW))); + assert_se(handle = dlopen(argv[1], RTLD_NOW)); assert_se(dlclose(handle) == 0); return EXIT_SUCCESS; diff --git a/src/test/test-fileio.c b/src/test/test-fileio.c index ba22e865fd..2ae954c2f2 100644 --- a/src/test/test-fileio.c +++ b/src/test/test-fileio.c @@ -467,7 +467,7 @@ static void test_write_string_file_verify(void) { int r; assert_se(read_one_line_file("/proc/cmdline", &buf) >= 0); - assert_se((buf2 = strjoin(buf, "\n"))); + assert_se(buf2 = strjoin(buf, "\n")); r = write_string_file("/proc/cmdline", buf, 0); assert_se(IN_SET(r, -EACCES, -EIO)); diff --git a/src/test/test-hexdecoct.c b/src/test/test-hexdecoct.c index fb98826c27..41ca9805fe 100644 --- a/src/test/test-hexdecoct.c +++ b/src/test/test-hexdecoct.c @@ -85,10 +85,10 @@ static void test_unhexmem_one(const char *s, size_t l, int retval) { if (retval == 0) { char *answer; - if (l == (size_t) - 1) + if (l == (size_t) -1) l = strlen(s); - assert_se((hex = hexmem(mem, len))); + assert_se(hex = hexmem(mem, len)); answer = strndupa(s, l); assert_se(streq(delete_chars(answer, WHITESPACE), hex)); } diff --git a/src/test/test-nss.c b/src/test/test-nss.c index f6dee45ac0..6d4c51d6d8 100644 --- a/src/test/test-nss.c +++ b/src/test/test-nss.c @@ -87,7 +87,7 @@ static int print_gaih_addrtuples(const struct gaih_addrtuple *tuples) { r = in_addr_to_string(it->family, &u, &a); assert_se(IN_SET(r, 0, -EAFNOSUPPORT)); if (r == -EAFNOSUPPORT) - assert_se((a = hexmem(it->addr, 16))); + assert_se(a = hexmem(it->addr, 16)); if (it->scopeid == 0) goto numerical_index; diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index 3fd4ac9e5d..13db2f4225 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -636,8 +636,8 @@ static void test_install_printf(void) { assert_se(specifier_machine_id('m', NULL, NULL, &mid) >= 0 && mid); assert_se(specifier_boot_id('b', NULL, NULL, &bid) >= 0 && bid); - assert_se((host = gethostname_malloc())); - assert_se((user = uid_to_name(getuid()))); + assert_se(host = gethostname_malloc()); + assert_se(user = uid_to_name(getuid())); assert_se(asprintf(&uid, UID_FMT, getuid()) >= 0); #define expect(src, pattern, result) \ From 9421cab6db41498f476c5fa2f1c745b6cf15ec97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 May 2018 09:20:17 +0200 Subject: [PATCH 3/8] test-copy: remove unnecessary initialization Initializing just some of the values doesn't make sense. We should handle them all in the same way. In those tests there are no jumps, all steps are covered with assert_se(), so we know everything will be set if we get to the end of the function. And _not_ initializing those variables has the advantage that it allows valgrind to catch potential initalization errors in the function being tested. --- src/test/test-copy.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/test-copy.c b/src/test/test-copy.c index 6b5d1e152d..a02746c76d 100644 --- a/src/test/test-copy.c +++ b/src/test/test-copy.c @@ -117,8 +117,8 @@ static void test_copy_tree(void) { assert_se(copy_tree(original_dir, copy_dir, UID_INVALID, GID_INVALID, COPY_REFLINK|COPY_MERGE) == 0); STRV_FOREACH(p, files) { - _cleanup_free_ char *buf = NULL, *f; - size_t sz = 0; + _cleanup_free_ char *buf, *f; + size_t sz; assert_se(f = strappend(copy_dir, *p)); @@ -128,7 +128,7 @@ static void test_copy_tree(void) { } STRV_FOREACH_PAIR(link, p, links) { - _cleanup_free_ char *target = NULL, *f, *l; + _cleanup_free_ char *target, *f, *l; assert_se(f = strjoin(original_dir, *p)); assert_se(l = strjoin(copy_dir, *link)); From 0b45ff5278f0aa5770f661969fcb850e12f9ecf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 May 2018 09:50:58 +0200 Subject: [PATCH 4/8] sd-resolve: add helper macro for setting of ret/_errno/_h_errno Just to make things a bit shorter. This fixes an error where errno==-EIO would be set (negative value), introduced in 1a96c8e1ccb06f87b6bfaff4639390ecd00af588. --- src/libsystemd/sd-resolve/sd-resolve.c | 40 +++++++++----------------- 1 file changed, 14 insertions(+), 26 deletions(-) diff --git a/src/libsystemd/sd-resolve/sd-resolve.c b/src/libsystemd/sd-resolve/sd-resolve.c index 4877206a2c..9fabfcec6a 100644 --- a/src/libsystemd/sd-resolve/sd-resolve.c +++ b/src/libsystemd/sd-resolve/sd-resolve.c @@ -168,8 +168,10 @@ static void resolve_query_disconnect(sd_resolve_query *q); #define RESOLVE_DONT_DESTROY(resolve) \ _cleanup_(sd_resolve_unrefp) _unused_ sd_resolve *_dont_destroy_##resolve = sd_resolve_ref(resolve) -static int send_died(int out_fd) { +#define ASSIGN_ERRNO(q, val, error, h_error) \ + ({ (q)->ret = (val); (q)->_errno = (error); (q)->_h_errno = (h_error); }) +static int send_died(int out_fd) { RHeader rh = { .type = RESPONSE_DIED, .length = sizeof(RHeader), @@ -781,9 +783,7 @@ static int handle_response(sd_resolve *resolve, const Packet *packet, size_t len assert(length >= sizeof(AddrInfoResponse)); assert(q->type == REQUEST_ADDRINFO); - q->ret = ai_resp->ret; - q->_errno = ai_resp->_errno; - q->_h_errno = ai_resp->_h_errno; + ASSIGN_ERRNO(q, ai_resp->ret, ai_resp->_errno, ai_resp->_h_errno); l = length - sizeof(AddrInfoResponse); p = (const uint8_t*) resp + sizeof(AddrInfoResponse); @@ -793,9 +793,7 @@ static int handle_response(sd_resolve *resolve, const Packet *packet, size_t len r = unserialize_addrinfo(&p, &l, &ai); if (r < 0) { - q->ret = EAI_SYSTEM; - q->_errno = -r; - q->_h_errno = 0; + ASSIGN_ERRNO(q, EAI_SYSTEM, -r, 0); freeaddrinfo(q->addrinfo); q->addrinfo = NULL; break; @@ -820,34 +818,24 @@ static int handle_response(sd_resolve *resolve, const Packet *packet, size_t len 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; + sizeof(NameInfoResponse) + ni_resp->hostlen + ni_resp->servlen > length + 2) + ASSIGN_ERRNO(q, EAI_SYSTEM, EIO, 0); - } else { - q->ret = ni_resp->ret; - q->_errno = ni_resp->_errno; - q->_h_errno = ni_resp->_h_errno; + else { + ASSIGN_ERRNO(q, ni_resp->ret, ni_resp->_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 (!q->host) + ASSIGN_ERRNO(q, EAI_MEMORY, ENOMEM, 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 (!q->serv) + ASSIGN_ERRNO(q, EAI_MEMORY, ENOMEM, 0); } } @@ -1087,7 +1075,7 @@ static int getnameinfo_done(sd_resolve_query *q) { assert(q->getnameinfo_handler); errno = q->_errno; - h_errno= q->_h_errno; + h_errno = q->_h_errno; return q->getnameinfo_handler(q, q->ret, q->host, q->serv, q->userdata); } From f6ddae40ab7790910b1f454c4a838ba624898900 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 May 2018 11:08:59 +0200 Subject: [PATCH 5/8] sd-resolve: do not assert on packet size received over a socket This is external data, even if trusted. We should not assert on it, but verify and return proper error instead, which assert_return does. In particular, write(2) says that a partial write could occur when interupted by a signal. When compiled with asserts disabled, we could access memory outside of the allocated buffer. CID #1237671. Follow-up for 1a96c8e1ccb06f87b6bfaff4639390ecd00af588. --- src/libsystemd/sd-resolve/sd-resolve.c | 28 +++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/libsystemd/sd-resolve/sd-resolve.c b/src/libsystemd/sd-resolve/sd-resolve.c index 9fabfcec6a..b5e9c01313 100644 --- a/src/libsystemd/sd-resolve/sd-resolve.c +++ b/src/libsystemd/sd-resolve/sd-resolve.c @@ -321,8 +321,8 @@ static int handle_request(int out_fd, const Packet *packet, size_t length) { req = &packet->rheader; - assert(length >= sizeof(RHeader)); - assert(length == req->length); + assert_return(length >= sizeof(RHeader), -EIO); + assert_return(length == req->length, -EIO); switch (req->type) { @@ -332,8 +332,8 @@ static int handle_request(int out_fd, const Packet *packet, size_t length) { const char *node, *service; int ret; - assert(length >= sizeof(AddrInfoRequest)); - assert(length == sizeof(AddrInfoRequest) + ai_req->node_len + ai_req->service_len); + assert_return(length >= sizeof(AddrInfoRequest), -EBADMSG); + assert_return(length == sizeof(AddrInfoRequest) + ai_req->node_len + ai_req->service_len, -EBADMSG); hints.ai_flags = ai_req->ai_flags; hints.ai_family = ai_req->ai_family; @@ -358,9 +358,9 @@ static int handle_request(int out_fd, const Packet *packet, size_t length) { union sockaddr_union sa; int ret; - assert(length >= sizeof(NameInfoRequest)); - assert(length == sizeof(NameInfoRequest) + ni_req->sockaddr_len); - assert(sizeof(sa) >= ni_req->sockaddr_len); + assert_return(length >= sizeof(NameInfoRequest), -EBADMSG); + assert_return(length == sizeof(NameInfoRequest) + ni_req->sockaddr_len, -EBADMSG); + assert_return(ni_req->sockaddr_len <= sizeof(sa), -EBADMSG); memcpy(&sa, (const uint8_t *) ni_req + sizeof(NameInfoRequest), ni_req->sockaddr_len); @@ -754,11 +754,11 @@ static int handle_response(sd_resolve *resolve, const Packet *packet, size_t len int r; assert(resolve); + assert(packet); resp = &packet->rheader; - assert(resp); - assert(length >= sizeof(RHeader)); - assert(length == resp->length); + assert_return(length >= sizeof(RHeader), -EIO); + assert_return(length == resp->length, -EIO); if (resp->type == RESPONSE_DIED) { resolve->dead = true; @@ -780,8 +780,8 @@ static int handle_response(sd_resolve *resolve, const Packet *packet, size_t len size_t l; struct addrinfo *prev = NULL; - assert(length >= sizeof(AddrInfoResponse)); - assert(q->type == REQUEST_ADDRINFO); + assert_return(length >= sizeof(AddrInfoResponse), -EBADMSG); + assert_return(q->type == REQUEST_ADDRINFO, -EBADMSG); ASSIGN_ERRNO(q, ai_resp->ret, ai_resp->_errno, ai_resp->_h_errno); @@ -813,8 +813,8 @@ static int handle_response(sd_resolve *resolve, const Packet *packet, size_t len case RESPONSE_NAMEINFO: { const NameInfoResponse *ni_resp = &packet->nameinfo_response; - assert(length >= sizeof(NameInfoResponse)); - assert(q->type == REQUEST_NAMEINFO); + assert_return(length >= sizeof(NameInfoResponse), -EBADMSG); + assert_return(q->type == REQUEST_NAMEINFO, -EBADMSG); if (ni_resp->hostlen > DNS_HOSTNAME_MAX || ni_resp->servlen > DNS_HOSTNAME_MAX || From e8bce4fad357bbf98a900381bbf50343229180bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 May 2018 15:52:17 +0200 Subject: [PATCH 6/8] sd-resolve: trivial indentation fixes --- src/libsystemd/sd-resolve/sd-resolve.c | 27 ++++++++++++-------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/libsystemd/sd-resolve/sd-resolve.c b/src/libsystemd/sd-resolve/sd-resolve.c index b5e9c01313..f662ae4a42 100644 --- a/src/libsystemd/sd-resolve/sd-resolve.c +++ b/src/libsystemd/sd-resolve/sd-resolve.c @@ -343,10 +343,9 @@ static int handle_request(int out_fd, const Packet *packet, size_t length) { node = ai_req->node_len ? (const char*) ai_req + sizeof(AddrInfoRequest) : NULL; service = ai_req->service_len ? (const char*) ai_req + sizeof(AddrInfoRequest) + ai_req->node_len : NULL; - ret = getaddrinfo( - node, service, - ai_req->hints_valid ? &hints : NULL, - &result); + ret = getaddrinfo(node, service, + ai_req->hints_valid ? &hints : NULL, + &result); /* send_addrinfo_reply() frees result */ return send_addrinfo_reply(out_fd, req->id, ret, result, errno, h_errno); @@ -365,14 +364,14 @@ static int handle_request(int out_fd, const Packet *packet, size_t length) { memcpy(&sa, (const uint8_t *) ni_req + sizeof(NameInfoRequest), ni_req->sockaddr_len); ret = getnameinfo(&sa.sa, ni_req->sockaddr_len, - ni_req->gethost ? hostbuf : NULL, ni_req->gethost ? sizeof(hostbuf) : 0, - ni_req->getserv ? servbuf : NULL, ni_req->getserv ? sizeof(servbuf) : 0, - ni_req->flags); + ni_req->gethost ? hostbuf : NULL, ni_req->gethost ? sizeof(hostbuf) : 0, + ni_req->getserv ? servbuf : NULL, ni_req->getserv ? sizeof(servbuf) : 0, + ni_req->flags); return send_nameinfo_reply(out_fd, req->id, ret, - ret == 0 && ni_req->gethost ? hostbuf : NULL, - ret == 0 && ni_req->getserv ? servbuf : NULL, - errno, h_errno); + ret == 0 && ni_req->gethost ? hostbuf : NULL, + ret == 0 && ni_req->getserv ? servbuf : NULL, + errno, h_errno); } case REQUEST_TERMINATE: @@ -399,7 +398,7 @@ static void* thread_worker(void *p) { } buf; ssize_t length; - length = recv(resolve->fds[REQUEST_RECV_FD], &buf, sizeof(buf), 0); + length = recv(resolve->fds[REQUEST_RECV_FD], &buf, sizeof buf, 0); if (length < 0) { if (errno == EINTR) continue; @@ -439,7 +438,6 @@ static int start_threads(sd_resolve *resolve, unsigned extra) { n = CLAMP(n, WORKERS_MIN, WORKERS_MAX); while (resolve->n_valid_workers < n) { - r = pthread_create(&resolve->workers[resolve->n_valid_workers], NULL, thread_worker, resolve); if (r > 0) { r = -r; @@ -580,7 +578,7 @@ static void resolve_free(sd_resolve *resolve) { RHeader req = { .type = REQUEST_TERMINATE, - .length = sizeof(req) + .length = sizeof req, }; /* Send one termination packet for each worker */ @@ -608,7 +606,6 @@ _public_ sd_resolve* sd_resolve_ref(sd_resolve *resolve) { } _public_ sd_resolve* sd_resolve_unref(sd_resolve *resolve) { - if (!resolve) return NULL; @@ -863,7 +860,7 @@ _public_ int sd_resolve_process(sd_resolve *resolve) { /* We don't allow recursively invoking sd_resolve_process(). */ assert_return(!resolve->current, -EBUSY); - l = recv(resolve->fds[RESPONSE_RECV_FD], &buf, sizeof(buf), 0); + l = recv(resolve->fds[RESPONSE_RECV_FD], &buf, sizeof buf, 0); if (l < 0) { if (errno == EAGAIN) return 0; From 2da063370acdb83a408095431a3a7d124f4ea5dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 May 2018 15:53:00 +0200 Subject: [PATCH 7/8] sd-resolve: use structured initializers and _cleanup_ Also remove a break that cannot be reached and a resolve->dead check that duplicates the loop condition. --- src/libsystemd/sd-resolve/sd-resolve.c | 124 +++++++++++------------ src/libsystemd/sd-resolve/test-resolve.c | 1 - 2 files changed, 57 insertions(+), 68 deletions(-) diff --git a/src/libsystemd/sd-resolve/sd-resolve.c b/src/libsystemd/sd-resolve/sd-resolve.c index f662ae4a42..acd3146bfb 100644 --- a/src/libsystemd/sd-resolve/sd-resolve.c +++ b/src/libsystemd/sd-resolve/sd-resolve.c @@ -233,12 +233,12 @@ static int send_addrinfo_reply( ._h_errno = _h_errno, }; - struct msghdr mh = {}; - struct iovec iov[2]; union { AddrInfoSerialization ais; uint8_t space[BUFSIZE]; } buffer; + struct iovec iov[2]; + struct msghdr mh; assert(out_fd >= 0); @@ -261,8 +261,7 @@ static int send_addrinfo_reply( iov[0] = (struct iovec) { .iov_base = &resp, .iov_len = sizeof(AddrInfoResponse) }; iov[1] = (struct iovec) { .iov_base = &buffer, .iov_len = resp.header.length - sizeof(AddrInfoResponse) }; - mh.msg_iov = iov; - mh.msg_iovlen = ELEMENTSOF(iov); + mh = (struct msghdr) { .msg_iov = iov, .msg_iovlen = ELEMENTSOF(iov) }; if (sendmsg(out_fd, &mh, MSG_NOSIGNAL) < 0) return -errno; @@ -287,8 +286,8 @@ static int send_nameinfo_reply( ._h_errno = _h_errno, }; - struct msghdr mh = {}; struct iovec iov[3]; + struct msghdr mh; size_t hl, sl; assert(out_fd >= 0); @@ -304,8 +303,7 @@ static int send_nameinfo_reply( iov[1] = (struct iovec) { .iov_base = (void*) host, .iov_len = hl }; iov[2] = (struct iovec) { .iov_base = (void*) serv, .iov_len = sl }; - mh.msg_iov = iov; - mh.msg_iovlen = ELEMENTSOF(iov); + mh = (struct msghdr) { .msg_iov = iov, .msg_iovlen = ELEMENTSOF(iov) }; if (sendmsg(out_fd, &mh, MSG_NOSIGNAL) < 0) return -errno; @@ -328,17 +326,19 @@ static int handle_request(int out_fd, const Packet *packet, size_t length) { case REQUEST_ADDRINFO: { const AddrInfoRequest *ai_req = &packet->addrinfo_request; - struct addrinfo hints = {}, *result = NULL; + struct addrinfo hints, *result = NULL; const char *node, *service; int ret; assert_return(length >= sizeof(AddrInfoRequest), -EBADMSG); assert_return(length == sizeof(AddrInfoRequest) + ai_req->node_len + ai_req->service_len, -EBADMSG); - hints.ai_flags = ai_req->ai_flags; - hints.ai_family = ai_req->ai_family; - hints.ai_socktype = ai_req->ai_socktype; - hints.ai_protocol = ai_req->ai_protocol; + hints = (struct addrinfo) { + .ai_flags = ai_req->ai_flags, + .ai_family = ai_req->ai_family, + .ai_socktype = ai_req->ai_socktype, + .ai_protocol = ai_req->ai_protocol, + }; node = ai_req->node_len ? (const char*) ai_req + sizeof(AddrInfoRequest) : NULL; service = ai_req->service_len ? (const char*) ai_req + sizeof(AddrInfoRequest) + ai_req->node_len : NULL; @@ -408,9 +408,6 @@ static void* thread_worker(void *p) { if (length == 0) break; - if (resolve->dead) - break; - if (handle_request(resolve->fds[RESPONSE_SEND_FD], &buf.packet, (size_t) length) < 0) break; } @@ -467,8 +464,8 @@ static bool resolve_pid_changed(sd_resolve *r) { } _public_ int sd_resolve_new(sd_resolve **ret) { - sd_resolve *resolve = NULL; - int i, r; + _cleanup_(sd_resolve_unrefp) sd_resolve *resolve = NULL; + int i; assert_return(ret, -EINVAL); @@ -482,17 +479,11 @@ _public_ int sd_resolve_new(sd_resolve **ret) { for (i = 0; i < _FD_MAX; i++) resolve->fds[i] = -1; - r = socketpair(PF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0, resolve->fds + REQUEST_RECV_FD); - if (r < 0) { - r = -errno; - goto fail; - } + if (socketpair(PF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0, resolve->fds + REQUEST_RECV_FD) < 0) + return -errno; - r = socketpair(PF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0, resolve->fds + RESPONSE_RECV_FD); - if (r < 0) { - r = -errno; - goto fail; - } + if (socketpair(PF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0, resolve->fds + RESPONSE_RECV_FD) < 0) + return -errno; for (i = 0; i < _FD_MAX; i++) resolve->fds[i] = fd_move_above_stdio(resolve->fds[i]); @@ -504,12 +495,8 @@ _public_ int sd_resolve_new(sd_resolve **ret) { (void) fd_nonblock(resolve->fds[RESPONSE_RECV_FD], true); - *ret = resolve; + *ret = TAKE_PTR(resolve); return 0; - -fail: - sd_resolve_unref(resolve); - return r; } _public_ int sd_resolve_default(sd_resolve **ret) { @@ -941,11 +928,12 @@ _public_ int sd_resolve_getaddrinfo( const struct addrinfo *hints, sd_resolve_getaddrinfo_handler_t callback, void *userdata) { - AddrInfoRequest req = {}; - struct msghdr mh = {}; + _cleanup_(sd_resolve_query_unrefp) sd_resolve_query *q = NULL; + AddrInfoRequest req; struct iovec iov[3]; - sd_resolve_query *q; + struct msghdr mh = {}; int r; + size_t node_len, service_len; assert_return(resolve, -EINVAL); assert_return(node || service, -EINVAL); @@ -960,20 +948,23 @@ _public_ int sd_resolve_getaddrinfo( q->getaddrinfo_handler = callback; q->userdata = userdata; - req.node_len = node ? strlen(node)+1 : 0; - req.service_len = service ? strlen(service)+1 : 0; + node_len = node ? strlen(node) + 1 : 0; + service_len = service ? strlen(service) + 1 : 0; - req.header.id = q->id; - req.header.type = REQUEST_ADDRINFO; - req.header.length = sizeof(AddrInfoRequest) + req.node_len + req.service_len; + req = (AddrInfoRequest) { + .node_len = node_len, + .service_len = service_len, - if (hints) { - req.hints_valid = true; - req.ai_flags = hints->ai_flags; - req.ai_family = hints->ai_family; - req.ai_socktype = hints->ai_socktype; - req.ai_protocol = hints->ai_protocol; - } + .header.id = q->id, + .header.type = REQUEST_ADDRINFO, + .header.length = sizeof(AddrInfoRequest) + node_len + service_len, + + .hints_valid = hints, + .ai_flags = hints ? hints->ai_flags : 0, + .ai_family = hints ? hints->ai_family : 0, + .ai_socktype = hints ? hints->ai_socktype : 0, + .ai_protocol = hints ? hints->ai_protocol : 0, + }; iov[mh.msg_iovlen++] = (struct iovec) { .iov_base = &req, .iov_len = sizeof(AddrInfoRequest) }; if (node) @@ -982,15 +973,14 @@ _public_ int sd_resolve_getaddrinfo( iov[mh.msg_iovlen++] = (struct iovec) { .iov_base = (void*) service, .iov_len = req.service_len }; mh.msg_iov = iov; - if (sendmsg(resolve->fds[REQUEST_SEND_FD], &mh, MSG_NOSIGNAL) < 0) { - sd_resolve_query_unref(q); + if (sendmsg(resolve->fds[REQUEST_SEND_FD], &mh, MSG_NOSIGNAL) < 0) return -errno; - } resolve->n_outstanding++; if (_q) *_q = q; + TAKE_PTR(q); return 0; } @@ -1015,10 +1005,10 @@ _public_ int sd_resolve_getnameinfo( sd_resolve_getnameinfo_handler_t callback, void *userdata) { - NameInfoRequest req = {}; - struct msghdr mh = {}; + _cleanup_(sd_resolve_query_unrefp) sd_resolve_query *q = NULL; + NameInfoRequest req; struct iovec iov[2]; - sd_resolve_query *q; + struct msghdr mh; int r; assert_return(resolve, -EINVAL); @@ -1037,31 +1027,31 @@ _public_ int sd_resolve_getnameinfo( q->getnameinfo_handler = callback; q->userdata = userdata; - req.header.id = q->id; - req.header.type = REQUEST_NAMEINFO; - req.header.length = sizeof(NameInfoRequest) + salen; + req = (NameInfoRequest) { + .header.id = q->id, + .header.type = REQUEST_NAMEINFO, + .header.length = sizeof(NameInfoRequest) + salen, - req.flags = flags; - req.sockaddr_len = salen; - req.gethost = !!(get & SD_RESOLVE_GET_HOST); - req.getserv = !!(get & SD_RESOLVE_GET_SERVICE); + .flags = flags, + .sockaddr_len = salen, + .gethost = !!(get & SD_RESOLVE_GET_HOST), + .getserv = !!(get & SD_RESOLVE_GET_SERVICE), + }; iov[0] = (struct iovec) { .iov_base = &req, .iov_len = sizeof(NameInfoRequest) }; iov[1] = (struct iovec) { .iov_base = (void*) sa, .iov_len = salen }; - mh.msg_iov = iov; - mh.msg_iovlen = 2; + mh = (struct msghdr) { .msg_iov = iov, .msg_iovlen = ELEMENTSOF(iov) }; - if (sendmsg(resolve->fds[REQUEST_SEND_FD], &mh, MSG_NOSIGNAL) < 0) { - sd_resolve_query_unref(q); + if (sendmsg(resolve->fds[REQUEST_SEND_FD], &mh, MSG_NOSIGNAL) < 0) return -errno; - } - - resolve->n_outstanding++; if (_q) *_q = q; + resolve->n_outstanding++; + TAKE_PTR(q); + return 0; } diff --git a/src/libsystemd/sd-resolve/test-resolve.c b/src/libsystemd/sd-resolve/test-resolve.c index 5ec35e1b4f..7020412ab2 100644 --- a/src/libsystemd/sd-resolve/test-resolve.c +++ b/src/libsystemd/sd-resolve/test-resolve.c @@ -103,7 +103,6 @@ int main(int argc, char *argv[]) { log_notice_errno(r, "sd_resolve_wait() timed out, but that's OK"); exit(EXIT_SUCCESS); - break; } if (r < 0) { log_error_errno(r, "sd_resolve_wait(): %m"); From 38dad4433295d583343b82aac87326d407fe2927 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 14 May 2018 16:59:20 +0200 Subject: [PATCH 8/8] sd-resolve: fix check for packet size The protocol is that a string is serialized with the nul byte at the end, and the terminator is included in length. We'd call strndup with offset 0, length len1-1, and then a second time with offset len1, length len2-1, so in the end the check was off by one. But let's require the terminating nul too, even if we don't access it. CID #1383035. --- src/libsystemd/sd-resolve/sd-resolve.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsystemd/sd-resolve/sd-resolve.c b/src/libsystemd/sd-resolve/sd-resolve.c index acd3146bfb..ba0f15538d 100644 --- a/src/libsystemd/sd-resolve/sd-resolve.c +++ b/src/libsystemd/sd-resolve/sd-resolve.c @@ -802,7 +802,7 @@ static int handle_response(sd_resolve *resolve, const Packet *packet, size_t len if (ni_resp->hostlen > DNS_HOSTNAME_MAX || ni_resp->servlen > DNS_HOSTNAME_MAX || - sizeof(NameInfoResponse) + ni_resp->hostlen + ni_resp->servlen > length + 2) + sizeof(NameInfoResponse) + ni_resp->hostlen + ni_resp->servlen > length) ASSIGN_ERRNO(q, EAI_SYSTEM, EIO, 0); else {