Merge pull request #5068 from c4rlo/sway-ipc-fixes

Sway IPC client: robustness improvements & code cleanups

include/modules/sway/ipc/client.hpp

@@ -1,14 +1,10 @@
 #pragma once
 
 #include <sigc++/sigc++.h>
-#include <sys/socket.h>
-#include <sys/un.h>
-#include <unistd.h>
 
-#include <cstring>
+#include <cstdint>
-#include <memory>
+#include <functional>
 #include <mutex>
-#include <stdexcept>
 #include <string>
 
 #include "ipc.hpp"
@@ -41,8 +37,9 @@ class Ipc {
   static inline const std::string ipc_magic_ = "i3-ipc";
   static inline const size_t ipc_header_size_ = ipc_magic_.size() + 8;
 
-  const std::string getSocketPath() const;
+  static std::string getSocketPath();
-  int open(const std::string&) const;
+  static int open(const std::string&);
+
   struct ipc_response send(int fd, uint32_t type, const std::string& payload = "");
   struct ipc_response recv(int fd);
 

src/modules/sway/ipc/client.cpp

@@ -2,13 +2,46 @@
 
 #include <fcntl.h>
 #include <spdlog/spdlog.h>
+#include <sys/socket.h>
+#include <sys/types.h>
+#include <sys/un.h>
+#include <unistd.h>
 
+#include <cerrno>
+#include <cstdio>
+#include <cstdlib>
+#include <cstring>
+#include <limits>
 #include <stdexcept>
+#include <string_view>
+#include <utility>
+
+#include "modules/sway/ipc/ipc.hpp"
 
 namespace waybar::modules::sway {
+namespace {
+
+void sendAll(int fd, const char* data, size_t size, const char* what) {
+  size_t total = 0;
+  while (total < size) {
+    const auto res = ::send(fd, data + total, size - total, 0);
+    if (res < 0) {
+      if (errno == EINTR || errno == EAGAIN) {
+        continue;
+      }
+      throw std::runtime_error(what);
+    }
+    if (res == 0) {
+      throw std::runtime_error(what);
+    }
+    total += static_cast<size_t>(res);
+  }
+}
+
+}  // namespace
 
 Ipc::Ipc() {
-  const std::string& socketPath = getSocketPath();
+  const std::string socketPath = getSocketPath();
   fd_ = util::ScopedFd(open(socketPath));
   fd_event_ = util::ScopedFd(open(socketPath));
 }
@@ -29,49 +62,49 @@ Ipc::~Ipc() {
   }
 }
 
-void Ipc::setWorker(std::function<void()>&& func) { thread_ = func; }
+void Ipc::setWorker(std::function<void()>&& func) { thread_ = std::move(func); }
 
-const std::string Ipc::getSocketPath() const {
+std::string Ipc::getSocketPath() {
   const char* env = getenv("SWAYSOCK");
-  if (env != nullptr) {
+  if (env != nullptr && env[0] != '\0') {
-    return std::string(env);
+    return {env};
   }
+
+  FILE* in = popen("sway --get-socketpath 2>/dev/null", "r");
+  if (in == nullptr) {
+    throw std::runtime_error("Failed to get socket path");
+  }
+
   std::string str;
-  {
+  char buf[512] = {0};
-    std::string str_buf;
+  while (fgets(buf, sizeof(buf), in) != nullptr) {
-    FILE* in;
+    str.append(buf);
-    char buf[512] = {0};
-    if ((in = popen("sway --get-socketpath 2>/dev/null", "r")) == nullptr) {
-      throw std::runtime_error("Failed to get socket path");
-    }
-    while (fgets(buf, sizeof(buf), in) != nullptr) {
-      str_buf.append(buf, sizeof(buf));
-    }
-    pclose(in);
-    str = str_buf;
-    if (str.empty()) {
-      throw std::runtime_error("Socket path is empty");
-    }
   }
-  if (str.back() == '\n') {
+
+  if (pclose(in) == -1) {
+    throw std::runtime_error("Failed to get socket path");
+  }
+
+  if (str.ends_with('\n')) {
     str.pop_back();
   }
+  if (str.empty()) {
+    throw std::runtime_error("Socket path is empty");
+  }
+
   return str;
 }
 
-int Ipc::open(const std::string& socketPath) const {
+int Ipc::open(const std::string& socketPath) {
   util::ScopedFd fd(socket(AF_UNIX, SOCK_STREAM, 0));
   if (fd == -1) {
     throw std::runtime_error("Unable to open Unix socket");
   }
   (void)fcntl(fd, F_SETFD, FD_CLOEXEC);
-  struct sockaddr_un addr;
+  struct sockaddr_un addr{.sun_family = AF_UNIX};
-  memset(&addr, 0, sizeof(struct sockaddr_un));
-  addr.sun_family = AF_UNIX;
   strncpy(addr.sun_path, socketPath.c_str(), sizeof(addr.sun_path) - 1);
   addr.sun_path[sizeof(addr.sun_path) - 1] = 0;
-  int l = sizeof(struct sockaddr_un);
+  if (::connect(fd, reinterpret_cast<struct sockaddr*>(&addr), sizeof addr) == -1) {
-  if (::connect(fd, reinterpret_cast<struct sockaddr*>(&addr), l) == -1) {
     throw std::runtime_error("Unable to connect to Sway");
   }
   return fd.release();
@@ -80,55 +113,70 @@ int Ipc::open(const std::string& socketPath) const {
 struct Ipc::ipc_response Ipc::recv(int fd) {
   std::string header;
   header.resize(ipc_header_size_);
-  auto data32 = reinterpret_cast<uint32_t*>(header.data() + ipc_magic_.size());
-  size_t total = 0;
 
+  size_t total = 0;
   while (total < ipc_header_size_) {
-    auto res = ::recv(fd, header.data() + total, ipc_header_size_ - total, 0);
+    const ssize_t res = ::recv(fd, header.data() + total, ipc_header_size_ - total, 0);
     if (fd_event_ == -1 || fd_ == -1) {
       // IPC is closed so just return an empty response
-      return {0, 0, ""};
+      return {.size = 0, .type = 0, .payload = ""};
     }
-    if (res <= 0) {
+    if (res < 0) {
+      if (errno == EINTR || errno == EAGAIN) {
+        continue;
+      }
       throw std::runtime_error("Unable to receive IPC header");
     }
-    total += res;
+    if (res == 0) {
+      throw std::runtime_error("Unable to receive IPC header");
+    }
+    total += static_cast<size_t>(res);
   }
-  auto magic = std::string(header.data(), header.data() + ipc_magic_.size());
+  if (std::string_view(header.data(), ipc_magic_.size()) != ipc_magic_) {
-  if (magic != ipc_magic_) {
     throw std::runtime_error("Invalid IPC magic");
   }
 
-  total = 0;
+  uint32_t payload_size = 0;
+  uint32_t payload_type = 0;
+  memcpy(&payload_size, header.data() + ipc_magic_.size(), sizeof payload_size);
+  memcpy(&payload_type, header.data() + ipc_magic_.size() + sizeof payload_size,
+         sizeof payload_type);
+
   std::string payload;
-  payload.resize(data32[0]);
+  payload.resize(payload_size);
-  while (total < data32[0]) {
+
-    auto res = ::recv(fd, payload.data() + total, data32[0] - total, 0);
+  total = 0;
+  while (total < payload_size) {
+    const ssize_t res = ::recv(fd, payload.data() + total, payload_size - total, 0);
     if (res < 0) {
       if (errno == EINTR || errno == EAGAIN) {
         continue;
       }
       throw std::runtime_error("Unable to receive IPC payload");
     }
-    total += res;
+    if (res == 0) {
+      throw std::runtime_error("Unable to receive IPC payload");
+    }
+    total += static_cast<size_t>(res);
   }
-  return {data32[0], data32[1], &payload.front()};
+
+  return {.size = payload_size, .type = payload_type, .payload = std::move(payload)};
 }
 
 struct Ipc::ipc_response Ipc::send(int fd, uint32_t type, const std::string& payload) {
   std::string header;
   header.resize(ipc_header_size_);
-  auto data32 = reinterpret_cast<uint32_t*>(header.data() + ipc_magic_.size());
+  memcpy(header.data(), ipc_magic_.data(), ipc_magic_.size());
-  memcpy(header.data(), ipc_magic_.c_str(), ipc_magic_.size());
+  if (payload.size() > std::numeric_limits<uint32_t>::max()) {
-  data32[0] = payload.size();
+    throw std::runtime_error("IPC payload is too large");
-  data32[1] = type;
+  }
+  const auto payload_size = static_cast<uint32_t>(payload.size());
+  memcpy(header.data() + ipc_magic_.size(), &payload_size, sizeof payload_size);
+  memcpy(header.data() + ipc_magic_.size() + sizeof payload_size, &type, sizeof type);
+
+  sendAll(fd, header.data(), ipc_header_size_, "Unable to send IPC header");
+  sendAll(fd, payload.data(), payload.size(), "Unable to send IPC payload");
 
-  if (::send(fd, header.data(), ipc_header_size_, 0) == -1) {
-    throw std::runtime_error("Unable to send IPC header");
-  }
-  if (::send(fd, payload.c_str(), payload.size(), 0) == -1) {
-    throw std::runtime_error("Unable to send IPC payload");
-  }
   return Ipc::recv(fd);
 }