From a662c32eb074cc624bf344f810f65f8637a89552 Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@google.com>
Date: Mon, 21 Jun 2021 18:08:54 -0700
Subject: [PATCH] [srp-server] free `aHost` in `CommitSrpUpdate()` in case of
 error (#6754)

This commit ensures that in `Srp::Server::CommitSrpUpdate()` the
`aHost` parameter is correctly freed in case of an error and an early
jump to exit (e.g., from `SuccessOrExit(aError)` at the start of this
method). The `aHost` is now freed at the `exit` label and a boolean
`shouldFreeHost` tracks whether or not it needs to be freed. The
boolean variable is set to `false` when  the ownership of `aHost` is
taken over (e.g., from the call to `AddHost(aHost)`).
---
 src/core/net/srp_server.cpp | 19 +++++++++++--------
 src/core/net/srp_server.hpp |  2 +-
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/src/core/net/srp_server.cpp b/src/core/net/srp_server.cpp
index 363accc0b..bc9e88c95 100644
--- a/src/core/net/srp_server.cpp
+++ b/src/core/net/srp_server.cpp
@@ -205,10 +205,10 @@ const Server::Host *Server::GetNextHost(const Server::Host *aHost)
 
 // This method adds a SRP service host and takes ownership of it.
 // The caller MUST make sure that there is no existing host with the same hostname.
-void Server::AddHost(Host *aHost)
+void Server::AddHost(Host &aHost)
 {
-    OT_ASSERT(mHosts.FindMatching(aHost->GetFullName()) == nullptr);
-    IgnoreError(mHosts.Add(*aHost));
+    OT_ASSERT(mHosts.FindMatching(aHost.GetFullName()) == nullptr);
+    IgnoreError(mHosts.Add(aHost));
 }
 
 void Server::RemoveHost(Host *aHost, bool aRetainName, bool aNotifyServiceHandler)
@@ -330,6 +330,7 @@ void Server::CommitSrpUpdate(Error                    aError,
     uint32_t hostKeyLease;
     uint32_t grantedLease;
     uint32_t grantedKeyLease;
+    bool     shouldFreeHost = true;
 
     SuccessOrExit(aError);
 
@@ -361,8 +362,6 @@ void Server::CommitSrpUpdate(Error                    aError,
                 existingHost->RemoveService(service, /* aRetainName */ true, /* aNotifyServiceHandler */ false);
             }
         }
-
-        aHost.Free();
     }
     else if (existingHost != nullptr)
     {
@@ -391,13 +390,12 @@ void Server::CommitSrpUpdate(Error                    aError,
                              newService->GetFullName());
             }
         }
-
-        aHost.Free();
     }
     else
     {
         otLogInfoSrp("[server] add new host %s", aHost.GetFullName());
-        AddHost(&aHost);
+        AddHost(aHost);
+        shouldFreeHost = false;
 #if OPENTHREAD_CONFIG_SRP_SERVER_PORT_SWITCH_ENABLE
         if (!mHasRegisteredAnyService)
         {
@@ -422,6 +420,11 @@ exit:
     {
         SendResponse(aDnsHeader, ErrorToDnsResponseCode(aError), aMessageInfo);
     }
+
+    if (shouldFreeHost)
+    {
+        aHost.Free();
+    }
 }
 
 void Server::Start(void)
diff --git a/src/core/net/srp_server.hpp b/src/core/net/srp_server.hpp
index 3542275f8..d6aec724b 100644
--- a/src/core/net/srp_server.hpp
+++ b/src/core/net/srp_server.hpp
@@ -637,7 +637,7 @@ private:
     const Service *FindService(const char *aFullName) const;
 
     void        HandleUpdate(const Dns::UpdateHeader &aDnsHeader, Host *aHost, const Ip6::MessageInfo &aMessageInfo);
-    void        AddHost(Host *aHost);
+    void        AddHost(Host &aHost);
     void        RemoveHost(Host *aHost, bool aRetainName, bool aNotifyServiceHandler);
     bool        HasNameConflictsWith(Host &aHost) const;
     void        SendResponse(const Dns::UpdateHeader &   aHeader,