diff --git a/src/core/net/srp_client.cpp b/src/core/net/srp_client.cpp index da697968c..96b647ed3 100644 --- a/src/core/net/srp_client.cpp +++ b/src/core/net/srp_client.cpp @@ -68,13 +68,19 @@ void Client::HostInfo::Clear(void) SetState(kRemoved); } -void Client::HostInfo::SetState(ItemState aState) +bool Client::HostInfo::SetState(ItemState aState) { - if (aState != GetState()) - { - LogInfo("HostInfo %s -> %s", ItemStateToString(GetState()), ItemStateToString(aState)); - mState = MapEnum(aState); - } + bool didChange; + + VerifyOrExit(aState != GetState(), didChange = false); + + LogInfo("HostInfo %s -> %s", ItemStateToString(GetState()), ItemStateToString(aState)); + + mState = MapEnum(aState); + didChange = true; + +exit: + return didChange; } void Client::HostInfo::EnableAutoAddress(void) @@ -121,9 +127,11 @@ exit: return error; } -void Client::Service::SetState(ItemState aState) +bool Client::Service::SetState(ItemState aState) { - VerifyOrExit(GetState() != aState); + bool didChange; + + VerifyOrExit(GetState() != aState, didChange = false); LogInfo("Service %s -> %s, \"%s\" \"%s\"", ItemStateToString(GetState()), ItemStateToString(aState), GetInstanceName(), GetName()); @@ -150,10 +158,11 @@ void Client::Service::SetState(ItemState aState) GetPriority(), GetNumTxtEntries()); } - mState = MapEnum(aState); + mState = MapEnum(aState); + didChange = true; exit: - return; + return didChange; } bool Client::Service::Matches(const Service &aOther) const @@ -249,7 +258,8 @@ Client::Client(Instance &aInstance) , mServiceKeyRecordEnabled(false) , mUseShortLeaseOption(false) #endif - , mUpdateMessageId(0) + , mNextMessageId(0) + , mResponseMessageId(0) , mAutoHostAddressCount(0) , mRetryWaitInterval(kMinRetryWaitInterval) , mTtl(0) @@ -345,6 +355,7 @@ void Client::Stop(Requester aRequester, StopMode aMode) mShouldRemoveKeyLease = false; mTxFailureRetryCount = 0; + mResponseMessageId = mNextMessageId; if (aMode == kResetRetryInterval) { @@ -739,13 +750,15 @@ exit: return; } -void Client::ChangeHostAndServiceStates(const ItemState *aNewStates, ServiceStateChangeMode aMode) +bool Client::ChangeHostAndServiceStates(const ItemState *aNewStates, ServiceStateChangeMode aMode) { + bool anyChanged; + #if OPENTHREAD_CONFIG_SRP_CLIENT_AUTO_START_API_ENABLE && OPENTHREAD_CONFIG_SRP_CLIENT_SAVE_SELECTED_SERVER_ENABLE ItemState oldHostState = mHostInfo.GetState(); #endif - mHostInfo.SetState(aNewStates[mHostInfo.GetState()]); + anyChanged = mHostInfo.SetState(aNewStates[mHostInfo.GetState()]); for (Service &service : mServices) { @@ -754,7 +767,7 @@ void Client::ChangeHostAndServiceStates(const ItemState *aNewStates, ServiceStat continue; } - service.SetState(aNewStates[service.GetState()]); + anyChanged |= service.SetState(aNewStates[service.GetState()]); } #if OPENTHREAD_CONFIG_SRP_CLIENT_AUTO_START_API_ENABLE && OPENTHREAD_CONFIG_SRP_CLIENT_SAVE_SELECTED_SERVER_ENABLE @@ -781,6 +794,8 @@ void Client::ChangeHostAndServiceStates(const ItemState *aNewStates, ServiceStat } } #endif // OPENTHREAD_CONFIG_SRP_CLIENT_AUTO_START_API_ENABLE && OPENTHREAD_CONFIG_SRP_CLIENT_SAVE_SELECTED_SERVER_ENABLE + + return anyChanged; } void Client::InvokeCallback(Error aError) const { InvokeCallback(aError, mHostInfo, nullptr); } @@ -806,6 +821,7 @@ void Client::SendUpdate(void) Error error = kErrorNone; Message *message = mSocket.NewMessage(); uint32_t length; + bool anyChanged; VerifyOrExit(message != nullptr, error = kErrorNoBufs); SuccessOrExit(error = PrepareUpdateMessage(*message)); @@ -822,14 +838,35 @@ void Client::SendUpdate(void) SuccessOrExit(error = mSocket.SendTo(*message, Ip6::MessageInfo())); - LogInfo("Send update"); + LogInfo("Send update, msg-id:0x%x", mNextMessageId); // State changes: // kToAdd -> kAdding // kToRefresh -> kRefreshing // kToRemove -> kRemoving - ChangeHostAndServiceStates(kNewStateOnMessageTx, kForServicesAppendedInMessage); + anyChanged = ChangeHostAndServiceStates(kNewStateOnMessageTx, kForServicesAppendedInMessage); + + // `mNextMessageId` tracks the message ID used in the prepared + // update message. It is incremented after a successful + // `mSocket.SendTo()` call. If unsuccessful, the same ID can be + // reused for the next update. + // + // Acceptable response message IDs fall within the range starting + // at `mResponseMessageId ` and ending before `mNextMessageId`. + // + // `anyChanged` tracks if any host or service states have changed. + // If not, the prepared message is identical to the last one with + // the same hosts/services, allowing us to accept earlier message + // IDs. If changes occur, `mResponseMessageId ` is updated to + // ensure only responses to the latest message are accepted. + + if (anyChanged) + { + mResponseMessageId = mNextMessageId; + } + + mNextMessageId++; // Remember the update message tx time to use later to determine the // lease renew time. @@ -902,13 +939,7 @@ Error Client::PrepareUpdateMessage(Message &aMessage) SuccessOrExit(error = ReadOrGenerateKey(info.mKeyPair)); #endif - // Generate random Message ID and ensure it is different from last one - do - { - SuccessOrExit(error = header.SetRandomMessageId()); - } while (header.GetMessageId() == mUpdateMessageId); - - mUpdateMessageId = header.GetMessageId(); + header.SetMessageId(mNextMessageId); // SRP Update (DNS Update) message must have exactly one record in // Zone section, no records in Prerequisite Section, can have @@ -1578,22 +1609,32 @@ void Client::ProcessResponse(Message &aMessage) uint16_t recordCount; LinkedList removedServices; - VerifyOrExit(GetState() == kStateUpdating); + switch (GetState()) + { + case kStateToUpdate: + case kStateUpdating: + case kStateToRetry: + break; + case kStateStopped: + case kStatePaused: + case kStateUpdated: + ExitNow(); + } SuccessOrExit(error = aMessage.Read(offset, header)); VerifyOrExit(header.GetType() == Dns::Header::kTypeResponse, error = kErrorParse); VerifyOrExit(header.GetQueryType() == Dns::Header::kQueryTypeUpdate, error = kErrorParse); - VerifyOrExit(header.GetMessageId() == mUpdateMessageId, error = kErrorDrop); + + VerifyOrExit(IsResponseMessageIdValid(header.GetMessageId()), error = kErrorDrop); + mResponseMessageId = header.GetMessageId() + 1; if (!Get().IsRxOnWhenIdle()) { Get().StopFastPolls(); } - // Response is for the earlier request message. - - LogInfo("Received response"); + LogInfo("Received response, msg-id:0x%x", header.GetMessageId()); #if OPENTHREAD_CONFIG_SRP_CLIENT_AUTO_START_API_ENABLE && OPENTHREAD_CONFIG_SRP_CLIENT_SWITCH_SERVER_ON_FAILURE mAutoStart.ResetTimeoutFailureCount(); @@ -1722,6 +1763,13 @@ exit: } } +bool Client::IsResponseMessageIdValid(uint16_t aId) const +{ + // Semantically equivalent to `(aId >= mResponseMessageId) && (aId < mNextMessageId)` + + return !SerialNumber::IsLess(aId, mResponseMessageId) && SerialNumber::IsLess(aId, mNextMessageId); +} + void Client::HandleUpdateDone(void) { HostInfo hostInfoCopy = mHostInfo; diff --git a/src/core/net/srp_client.hpp b/src/core/net/srp_client.hpp index 32b1ea0de..8c9cf7e3f 100644 --- a/src/core/net/srp_client.hpp +++ b/src/core/net/srp_client.hpp @@ -167,7 +167,7 @@ public: private: void SetName(const char *aName) { mName = aName; } - void SetState(ItemState aState); + bool SetState(ItemState aState); void SetAddresses(const Ip6::Address *aAddresses, uint8_t aNumAddresses); void EnableAutoAddress(void); }; @@ -303,7 +303,7 @@ public: static constexpr uint32_t kAppendedInMsgFlag = (1U << 31); static constexpr uint32_t kLeaseMask = ~kAppendedInMsgFlag; - void SetState(ItemState aState); + bool SetState(ItemState aState); TimeMilli GetLeaseRenewTime(void) const { return TimeMilli(mData); } void SetLeaseRenewTime(TimeMilli aTime) { mData = aTime.GetValue(); } bool IsAppendedInMessage(void) const { return mLease & kAppendedInMsgFlag; } @@ -788,6 +788,17 @@ public: * */ bool GetUseShortLeaseOption(void) const { return mUseShortLeaseOption; } + + /** + * Set the next DNS message ID for client to use. + * + * This is intended for testing only. + * + * @pram[in] aMessageId A message ID. + * + */ + void SetNextMessageId(uint16_t aMessageId) { mNextMessageId = aMessageId; } + #endif // OPENTHREAD_CONFIG_REFERENCE_DEVICE_ENABLE private: @@ -1007,7 +1018,7 @@ private: void UpdateServiceStateToRemove(Service &aService); State GetState(void) const { return mState; } void SetState(State aState); - void ChangeHostAndServiceStates(const ItemState *aNewStates, ServiceStateChangeMode aMode); + bool ChangeHostAndServiceStates(const ItemState *aNewStates, ServiceStateChangeMode aMode); void InvokeCallback(Error aError) const; void InvokeCallback(Error aError, const HostInfo &aHostInfo, const Service *aRemovedServices) const; void HandleHostInfoOrServiceChange(void); @@ -1031,6 +1042,7 @@ private: void UpdateRecordLengthInMessage(Dns::ResourceRecord &aRecord, uint16_t aOffset, Message &aMessage) const; static void HandleUdpReceive(void *aContext, otMessage *aMessage, const otMessageInfo *aMessageInfo); void ProcessResponse(Message &aMessage); + bool IsResponseMessageIdValid(uint16_t aId) const; void HandleUpdateDone(void); void GetRemovedServices(LinkedList &aRemovedServices); static Error ReadResourceRecord(const Message &aMessage, uint16_t &aOffset, Dns::ResourceRecord &aRecord); @@ -1073,7 +1085,8 @@ private: bool mUseShortLeaseOption : 1; #endif - uint16_t mUpdateMessageId; + uint16_t mNextMessageId; + uint16_t mResponseMessageId; uint16_t mAutoHostAddressCount; uint32_t mRetryWaitInterval; diff --git a/tests/unit/test_srp_server.cpp b/tests/unit/test_srp_server.cpp index 2104ae827..d381ee250 100644 --- a/tests/unit/test_srp_server.cpp +++ b/tests/unit/test_srp_server.cpp @@ -1024,6 +1024,187 @@ void TestUpdateLeaseShortVariant(void) Log("End of TestUpdateLeaseShortVariant"); } +static uint16_t sServerRxCount; +static Ip6::MessageInfo sServerMsgInfo; +static uint16_t sServerLastMsgId; + +void HandleServerUdpReceive(void *aContext, otMessage *aMessage, const otMessageInfo *aMessageInfo) +{ + Dns::Header header; + + VerifyOrQuit(aContext == nullptr); + VerifyOrQuit(aMessage != nullptr); + VerifyOrQuit(aMessageInfo != nullptr); + + SuccessOrQuit(AsCoreType(aMessage).Read(0, header)); + + sServerMsgInfo = AsCoreType(aMessageInfo); + sServerLastMsgId = header.GetMessageId(); + sServerRxCount++; + + Log("HandleServerUdpReceive(), message-id: 0x%x", header.GetMessageId()); +} + +void TestSrpClientDelayedResponse(void) +{ + static constexpr uint16_t kServerPort = 53535; + + Srp::Client *srpClient; + Srp::Client::Service service1; + Srp::Client::Service service2; + + Log("--------------------------------------------------------------------------------------------"); + Log("TestSrpClientDelayedResponse"); + + InitTest(); + + srpClient = &sInstance->Get(); + + for (uint8_t testIter = 0; testIter < 3; testIter++) + { + Log("- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -"); + Log("testIter = %u", testIter); + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // Prepare a socket to act as SRP server. + + Ip6::Udp::Socket udpSocket(*sInstance); + Ip6::SockAddr serverSockAddr; + uint16_t firstMsgId; + Message *response; + Dns::UpdateHeader header; + + sServerRxCount = 0; + + SuccessOrQuit(udpSocket.Open(HandleServerUdpReceive, nullptr)); + SuccessOrQuit(udpSocket.Bind(kServerPort, Ip6::kNetifThread)); + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // Manually start the client with a message ID based on `testIter` + // We use zero in the first iteration, `0xffff` in the second + // iteration to test wrapping of 16-bit message ID. + + switch (testIter) + { + case 0: + srpClient->SetNextMessageId(0); + break; + case 1: + srpClient->SetNextMessageId(0xffff); + break; + case 2: + srpClient->SetNextMessageId(0xaaaa); + break; + } + + serverSockAddr.SetAddress(sInstance->Get().GetMeshLocal16()); + serverSockAddr.SetPort(kServerPort); + SuccessOrQuit(srpClient->Start(serverSockAddr)); + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // Register a service + + SuccessOrQuit(srpClient->SetHostName(kHostName)); + SuccessOrQuit(srpClient->EnableAutoHostAddress()); + + PrepareService1(service1); + SuccessOrQuit(srpClient->AddService(service1)); + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // Wait for short time and make sure server receives an SRP + // update message from client. + + AdvanceTime(1 * 1000); + + VerifyOrQuit(sServerRxCount == 1); + firstMsgId = sServerLastMsgId; + + switch (testIter) + { + case 0: + VerifyOrQuit(firstMsgId == 0); + break; + case 1: + VerifyOrQuit(firstMsgId == 0xffff); + break; + case 2: + VerifyOrQuit(firstMsgId == 0xaaaa); + break; + } + + if (testIter == 2) + { + AdvanceTime(2 * 1000); + + PrepareService2(service2); + SuccessOrQuit(srpClient->AddService(service2)); + } + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // Wait for longer to allow client to retry a bunch of times + + AdvanceTime(20 * 1000); + VerifyOrQuit(sServerRxCount > 1); + VerifyOrQuit(sServerLastMsgId != firstMsgId); + + VerifyOrQuit(srpClient->GetHostInfo().GetState() != Srp::Client::kRegistered); + VerifyOrQuit(service1.GetState() != Srp::Client::kRegistered); + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // Now send a delayed response from server using the first + // message ID. + + response = udpSocket.NewMessage(); + VerifyOrQuit(response != nullptr); + + Log("Sending response with msg-id: 0x%x", firstMsgId); + + header.SetMessageId(firstMsgId); + header.SetType(Dns::UpdateHeader::kTypeResponse); + header.SetResponseCode(Dns::UpdateHeader::kResponseSuccess); + SuccessOrQuit(response->Append(header)); + SuccessOrQuit(udpSocket.SendTo(*response, sServerMsgInfo)); + + AdvanceTime(10); + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // In the first two iterations, we ensure that client + // did successfully accept the response with older message ID. + // This should not be the case in the third iteration due to + // changes to client services after first UPdate message was + // sent by client. + + switch (testIter) + { + case 0: + case 1: + VerifyOrQuit(srpClient->GetHostInfo().GetState() == Srp::Client::kRegistered); + VerifyOrQuit(service1.GetState() == Srp::Client::kRegistered); + break; + case 2: + VerifyOrQuit(srpClient->GetHostInfo().GetState() != Srp::Client::kRegistered); + VerifyOrQuit(service1.GetState() != Srp::Client::kRegistered); + break; + } + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // Remove service and close socket. + + srpClient->ClearHostAndServices(); + srpClient->Stop(); + + SuccessOrQuit(udpSocket.Close()); + } + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // Finalize OT instance + + Log("Finalizing OT instance"); + FinalizeTest(); + + Log("End of TestSrpClientDelayedResponse"); +} + #endif // OPENTHREAD_CONFIG_REFERENCE_DEVICE_ENABLE #endif // ENABLE_SRP_TEST @@ -1040,7 +1221,9 @@ int main(void) ot::TestSrpServerClientRemove(/* aShouldRemoveKeyLease */ false); #if OPENTHREAD_CONFIG_REFERENCE_DEVICE_ENABLE ot::TestUpdateLeaseShortVariant(); + ot::TestSrpClientDelayedResponse(); #endif + printf("All tests passed\n"); #else printf("SRP_SERVER or SRP_CLIENT feature is not enabled\n");