interop: fix issues in IPC message construction (#9035)

* interop: fix issues in IPC message construction

- simplify logic
- handle exceptions to prevent crashes
- log errors

* Update src/runner/settings_window.cpp

Co-authored-by: Enrico Giordani <enricogior@users.noreply.github.com>

* Update src/runner/settings_window.cpp

Co-authored-by: Enrico Giordani <enricogior@users.noreply.github.com>

* fixup

* comments + fix build

Co-authored-by: Enrico Giordani <enricogior@users.noreply.github.com>

src/common/interop/two_way_pipe_message_ipc.cpp

@@ -3,6 +3,8 @@
 
 #include <iterator>
 
+constexpr DWORD BUFSIZE = 1024;
+
 TwoWayPipeMessageIPC::TwoWayPipeMessageIPC(
     std::wstring _input_pipe_name,
     std::wstring _output_pipe_name,
@@ -347,70 +349,43 @@ HANDLE TwoWayPipeMessageIPC::TwoWayPipeMessageIPCImpl::create_medium_integrity_t
 
 void TwoWayPipeMessageIPC::TwoWayPipeMessageIPCImpl::handle_pipe_connection(HANDLE input_pipe_handle)
 {
-    //Adapted from https://docs.microsoft.com/en-us/windows/win32/ipc/multithreaded-pipe-server
+    if (!input_pipe_handle)
-    HANDLE hHeap = GetProcessHeap();
-    uint8_t* pchRequest = (uint8_t*)HeapAlloc(hHeap, 0, BUFSIZE * sizeof(uint8_t));
-
-    DWORD cbBytesRead = 0, cbReplyBytes = 0, cbWritten = 0;
-    BOOL fSuccess = FALSE;
-
-    // Do some extra error checking since the app will keep running even if this thread fails.
-    std::list<std::vector<uint8_t>> message_parts;
-
-    if (input_pipe_handle == NULL)
-    {
-        if (pchRequest != NULL)
-            HeapFree(hHeap, 0, pchRequest);
-        return;
-    }
-
-    if (pchRequest == NULL)
     {
         return;
     }
-
+    constexpr DWORD readBlockBytes = BUFSIZE;
-    // Loop until done reading
+    std::wstring message;
+    size_t iBlock = 0;
+    message.reserve(BUFSIZE);
+    bool ok;
     do
     {
-        // Read client requests from the pipe. This simplistic code only allows messages
+        constexpr size_t charsPerBlock = readBlockBytes / sizeof(message[0]);
-        // up to BUFSIZE characters in length.
+        message.resize(message.size() + charsPerBlock);
-        ZeroMemory(pchRequest, BUFSIZE * sizeof(uint8_t));
+        DWORD bytesRead = 0;
-        fSuccess = ReadFile(
+        ok = ReadFile(
-            input_pipe_handle, // handle to pipe
+            input_pipe_handle,
-            pchRequest, // buffer to receive data
+            // read the message directly into the string block by block simultaneously resizing it
-            BUFSIZE * sizeof(uint8_t), // size of buffer
+            message.data() + iBlock * charsPerBlock,
-            &cbBytesRead, // number of bytes read
+            readBlockBytes,
-            NULL); // not overlapped I/O
+            &bytesRead,
+            nullptr);
 
-        if (!fSuccess && GetLastError() != ERROR_MORE_DATA)
+        if (!ok && GetLastError() != ERROR_MORE_DATA)
         {
             break;
         }
-        std::vector<uint8_t> part_vector;
+        iBlock++;
-        part_vector.reserve(cbBytesRead);
+    } while (!ok);
-        std::copy(pchRequest, pchRequest + cbBytesRead, std::back_inserter(part_vector));
+    // trim the message's buffer
-        message_parts.push_back(part_vector);
+    const auto nullCharPos = message.find_last_not_of(L'\0');
-    } while (!fSuccess);
+    if (nullCharPos != std::wstring::npos)
-
-    if (fSuccess)
     {
-        // Reconstruct the total_message.
+        message.resize(nullCharPos + 1);
-        std::vector<uint8_t> reconstructed_message;
-        size_t total_size = 0;
-        for (auto& part_vector : message_parts)
-        {
-            total_size += part_vector.size();
-        }
-        reconstructed_message.reserve(total_size);
-        for (auto& part_vector : message_parts)
-        {
-            std::move(part_vector.begin(), part_vector.end(), std::back_inserter(reconstructed_message));
-        }
-        std::wstring unicode_msg;
-        unicode_msg.assign(reinterpret_cast<std::wstring::const_pointer>(reconstructed_message.data()), reconstructed_message.size() / sizeof(std::wstring::value_type));
-        input_queue.queue_message(unicode_msg);
     }
 
+    input_queue.queue_message(std::move(message));
+
     // Flush the pipe to allow the client to read the pipe's contents
     // before disconnecting. Then disconnect the pipe, and close the
     // handle to this pipe instance.
@@ -418,10 +393,6 @@ void TwoWayPipeMessageIPC::TwoWayPipeMessageIPCImpl::handle_pipe_connection(HAND
     FlushFileBuffers(input_pipe_handle);
     DisconnectNamedPipe(input_pipe_handle);
     CloseHandle(input_pipe_handle);
-
-    HeapFree(hHeap, 0, pchRequest);
-
-    printf("InstanceThread exiting.\n");
 }
 
 void TwoWayPipeMessageIPC::TwoWayPipeMessageIPCImpl::start_named_pipe_server(HANDLE token)

src/common/interop/two_way_pipe_message_ipc_impl.h

@@ -29,7 +29,6 @@ private:
     HANDLE current_connect_pipe_handle = NULL;
     bool closed = false;
     TwoWayPipeMessageIPC::callback_function dispatch_inc_message_function;
-    const DWORD BUFSIZE = 1024;
 
     void send_pipe_message(std::wstring message);
     void consume_output_queue_thread();

src/runner/settings_window.cpp

@@ -41,7 +41,7 @@ json::JsonObject get_power_toys_settings()
         }
         catch (...)
         {
-            // TODO: handle malformed JSON.
+            Logger::error("get_power_toys_settings: got malformed json");
         }
     }
     return result;
@@ -154,7 +154,14 @@ void dispatch_json_config_to_modules(const json::JsonObject& powertoys_configs)
 
 void dispatch_received_json(const std::wstring& json_to_parse)
 {
-    const json::JsonObject j = json::JsonObject::Parse(json_to_parse);
+    json::JsonObject j;
+    const bool ok = json::JsonObject::TryParse(json_to_parse, j);
+    if (!ok)
+    {
+        Logger::error(L"dispatch_received_json: got malformed json: {}", json_to_parse);
+        return;
+    }
+
     for (const auto& base_element : j)
     {
         if (!current_settings_ipc)