From 0bc66e529fa34b84ae31301dd99f31cc16ccfd6c Mon Sep 17 00:00:00 2001 From: Jacek Galowicz Date: Thu, 2 Nov 2023 10:13:55 +0100 Subject: [PATCH 1/6] Use npos member variables instead of full type --- src/libutil/file-system.cc | 6 +++--- src/libutil/util.cc | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index ab8d32275..14d496958 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -90,7 +90,7 @@ Path canonPath(PathView path, bool resolveSymlinks) /* Normal component; copy it. */ else { s += '/'; - if (const auto slash = path.find('/'); slash == std::string::npos) { + if (const auto slash = path.find('/'); slash == path.npos) { s += path; path = {}; } else { @@ -123,7 +123,7 @@ Path canonPath(PathView path, bool resolveSymlinks) Path dirOf(const PathView path) { Path::size_type pos = path.rfind('/'); - if (pos == std::string::npos) + if (pos == path.npos) return "."; return pos == 0 ? "/" : Path(path, 0, pos); } @@ -139,7 +139,7 @@ std::string_view baseNameOf(std::string_view path) last -= 1; auto pos = path.rfind('/', last); - if (pos == std::string::npos) + if (pos == path.npos) pos = 0; else pos += 1; diff --git a/src/libutil/util.cc b/src/libutil/util.cc index b23362b5c..6e47ce2a3 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -52,9 +52,9 @@ template C tokenizeString(std::string_view s, std::string_view separato { C result; auto pos = s.find_first_not_of(separators, 0); - while (pos != std::string_view::npos) { + while (pos != s.npos) { auto end = s.find_first_of(separators, pos + 1); - if (end == std::string_view::npos) end = s.size(); + if (end == s.npos) end = s.size(); result.insert(result.end(), std::string(s, pos, end - pos)); pos = s.find_first_not_of(separators, end); } @@ -69,7 +69,7 @@ template std::vector tokenizeString(std::string_view s, std::string std::string chomp(std::string_view s) { size_t i = s.find_last_not_of(" \n\r\t"); - return i == std::string_view::npos ? "" : std::string(s, 0, i + 1); + return i == s.npos ? "" : std::string(s, 0, i + 1); } @@ -89,7 +89,7 @@ std::string replaceStrings( { if (from.empty()) return res; size_t pos = 0; - while ((pos = res.find(from, pos)) != std::string::npos) { + while ((pos = res.find(from, pos)) != res.npos) { res.replace(pos, from.size(), to); pos += to.size(); } @@ -102,7 +102,7 @@ std::string rewriteStrings(std::string s, const StringMap & rewrites) for (auto & i : rewrites) { if (i.first == i.second) continue; size_t j = 0; - while ((j = s.find(i.first, j)) != std::string::npos) + while ((j = s.find(i.first, j)) != s.npos) s.replace(j, i.first.size(), i.second); } return s; From 1885d579db145d45f0aaf6130cd7e4db17b5e214 Mon Sep 17 00:00:00 2001 From: Jacek Galowicz Date: Thu, 2 Nov 2023 15:49:22 +0100 Subject: [PATCH 2/6] Improve String Handling --- src/libutil/file-system.cc | 6 +++++- src/libutil/util.cc | 9 ++++----- src/libutil/util.hh | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 14d496958..cf8a6d967 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -116,7 +116,11 @@ Path canonPath(PathView path, bool resolveSymlinks) } } - return s.empty() ? "/" : std::move(s); + if (s.empty()) { + s = "/"; + } + + return s; } diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 6e47ce2a3..8f310c6fe 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -122,12 +122,11 @@ bool hasSuffix(std::string_view s, std::string_view suffix) } -std::string toLower(const std::string & s) +std::string toLower(std::string s) { - std::string r(s); - for (auto & c : r) + for (auto & c : s) c = std::tolower(c); - return r; + return s; } @@ -135,7 +134,7 @@ std::string shellEscape(const std::string_view s) { std::string r; r.reserve(s.size() + 2); - r += "'"; + r += '\''; for (auto & i : s) if (i == '\'') r += "'\\''"; else r += i; r += '\''; diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 27faa4d6d..11a0431da 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -180,7 +180,7 @@ bool hasSuffix(std::string_view s, std::string_view suffix); /** * Convert a string to lower case. */ -std::string toLower(const std::string & s); +std::string toLower(std::string s); /** From c924147c9d782e70e0ad421329252ced57f88d09 Mon Sep 17 00:00:00 2001 From: Jacek Galowicz Date: Thu, 2 Nov 2023 15:50:00 +0100 Subject: [PATCH 3/6] Drop parentheses from thunks --- src/libutil/file-descriptor.cc | 2 +- src/libutil/processes.cc | 12 ++++++------ src/libutil/unix-domain-socket.cc | 4 ++-- src/libutil/util.cc | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libutil/file-descriptor.cc b/src/libutil/file-descriptor.cc index 692be3383..43e3cd979 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -96,7 +96,7 @@ void drainFD(int fd, Sink & sink, bool block) throw SysError("making file descriptor non-blocking"); } - Finally finally([&]() { + Finally finally([&] { if (!block) { if (fcntl(fd, F_SETFL, saved) == -1) throw SysError("making file descriptor blocking"); diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index 91a0ea66f..e1e60302b 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -131,7 +131,7 @@ void killUser(uid_t uid) users to which the current process can send signals. So we fork a process, switch to uid, and send a mass kill. */ - Pid pid = startProcess([&]() { + Pid pid = startProcess([&] { if (setuid(uid) == -1) throw SysError("setting uid"); @@ -197,7 +197,7 @@ static int childEntry(void * arg) pid_t startProcess(std::function fun, const ProcessOptions & options) { - std::function wrapper = [&]() { + ChildWrapperFunction wrapper = [&] { if (!options.allowVfork) logger = makeSimpleLogger(); try { @@ -229,7 +229,7 @@ pid_t startProcess(std::function fun, const ProcessOptions & options) PROT_WRITE | PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); if (stack == MAP_FAILED) throw SysError("allocating stack"); - Finally freeStack([&]() { munmap(stack, stackSize); }); + Finally freeStack([&] { munmap(stack, stackSize); }); pid = clone(childEntry, stack + stackSize, options.cloneFlags | SIGCHLD, &wrapper); #else @@ -308,7 +308,7 @@ void runProgram2(const RunOptions & options) } /* Fork. */ - Pid pid = startProcess([&]() { + Pid pid = startProcess([&] { if (options.environment) replaceEnv(*options.environment); if (options.standardOut && dup2(out.writeSide.get(), STDOUT_FILENO) == -1) @@ -350,7 +350,7 @@ void runProgram2(const RunOptions & options) std::promise promise; - Finally doJoin([&]() { + Finally doJoin([&] { if (writerThread.joinable()) writerThread.join(); }); @@ -358,7 +358,7 @@ void runProgram2(const RunOptions & options) if (source) { in.readSide.close(); - writerThread = std::thread([&]() { + writerThread = std::thread([&] { try { std::vector buf(8 * 1024); while (true) { diff --git a/src/libutil/unix-domain-socket.cc b/src/libutil/unix-domain-socket.cc index 05bbb5ba3..dc19daf9e 100644 --- a/src/libutil/unix-domain-socket.cc +++ b/src/libutil/unix-domain-socket.cc @@ -47,7 +47,7 @@ void bind(int fd, const std::string & path) addr.sun_family = AF_UNIX; if (path.size() + 1 >= sizeof(addr.sun_path)) { - Pid pid = startProcess([&]() { + Pid pid = startProcess([&] { Path dir = dirOf(path); if (chdir(dir.c_str()) == -1) throw SysError("chdir to '%s' failed", dir); @@ -78,7 +78,7 @@ void connect(int fd, const std::string & path) if (path.size() + 1 >= sizeof(addr.sun_path)) { Pipe pipe; pipe.create(); - Pid pid = startProcess([&]() { + Pid pid = startProcess([&] { try { pipe.readSide.close(); Path dir = dirOf(path); diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 8f310c6fe..75bb31c9b 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -183,7 +183,7 @@ std::string base64Encode(std::string_view s) std::string base64Decode(std::string_view s) { constexpr char npos = -1; - constexpr std::array base64DecodeChars = [&]() { + constexpr std::array base64DecodeChars = [&] { std::array result{}; for (auto& c : result) c = npos; From d11d7849f7676eb8b2c771356b9be8d8bb756cc8 Mon Sep 17 00:00:00 2001 From: Jacek Galowicz Date: Thu, 2 Nov 2023 15:51:47 +0100 Subject: [PATCH 4/6] Use ChildWrapperFunction type and make casts more explicit --- src/libutil/processes.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index e1e60302b..28f1adcf0 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -168,11 +168,12 @@ void killUser(uid_t uid) ////////////////////////////////////////////////////////////////////// +using ChildWrapperFunction = std::function; /* Wrapper around vfork to prevent the child process from clobbering the caller's stack frame in the parent. */ -static pid_t doFork(bool allowVfork, std::function fun) __attribute__((noinline)); -static pid_t doFork(bool allowVfork, std::function fun) +static pid_t doFork(bool allowVfork, ChildWrapperFunction & fun) __attribute__((noinline)); +static pid_t doFork(bool allowVfork, ChildWrapperFunction & fun) { #ifdef __linux__ pid_t pid = allowVfork ? vfork() : fork(); @@ -188,8 +189,8 @@ static pid_t doFork(bool allowVfork, std::function fun) #if __linux__ static int childEntry(void * arg) { - auto main = (std::function *) arg; - (*main)(); + auto & fun = *reinterpret_cast(arg); + fun(); return 1; } #endif From 9d9f42cc38b06ddc3fe30f4c1695514774b5217e Mon Sep 17 00:00:00 2001 From: Jacek Galowicz Date: Thu, 2 Nov 2023 15:52:38 +0100 Subject: [PATCH 5/6] Remove C-style casts --- src/libutil/file-descriptor.cc | 2 +- src/libutil/processes.cc | 4 ++-- src/libutil/unix-domain-socket.cc | 18 ++++++++++++++---- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/libutil/file-descriptor.cc b/src/libutil/file-descriptor.cc index 43e3cd979..55d57e29b 100644 --- a/src/libutil/file-descriptor.cc +++ b/src/libutil/file-descriptor.cc @@ -114,7 +114,7 @@ void drainFD(int fd, Sink & sink, bool block) throw SysError("reading from file"); } else if (rd == 0) break; - else sink({(char *) buf.data(), (size_t) rd}); + else sink({reinterpret_cast(buf.data()), size_t(rd)}); } } diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index 28f1adcf0..f5d584330 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -226,8 +226,8 @@ pid_t startProcess(std::function fun, const ProcessOptions & options) assert(!(options.cloneFlags & CLONE_VM)); size_t stackSize = 1 * 1024 * 1024; - auto stack = (char *) mmap(0, stackSize, - PROT_WRITE | PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); + auto stack = static_cast(mmap(0, stackSize, + PROT_WRITE | PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0)); if (stack == MAP_FAILED) throw SysError("allocating stack"); Finally freeStack([&] { munmap(stack, stackSize); }); diff --git a/src/libutil/unix-domain-socket.cc b/src/libutil/unix-domain-socket.cc index dc19daf9e..3b6d54a2c 100644 --- a/src/libutil/unix-domain-socket.cc +++ b/src/libutil/unix-domain-socket.cc @@ -38,6 +38,14 @@ AutoCloseFD createUnixDomainSocket(const Path & path, mode_t mode) return fdSocket; } +static struct sockaddr* safeSockAddrPointerCast(struct sockaddr_un *addr) { + // Casting between types like these legacy C library interfaces require + // is forbidden in C++. + // To maintain backwards compatibility, the implementation of the + // bind function contains some hints to the compiler that allow for this + // special case. + return reinterpret_cast(addr); +} void bind(int fd, const std::string & path) { @@ -45,6 +53,7 @@ void bind(int fd, const std::string & path) struct sockaddr_un addr; addr.sun_family = AF_UNIX; + auto psaddr {safeSockAddrPointerCast(&addr)}; if (path.size() + 1 >= sizeof(addr.sun_path)) { Pid pid = startProcess([&] { @@ -55,7 +64,7 @@ void bind(int fd, const std::string & path) if (base.size() + 1 >= sizeof(addr.sun_path)) throw Error("socket path '%s' is too long", base); memcpy(addr.sun_path, base.c_str(), base.size() + 1); - if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) + if (bind(fd, psaddr, sizeof(addr)) == -1) throw SysError("cannot bind to socket '%s'", path); _exit(0); }); @@ -64,7 +73,7 @@ void bind(int fd, const std::string & path) throw Error("cannot bind to socket '%s'", path); } else { memcpy(addr.sun_path, path.c_str(), path.size() + 1); - if (bind(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) + if (bind(fd, psaddr, sizeof(addr)) == -1) throw SysError("cannot bind to socket '%s'", path); } } @@ -74,6 +83,7 @@ void connect(int fd, const std::string & path) { struct sockaddr_un addr; addr.sun_family = AF_UNIX; + auto psaddr {safeSockAddrPointerCast(&addr)}; if (path.size() + 1 >= sizeof(addr.sun_path)) { Pipe pipe; @@ -88,7 +98,7 @@ void connect(int fd, const std::string & path) if (base.size() + 1 >= sizeof(addr.sun_path)) throw Error("socket path '%s' is too long", base); memcpy(addr.sun_path, base.c_str(), base.size() + 1); - if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) + if (connect(fd, psaddr, sizeof(addr)) == -1) throw SysError("cannot connect to socket at '%s'", path); writeFull(pipe.writeSide.get(), "0\n"); } catch (SysError & e) { @@ -107,7 +117,7 @@ void connect(int fd, const std::string & path) } } else { memcpy(addr.sun_path, path.c_str(), path.size() + 1); - if (connect(fd, (struct sockaddr *) &addr, sizeof(addr)) == -1) + if (connect(fd, psaddr, sizeof(addr)) == -1) throw SysError("cannot connect to socket at '%s'", path); } } From 8ae3aeec9442e2b249abdb42a2853618b74a68a2 Mon Sep 17 00:00:00 2001 From: Jacek Galowicz Date: Thu, 2 Nov 2023 15:52:53 +0100 Subject: [PATCH 6/6] Don't use std::make_unique right before release --- src/libutil/signals.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/signals.cc b/src/libutil/signals.cc index 4632aa319..eaa4ea30e 100644 --- a/src/libutil/signals.cc +++ b/src/libutil/signals.cc @@ -179,7 +179,7 @@ std::unique_ptr createInterruptCallback(std::function auto token = interruptCallbacks->nextToken++; interruptCallbacks->callbacks.emplace(token, callback); - auto res = std::make_unique(); + std::unique_ptr res {new InterruptCallbackImpl{}}; res->token = token; return std::unique_ptr(res.release());