[IPC] Don't unwrap invalid SendSyncResult replies

https://bugs.webkit.org/show_bug.cgi?id=259301
rdar://111895837

Reviewed by Dean Jackson.

We have a number of crash reports from an assert firing in std::optional when
unwrapping the result of a synchronous IPC call. This assert hints that we have
received a result where `succeeded()` returns true, yet we don't have a reply
payload in replyArguments. This is a violation of the prerequisites for
`ConnectionSendSyncReply`. This issue has been causes by improper handling of
`decoder` failure and has been fixed piecemeal, for example in
https://bugs.webkit.org/show_bug.cgi?id=259006.

This change extends the succeeded check to include checking for non-none
replyArguments to avoid asserting when using `if (sendResult.succeeded()) {
... = sendResult.reply(); }` pattern.

As an extra level of protection, the new ConnecttionSendSyncResult will set
error to Error::Unspecified if passed replyArguments that are none.

* Source/WebKit/Platform/IPC/Connection.h:
(IPC::ConnectionSendSyncResult::ConnectionSendSyncResult):
(IPC::ConnectionSendSyncResult::succeeded const):
(IPC::Connection::sendSync):
* Source/WebKit/Platform/IPC/MessageSenderInlines.h:
(IPC::MessageSender::sendSync):
* Source/WebKit/Platform/IPC/StreamClientConnection.h:
(IPC::StreamClientConnection::sendSync):
(IPC::StreamClientConnection::trySendSyncStream):
* Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:
(WebKit::AuxiliaryProcessProxy::sendSync):

Canonical link: https://commits.webkit.org/266147@main

Source/WebKit/Platform/IPC/Connection.h

@@ -160,7 +160,20 @@ template<typename T> struct ConnectionSendSyncResult {
     std::optional<typename T::ReplyArguments> replyArguments;
     Error error { Error::NoError };
 
-    bool succeeded() const { return error == Error::NoError; }
+    ConnectionSendSyncResult(Error error)
+        : error(error)
+    {
+        ASSERT(error != Error::NoError);
+    }
+
+    ConnectionSendSyncResult(std::unique_ptr<Decoder>&& decoder, std::optional<typename T::ReplyArguments>&& replyArguments)
+        : decoder(WTFMove(decoder)), replyArguments(WTFMove(replyArguments))
+    {
+        ASSERT(this->replyArguments.has_value());
+        error = !this->replyArguments ? Error::Unspecified : Error::NoError;
+    }
+
+    bool succeeded() const { return error == Error::NoError && replyArguments.has_value(); }
 
     typename T::ReplyArguments& reply()
     {
@@ -707,16 +720,15 @@ template<typename T> Connection::SendSyncResult<T> Connection::sendSync(T&& mess
     auto replyDecoderOrError = sendSyncMessage(syncRequestID, WTFMove(encoder), timeout, sendSyncOptions);
     if (!replyDecoderOrError.decoder) {
         ASSERT(replyDecoderOrError.error != Error::NoError);
-        return { nullptr, std::nullopt, replyDecoderOrError.error };
+        return { replyDecoderOrError.error };
     }
 
-    SendSyncResult<T> result;
-    *replyDecoderOrError.decoder >> result.replyArguments;
-    if (!result.replyArguments)
-        return { nullptr, std::nullopt, Error::FailedToDecodeReplyArguments };
+    std::optional<typename T::ReplyArguments> replyArguments;
+    *replyDecoderOrError.decoder >> replyArguments;
+    if (!replyArguments)
+        return { Error::FailedToDecodeReplyArguments };
 
-    result.decoder = WTFMove(replyDecoderOrError.decoder);
-    return result;
+    return { WTFMove(replyDecoderOrError.decoder), WTFMove(replyArguments) };
 }
 
 template<typename T> Error Connection::waitForAndDispatchImmediately(uint64_t destinationID, Timeout timeout, OptionSet<WaitForOption> waitForOptions)

Source/WebKit/Platform/IPC/MessageSenderInlines.h

@@ -43,7 +43,7 @@ template<typename MessageType> inline auto MessageSender::sendSync(MessageType&&
     static_assert(MessageType::isSync);
     if (auto* connection = messageSenderConnection())
         return connection->sendSync(std::forward<MessageType>(message), destinationID, timeout, options);
-    return { nullptr, std::nullopt, Error::NoMessageSenderConnection };
+    return { Error::NoMessageSenderConnection };
 }
 
 template<typename MessageType, typename C> inline AsyncReplyID MessageSender::sendWithAsyncReply(MessageType&& message, C&& completionHandler, uint64_t destinationID, OptionSet<SendOption> options)

Source/WebKit/Platform/IPC/StreamClientConnection.h

@@ -203,11 +203,11 @@ StreamClientConnection::SendSyncResult<T> StreamClientConnection::sendSync(T&& m
     static_assert(T::isSync, "Message is not sync!");
     auto error = trySendDestinationIDIfNeeded(destinationID.toUInt64(), timeout);
     if (error != Error::NoError)
-        return { nullptr, std::nullopt, error };
+        return { error };
 
     auto span = m_buffer.tryAcquire(timeout);
     if (!span)
-        return { nullptr, std::nullopt, Error::FailedToAcquireBufferSpan };
+        return { Error::FailedToAcquireBufferSpan };
 
     if constexpr(T::isStreamEncodable) {
         auto maybeSendResult = trySendSyncStream(message, timeout, *span);
@@ -231,7 +231,7 @@ std::optional<StreamClientConnection::SendSyncResult<T>> StreamClientConnection:
     // std::nullopt means we couldn't send through the stream, so try sending out of stream.
     auto syncRequestID = m_connection->makeSyncRequestID();
     if (!m_connection->pushPendingSyncRequestID(syncRequestID))
-        return { { nullptr, std::nullopt, Error::CantWaitForSyncReplies } };
+        return { { Error::CantWaitForSyncReplies } };
 
     auto decoderResult = [&]() -> std::optional<Connection::DecoderOrError> {
         StreamConnectionEncoder messageEncoder { T::name(), span.data(), span.size() };
@@ -260,17 +260,15 @@ std::optional<StreamClientConnection::SendSyncResult<T>> StreamClientConnection:
     if (!decoderResult)
         return std::nullopt;
 
-    SendSyncResult<T> result;
     if (decoderResult->decoder) {
+        std::optional<typename T::ReplyArguments> replyArguments;
         auto& decoder = decoderResult->decoder;
-        *decoder >> result.replyArguments;
-        if (result.replyArguments)
-            result.decoder = WTFMove(decoder);
-        else
-            result.error = Error::FailedToDecodeReplyArguments;
+        *decoder >> replyArguments;
+        if (replyArguments)
+            return { { WTFMove(decoder), WTFMove(replyArguments) } };
+        return { Error::FailedToDecodeReplyArguments };
     } else
-        result.error = decoderResult->error;
-    return result;
+        return { decoderResult->error };
 }
 
 inline Error StreamClientConnection::trySendDestinationIDIfNeeded(uint64_t destinationID, Timeout timeout)

Source/WebKit/UIProcess/AuxiliaryProcessProxy.h

@@ -246,7 +246,7 @@ AuxiliaryProcessProxy::SendSyncResult<T> AuxiliaryProcessProxy::sendSync(T&& mes
     static_assert(T::isSync, "Sync message expected");
 
     if (!m_connection)
-        return { };
+        return { IPC::Error::InvalidConnection };
 
     TraceScope scope(SyncMessageStart, SyncMessageEnd);