From 5a0a7f8254f7cd7cb00abecff81541cbdeeb9b31 Mon Sep 17 00:00:00 2001 From: jrd Date: Mon, 7 Sep 2026 16:31:45 +0000 Subject: [PATCH] Add JSON-RPC client connection control New methods: jamulusclient/requestConnection, jamulusclient/disconnect and jamulusclient/getConnectionState. New notification: jamulusclient/connectionStateChanged, carrying state and serverName and, on a failed attempt, error. Together with the existing connected/disconnected notifications this gives JSON-RPC full parity with the UI for joining and leaving servers (#3801) and makes a --nogui client fully scriptable. requestConnection returns "ok" as soon as the attempt is initiated, not when it succeeds, so the name reflects a request; it reads symmetrically with getConnectionState. An address that cannot be resolved is answered with an invalid-params error and leaves the current connection untouched: the address is resolved before CClient::Connect() disconnects, and the resolved CHostAddress is handed to a new Connect() overload that runs the lifecycle. The GUI and the startup path go through the string overload and get the same protection. NetworkUtil::ParseNetworkAddress reports an SRV record whose target is "." as invalid instead of handing back a null address. SetServerAddr() had no caller left and is removed. The failure notification reports the state and server name the client is actually in, not a fixed "disconnected": a resolve failure from the GUI while connected leaves the connection in place, and subscribers must not be told otherwise. jamulusclient/disconnect also cancels a pending attempt; its documentation says so. Crash fix: CClient::Stop() ran the event loop (QCoreApplication::processEvents) for its 100 ms settle. Called from the connect/disconnect handlers, that re-entered the JSON-RPC readyRead handler while it was still on the stack and freed a socket that was then written to: a use-after-free (stack overflow under overlapping requests). The settle is kept but done with QThread::msleep, so no events are pumped and no re-entrancy is possible. Verified with AddressSanitizer on Linux and macOS: overlapping connect/disconnect plus 8-thread churn crash before this change and are clean after; a plain deferral and a re-entrancy guard were each insufficient. Measured over JSON-RPC on the headless client: a bad address while connected returns -32602 and the session stays up; a bad address while disconnected emits no notification. docs/JSON-RPC.md regenerated. CHANGELOG: fix a client crash when connect/disconnect are driven over JSON-RPC Co-Authored-By: Claude Fable 5 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PEyPQdxy6h7vQtUDCh2KY2 --- docs/JSON-RPC.md | 66 +++++++++++++++++++++++++++ src/client.cpp | 69 +++++++++++----------------- src/client.h | 2 +- src/clientrpc.cpp | 112 ++++++++++++++++++++++++++++++++++++++++++++++ src/util.cpp | 4 +- 5 files changed, 208 insertions(+), 45 deletions(-) diff --git a/docs/JSON-RPC.md b/docs/JSON-RPC.md index 3501fbdc9d..da486b3474 100644 --- a/docs/JSON-RPC.md +++ b/docs/JSON-RPC.md @@ -129,6 +129,23 @@ Results: | result.version | string | The Jamulus version. | +### jamulusclient/disconnect + +Disconnects the client from the current server, or cancels a pending connection attempt. Does nothing if the client is disconnected. + +Parameters: + +| Name | Type | Description | +| --- | --- | --- | +| params | object | No parameters (empty object). | + +Results: + +| Name | Type | Description | +| --- | --- | --- | +| result | string | Always "ok". | + + ### jamulusclient/getChannelInfo Returns the client's profile information. @@ -188,6 +205,24 @@ Results: | result.clients | array | The client list. See jamulusclient/clientListReceived for the format. | +### jamulusclient/getConnectionState + +Returns the current connection state. + +Parameters: + +| Name | Type | Description | +| --- | --- | --- | +| params | object | No parameters (empty object). | + +Results: + +| Name | Type | Description | +| --- | --- | --- | +| result.state | string | The connection state (disconnected, connecting, or connected). | +| result.serverName | string | The human readable name of the current server (empty if disconnected). | + + ### jamulusclient/getCurrentDirectory Returns the currently selected directory socket address. @@ -256,6 +291,24 @@ Results: | result | string | "ok" or "error" if bad arguments. | +### jamulusclient/requestConnection + +Connects the client to a server. Any current connection is terminated first. The connection is established asynchronously: subscribe to the jamulusclient/connected and jamulusclient/connectionStateChanged notifications to follow its progress (a failed attempt arrives as connectionStateChanged with an error field). An address that cannot be resolved is rejected with an error and leaves the current connection untouched. + +Parameters: + +| Name | Type | Description | +| --- | --- | --- | +| params.address | string | Socket address of the server (host:port). | +| params.serverName | string | Optional human readable server name used for display purposes; if given it must be a string (null counts as omitted). Defaults to the address. | + +Results: + +| Name | Type | Description | +| --- | --- | --- | +| result | string | "ok" once the connection attempt has been initiated. | + + ### jamulusclient/sendChatText Sends a chat text message. @@ -656,6 +709,19 @@ Parameters: | params.id | number | The channel ID assigned to the client. | +### jamulusclient/connectionStateChanged + +Emitted whenever the connection state changes, and on a failed connection attempt, which adds an error field and reports the state the client is left in. + +Parameters: + +| Name | Type | Description | +| --- | --- | --- | +| params.state | string | The connection state (disconnected, connecting, or connected). | +| params.serverName | string | The human readable server name (empty when disconnected). | +| params.error | string | Only present on a failed connection attempt. | + + ### jamulusclient/disconnected Emitted when the client is disconnected from the server. diff --git a/src/client.cpp b/src/client.cpp index aa36cacb3f..5c23b027d0 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -45,6 +45,7 @@ \******************************************************************************/ #include "client.h" +#include #include "settings.h" #include "util.h" @@ -619,25 +620,6 @@ void CClient::SetRemoteChanPan ( const int iId, const float fPan ) StartTimerGainOrPan(); } -bool CClient::SetServerAddr ( QString strNAddr ) -{ - CHostAddress HostAddress; - if ( NetworkUtil::ParseNetworkAddress ( strNAddr, HostAddress, bIPv6Available ) ) - { - // apply address to the channel - Channel.SetAddress ( HostAddress ); - - // By default, set server name to HostAddress. If using the Connect() method, this may be overwritten - SetConnectedServerName ( HostAddress.toString() ); - - return true; - } - else - { - return false; // invalid address - } -} - bool CClient::GetAndResetbJitterBufferOKFlag() { // get the socket buffer put status flag and reset it @@ -1108,19 +1090,12 @@ void CClient::Stop() qWarning() << "Could not reinitialise the sound device while disconnecting:" << generr.GetErrorText(); } - // wait for approx. 100 ms to make sure no audio packet is still in the - // network queue causing the channel to be reconnected right after having - // received the disconnect message (seems not to gain much, disconnect is - // still not working reliably) - QTime DieTime = QTime::currentTime().addMSecs ( 100 ); - while ( QTime::currentTime() < DieTime ) - { - // exclude user input events because if we use AllEvents, it happens - // that if the user initiates a connection and disconnection quickly - // (e.g. quickly pressing enter five times), the software can get into - // an unknown state - QCoreApplication::processEvents ( QEventLoop::ExcludeUserInputEvents, 100 ); - } + // Wait ~100 ms so no audio packet is still in the network queue causing the + // channel to be reconnected right after the disconnect message. We must NOT + // run the event loop to do this: pumping events here re-entered the JSON-RPC + // read handler and freed a socket still being written to (use-after-free). A + // plain sleep keeps the settle without re-entrancy. + QThread::msleep ( 100 ); // Send disconnect message to server (Since we disable our protocol // receive mechanism with the next command, we do not evaluate any @@ -1162,13 +1137,28 @@ void CClient::Disconnect() /// @method /// @brief Connects to strServerAddress. If a connection is currently requested /// or established, that connection is terminated first. -/// @emit Connecting (strServerName) if SetServerAddr was valid. emit happens through Start(). +/// @emit Connecting (strServerName) if the address resolved. emit happens through Start(). /// Use to set CClientDlg to show being connected /// @emit ConnectingFailed (error) if an error occurred /// Use to display error message in CClientDlg /// @param strServerAddress - the server address to connect to /// @param strServerName - the human readable server name passed to Connecting() void CClient::Connect ( const QString& strServerAddress, const QString& strServerName ) +{ + // resolve before touching the current connection, so that an invalid + // address leaves it in place + CHostAddress HostAddress; + + if ( !NetworkUtil::ParseNetworkAddress ( strServerAddress, HostAddress, bIPv6Available ) ) + { + emit ConnectingFailed ( tr ( "Received invalid server address. Please check for typos in the provided server address." ) ); + return; + } + + Connect ( HostAddress, strServerName ); +} + +void CClient::Connect ( const CHostAddress& HostAddress, const QString& strServerName ) { try { @@ -1176,16 +1166,9 @@ void CClient::Connect ( const QString& strServerAddress, const QString& strServe // different server while connected behaves as a reconnect Disconnect(); - // Set server address and connect if valid address was supplied - if ( SetServerAddr ( strServerAddress ) ) - { - SetConnectedServerName ( strServerName ); - Start(); - } - else - { - throw CGenErr ( tr ( "Received invalid server address. Please check for typos in the provided server address." ) ); - } + Channel.SetAddress ( HostAddress ); + SetConnectedServerName ( strServerName ); + Start(); } catch ( const CGenErr& generr ) { diff --git a/src/client.h b/src/client.h index 1c4c4e5428..b1539bc796 100644 --- a/src/client.h +++ b/src/client.h @@ -171,6 +171,7 @@ class CClient : public QObject void Disconnect(); void Connect ( const QString& strServerAddress, const QString& strServerName ); + void Connect ( const CHostAddress& HostAddress, const QString& strServerName ); // The ConnectedServerName is emitted by Connecting() to update the UI with a human readable server name void SetConnectedServerName ( const QString& strServerName ) { strConnectedServerName = strServerName; }; @@ -180,7 +181,6 @@ class CClient : public QObject bool IsRunning() { return Sound.IsRunning(); } bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); } - bool SetServerAddr ( QString strNAddr ); // IPv6 Available bool IsIPv6Available() { return bIPv6Available; } diff --git a/src/clientrpc.cpp b/src/clientrpc.cpp index 0f376d10b4..1d4e669e52 100644 --- a/src/clientrpc.cpp +++ b/src/clientrpc.cpp @@ -47,6 +47,21 @@ #include "clientrpc.h" +static QString ConnectionStateToString ( const EConnectionState eState ) +{ + switch ( eState ) + { + case CS_CONNECTING: + return "connecting"; + + case CS_CONNECTED: + return "connected"; + + default: + return "disconnected"; + } +} + CClientRpc::CClientRpc ( CClient* pClient, CClientSettings* pSettings, CRpcServer* pRpcServer, QObject* parent ) : QObject ( parent ), m_pSettings ( pSettings ) @@ -168,6 +183,33 @@ CClientRpc::CClientRpc ( CClient* pClient, CClientSettings* pSettings, CRpcServe /// @param {object} params - No parameters (empty object). connect ( pClient, &CClient::Disconnected, [=]() { pRpcServer->BroadcastNotification ( "jamulusclient/disconnected", QJsonObject{} ); } ); + // A failed attempt surfaces through connectionStateChanged with the error attached. The + // state and name are the current ones: an address that fails to resolve leaves any + // existing connection in place. + connect ( pClient, &CClient::ConnectingFailed, [=] ( QString strError ) { + const EConnectionState eState = pClient->GetConnectionState(); + pRpcServer->BroadcastNotification ( "jamulusclient/connectionStateChanged", + QJsonObject{ + { "state", ConnectionStateToString ( eState ) }, + { "serverName", eState == CS_DISCONNECTED ? QString() : pClient->GetConnectedServerName() }, + { "error", strError }, + } ); + } ); + + /// @rpc_notification jamulusclient/connectionStateChanged + /// @brief Emitted whenever the connection state changes, and on a failed connection attempt, + /// which adds an error field and reports the state the client is left in. + /// @param {string} params.state - The connection state (disconnected, connecting, or connected). + /// @param {string} params.serverName - The human readable server name (empty when disconnected). + /// @param {string} params.error - Only present on a failed connection attempt. + connect ( pClient, &CClient::ConnectionStateChanged, [=] ( EConnectionState eState ) { + pRpcServer->BroadcastNotification ( "jamulusclient/connectionStateChanged", + QJsonObject{ + { "state", ConnectionStateToString ( eState ) }, + { "serverName", eState == CS_DISCONNECTED ? QString() : pClient->GetConnectedServerName() }, + } ); + } ); + /// @rpc_notification jamulusclient/recorderState /// @brief Emitted when the client is connected to a server whose recorder state changes. /// @param {number} params.state - The recorder state. @@ -212,6 +254,76 @@ CClientRpc::CClientRpc ( CClient* pClient, CClientSettings* pSettings, CRpcServe Q_UNUSED ( params ); } ); + /// @rpc_method jamulusclient/requestConnection + /// @brief Connects the client to a server. Any current connection is terminated first. + /// The connection is established asynchronously: subscribe to the jamulusclient/connected + /// and jamulusclient/connectionStateChanged notifications to follow its progress (a failed + /// attempt arrives as connectionStateChanged with an error field). An address that cannot + /// be resolved is rejected with an error and leaves the current connection untouched. + /// @param {string} params.address - Socket address of the server (host:port). + /// @param {string} params.serverName - Optional human readable server name used for display purposes; if given it must be a string + /// (null counts as omitted). Defaults to the address. + /// @result {string} result - "ok" once the connection attempt has been initiated. + pRpcServer->HandleMethod ( "jamulusclient/requestConnection", [=] ( const QJsonObject& params, QJsonObject& response ) { + auto jsonAddress = params["address"]; + if ( !jsonAddress.isString() ) + { + response["error"] = CRpcServer::CreateJsonRpcError ( CRpcServer::iErrInvalidParams, "Invalid params: address is not a string" ); + return; + } + + auto jsonServerName = params["serverName"]; + if ( !jsonServerName.isUndefined() && !jsonServerName.isNull() && !jsonServerName.isString() ) + { + response["error"] = CRpcServer::CreateJsonRpcError ( CRpcServer::iErrInvalidParams, "Invalid params: serverName is not a string" ); + return; + } + + const QString strAddress = NetworkUtil::FixAddress ( jsonAddress.toString() ); + const QString strServerName = jsonServerName.isString() ? jsonServerName.toString() : strAddress; + + // resolve here so that the caller gets an error result for an invalid address + CHostAddress haServer; + if ( !NetworkUtil::ParseNetworkAddress ( strAddress, haServer, pClient->IsIPv6Available() ) ) + { + response["error"] = + CRpcServer::CreateJsonRpcError ( CRpcServer::iErrInvalidParams, "Invalid params: address is not a valid socket address" ); + return; + } + + pClient->Connect ( haServer, strServerName ); + + response["result"] = "ok"; + } ); + + /// @rpc_method jamulusclient/disconnect + /// @brief Disconnects the client from the current server, or cancels a pending connection attempt. Does nothing if the client is + /// disconnected. + /// @param {object} params - No parameters (empty object). + /// @result {string} result - Always "ok". + pRpcServer->HandleMethod ( "jamulusclient/disconnect", [=] ( const QJsonObject& params, QJsonObject& response ) { + pClient->Disconnect(); + + response["result"] = "ok"; + Q_UNUSED ( params ); + } ); + + /// @rpc_method jamulusclient/getConnectionState + /// @brief Returns the current connection state. + /// @param {object} params - No parameters (empty object). + /// @result {string} result.state - The connection state (disconnected, connecting, or connected). + /// @result {string} result.serverName - The human readable name of the current server (empty if disconnected). + pRpcServer->HandleMethod ( "jamulusclient/getConnectionState", [=] ( const QJsonObject& params, QJsonObject& response ) { + const EConnectionState eState = pClient->GetConnectionState(); + + QJsonObject result{ + { "state", ConnectionStateToString ( eState ) }, + { "serverName", eState == CS_DISCONNECTED ? QString() : pClient->GetConnectedServerName() }, + }; + response["result"] = result; + Q_UNUSED ( params ); + } ); + /// @rpc_method jamulus/getMode /// @brief Returns the current mode, i.e. whether Jamulus is running as a server or client. /// @param {object} params - No parameters (empty object). diff --git a/src/util.cpp b/src/util.cpp index 39efa10a16..ddc92377a8 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -944,7 +944,9 @@ bool NetworkUtil::ParseNetworkAddress ( QString strAddress, CHostAddress& HostAd // Try SRV-based discovery first: if ( ParseNetworkAddressSrv ( strAddress, HostAddress, bIPv6Available ) ) { - return true; + // an SRV target of "." means the service is not offered: fail here + // rather than falling back to a host lookup + return !HostAddress.InetAddr.isNull(); } #endif // Try regular connect via plain IP or host name lookup (A/AAAA):