Handle syscall failures in wf::compositor_core_impl_t::run (#2575)

* Handle syscall failures in wf::compositor_core_impl_t::run

This patch address the compiler warnings:

[455/476] Compiling C++ object src/liblibwayfire.a.p/core_core.cpp.o
../../../../github/wayfire/wayfire.git/src/core/core.cpp: In member function ‘virtual pid_t wf::compositor_core_impl_t::run(std::string)’:
../../../../github/wayfire/wayfire.git/src/core/core.cpp:433:10: warning: ignoring return value of ‘int pipe2(int*, int)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  433 |     pipe2(pipe_fd, O_CLOEXEC);
      |     ~~~~~^~~~~~~~~~~~~~~~~~~~
../../../../github/wayfire/wayfire.git/src/core/core.cpp:467:18: warning: ignoring return value of ‘ssize_t write(int, const void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  467 |             write(pipe_fd[WRITE_END], (void*)(&pid), sizeof(pid));
      |             ~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../github/wayfire/wayfire.git/src/core/core.cpp:479:13: warning: ignoring return value of ‘ssize_t read(int, void*, size_t)’ declared with attribute ‘warn_unused_result’ [-Wunused-result]
  479 |         read(pipe_fd[READ_END], &child_pid, sizeof(child_pid));
      |         ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

In order to "handle" a failure to run a command, I'd normally throw
an exception that is caught up stream and pops up an informative window
to the user describing the error in detail... but this code is kinda
C-like, so I opted to just return 0 as the `pid_t`. The actual PID of
a client is NEVER 0, so we can use this perfectly to detect failure.

Why handling failure is correct:

- If pipe2 would fail (which is possible), it leaves `pipe_fd`
  untouched and the rest of the code would use uninitialed memory
  and have Undefined Behaviour.

- If write would fail, then the command just failed: we can't
  report back a PID.

- Same for the read: if we fail to read the PID then we can't return
  its value. Returning 0 and handling that as an error (failure to
  execute the command) makes sense.

- The return value of the executed script was also completely ignored.
  If the command being run return an non-zero status than now the command
  is also treated as having failed. It is also possible to never even get
  to the _exit inside the child. To detect those, WIFEXITED(status) &&
  WEXITSTATUS(status) == 0 are being used.

More sophisticated error handling is possible, but unnecessary: failure
never happened so far (apparently) and is just very unlikely in general
(apart from perhaps trying to execute non-existing scripts), so what we
exactly do isn't that important (as long as I got rid of the pesky
compiler warnings). Treating every error as a failure to execute the
command is OK.

Given that `wf::get_core().run(...)` can now return 0 with the meaning
that executing the command failed, we also need to handle that
gracefully, of course. Hence that `command_callback` now returns a bool
and converts the pid_t into a bool (so that zero now means failure)
whenever `wf::compositor_core_impl_t::run` is executed.

plugins/ipc/ipc.cpp, plugins/ipc/ipc.hpp, plugins/ipc/ipc-method-repository.hpp:
    `wf::ipc::client_t::send_json` now returns a bool (success).

plugins/single_plugins/command.cpp:
    `command_callback` now returns a bool (success).

plugins/ipc/stipc.cpp (stipc_plugin_t::run):
    Do not report a PID of zero as if that is the PID.

src/core/core.cpp:
    handle syscall errors and return (pid_t)0 upon failure.

* Address #2575 (comment)

* Address #2575 (comment)

And #2575 (comment)

plugins/ipc/ipc-method-repository.hpp

@@ -16,7 +16,7 @@ namespace ipc
 class client_interface_t
 {
   public:
-    virtual void send_json(nlohmann::json json) = 0;
+    virtual bool send_json(nlohmann::json json) = 0; // Returns true upon success.
     virtual ~client_interface_t() = default;
 };
 

plugins/ipc/ipc.cpp

@@ -295,23 +295,25 @@ static bool write_exact(int fd, char *buf, ssize_t n)
     return true;
 }
 
-void wf::ipc::client_t::send_json(nlohmann::json json)
+bool wf::ipc::client_t::send_json(nlohmann::json json)
 {
     std::string serialized = json.dump(-1, ' ', false, nlohmann::detail::error_handler_t::ignore);
     if (serialized.length() > MAX_MESSAGE_LEN)
     {
         LOGE("Error sending json to client: message too long!");
         shutdown(fd, SHUT_RDWR);
-        return;
+        return false;
     }
 
     uint32_t len = serialized.length();
     if (!write_exact(fd, (char*)&len, 4) || !write_exact(fd, serialized.data(), len))
     {
         LOGE("Error sending json to client!");
         shutdown(fd, SHUT_RDWR);
-        return;
+        return false;
     }
+
+    return true;
 }
 
 namespace wf

plugins/ipc/ipc.hpp

@@ -20,7 +20,7 @@ class client_t : public client_interface_t
   public:
     client_t(server_t *server, int client_fd);
     ~client_t();
-    void send_json(nlohmann::json json) override;
+    bool send_json(nlohmann::json json) override;
 
   private:
     int fd;

plugins/ipc/stipc.cpp

@@ -533,7 +533,13 @@ class stipc_plugin_t : public wf::plugin_interface_t
         }
 
         auto response = wf::ipc::json_ok();
-        response["pid"] = wf::get_core().run(data["cmd"]);
+        pid_t pid     = wf::get_core().run(data["cmd"]);
+        if (!pid)
+        {
+            return wf::ipc::json_error("failed to run command");
+        }
+
+        response["pid"] = pid;
         return response;
     };
 

plugins/single_plugins/command.cpp

@@ -63,7 +63,7 @@ class wayfire_command : public wf::plugin_interface_t
     }
 
     std::list<ipc_binding_t> ipc_bindings;
-    using command_callback = std::function<void ()>;
+    using command_callback = std::function<bool ()>;
 
     struct
     {
@@ -264,7 +264,7 @@ class wayfire_command : public wf::plugin_interface_t
             for (const auto& [_, _cmd, activator] : list)
             {
                 std::string cmd     = _cmd;
-                command_callback cb = [cmd] () { wf::get_core().run(cmd); };
+                command_callback cb = [cmd] () -> bool { return wf::get_core().run(cmd); };
                 bindings[i] =
                     std::bind(std::mem_fn(&wayfire_command::on_binding), this, cb, mode, always_exec, _1);
                 wf::get_core().bindings->add_activator(wf::create_option(activator), &bindings[i]);
@@ -366,31 +366,32 @@ class wayfire_command : public wf::plugin_interface_t
         {
             act_callback = [=] (const wf::activator_data_t& data)
             {
-                return on_binding([js, this] ()
+                return on_binding([js, this] () -> bool
                 {
                     method_repository->call_method(js["call-method"], js["call-data"]);
+                    return true;
                 }, mode, exec_always, data);
             };
         } else if (js.contains("command"))
         {
             act_callback = [=] (const wf::activator_data_t& data)
             {
-                return on_binding([js] ()
+                return on_binding([js] () -> bool
                 {
-                    wf::get_core().run(js["command"]);
+                    return wf::get_core().run(js["command"]);
                 }, mode, exec_always, data);
             };
         } else
         {
             temporary_binding = true;
             act_callback = [=] (const wf::activator_data_t& data)
             {
-                return on_binding([client, id] ()
+                return on_binding([client, id] () -> bool
                 {
                     nlohmann::json event;
                     event["event"] = "command-binding";
                     event["binding-id"] = id;
-                    client->send_json(event);
+                    return client->send_json(event);
                 }, mode, exec_always, data);
             };
         }

src/core/core.cpp

@@ -424,17 +424,26 @@ std::vector<wayfire_view> wf::compositor_core_t::get_all_views()
     return wf::tracking_allocator_t<view_interface_t>::get().get_all();
 }
 
+/**
+ * Upon successful execution, returns the PID of the child process.
+ * Returns 0 in case of failure.
+ */
 pid_t wf::compositor_core_impl_t::run(std::string command)
 {
     static constexpr size_t READ_END  = 0;
     static constexpr size_t WRITE_END = 1;
-    pid_t pid;
+
     int pipe_fd[2];
-    pipe2(pipe_fd, O_CLOEXEC);
+    int ret = pipe2(pipe_fd, O_CLOEXEC);
+    if (ret == -1)
+    {
+        LOGE("wf::compositor_core_impl_t::run: failed to create pipe2: ", strerror(errno));
+        return 0;
+    }
 
     /* The following is a "hack" for disowning the child processes,
      * otherwise they will simply stay as zombie processes */
-    pid = fork();
+    pid_t pid = fork();
     if (!pid)
     {
         pid = fork();
@@ -464,9 +473,9 @@ pid_t wf::compositor_core_impl_t::run(std::string command)
         } else
         {
             close(pipe_fd[READ_END]);
-            write(pipe_fd[WRITE_END], (void*)(&pid), sizeof(pid));
+            int ret = write(pipe_fd[WRITE_END], (void*)(&pid), sizeof(pid));
             close(pipe_fd[WRITE_END]);
-            _exit(0);
+            _exit(ret != sizeof(pid) ? 1 : 0);
         }
     } else
     {
@@ -475,8 +484,28 @@ pid_t wf::compositor_core_impl_t::run(std::string command)
         int status;
         waitpid(pid, &status, 0);
 
-        pid_t child_pid;
-        read(pipe_fd[READ_END], &child_pid, sizeof(child_pid));
+        // Return 0 if the child process didn't run or didn't exit normally, or returns a non-zero return
+        // value.
+        pid_t child_pid{};
+        if (WIFEXITED(status) && (WEXITSTATUS(status) == 0))
+        {
+            int ret = read(pipe_fd[READ_END], &child_pid, sizeof(child_pid));
+            if (ret != sizeof(child_pid))
+            {
+                // This is consider to be an error (even though theoretically a partial read would require an
+                // attempt to continue).
+                child_pid = 0;
+                if (ret == -1)
+                {
+                    LOGE("wf::compositor_core_impl_t::run(\"", command,
+                        "\"): failed to read PID from pipe end: ", strerror(errno));
+                } else
+                {
+                    LOGE("wf::compositor_core_impl_t::run(\"", command,
+                        "\"): short read of PID from pipe end, got ", std::to_string(ret), " bytes");
+                }
+            }
+        }
 
         close(pipe_fd[READ_END]);