From 25c6e0ec89055147580ef4ebe855ea34f7c2e2f6 Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@google.com>
Date: Thu, 29 Dec 2022 17:16:33 -0800
Subject: [PATCH] [nat64] fix message leak when NAT64 translator is enabled
 (#8592)

This commit fixes a potential message leak when NAT64 translator
feature is enabled in `Ip6::ProcessReceiveCallback()` where a cloned
copy of message is not freed if `TranslateFromIp6()` returns an
error or if `mReceiveIp4DatagramCallback` is not set.

This commit updates how we track and free the message in this method
(to help simplify the code and avoid similar situations). Variable
`message` points to the `Message` instance we own in this method. If
we can take ownership of `aMessage`, we use it as `message`.
Otherwise, we may create a clone of it. `message` will be set to
`nullptr` if the message ownership is transferred to an invoked
callback. At the end of this method we free `message` if it is not
`nullptr` (indicating it was not passed to a callback.
---
 src/core/net/ip6.cpp | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/src/core/net/ip6.cpp b/src/core/net/ip6.cpp
index 65b2bd340..64360f46f 100644
--- a/src/core/net/ip6.cpp
+++ b/src/core/net/ip6.cpp
@@ -998,13 +998,26 @@ Error Ip6::ProcessReceiveCallback(Message           &aMessage,
                                   Message::Ownership aMessageOwnership)
 {
     Error    error   = kErrorNone;
-    Message *message = &aMessage;
+    Message *message = nullptr;
 #if OPENTHREAD_CONFIG_IP6_BR_COUNTERS_ENABLE
     Header header;
 
     IgnoreError(header.ParseFrom(aMessage));
 #endif
 
+    // `message` points to the `Message` instance we own in this
+    // method. If we can take ownership of `aMessage`, we use it as
+    // `message`. Otherwise, we may create a clone of it and use as
+    // `message`. `message` variable will be set to `nullptr` if the
+    // message ownership is transferred to an invoked callback. At
+    // the end of this method we free `message` if it is not `nullptr`
+    // indicating it was not passed to a callback.
+
+    if (aMessageOwnership == Message::kTakeCustody)
+    {
+        message = &aMessage;
+    }
+
     VerifyOrExit(aOrigin != kFromHostDisallowLoopBack, error = kErrorNoRoute);
 
     VerifyOrExit(mReceiveIp6DatagramCallback.IsSet(), error = kErrorNoRoute);
@@ -1029,6 +1042,7 @@ Error Ip6::ProcessReceiveCallback(Message           &aMessage,
             if (mIcmp.ShouldHandleEchoRequest(aMessageInfo))
             {
                 Icmp::Header icmp;
+
                 IgnoreError(aMessage.Read(aMessage.GetOffset(), icmp));
 
                 // do not pass ICMP Echo Request messages
@@ -1081,24 +1095,23 @@ Error Ip6::ProcessReceiveCallback(Message           &aMessage,
         ExitNow(error = kErrorDrop);
     case Nat64::Translator::kForward:
         VerifyOrExit(mReceiveIp4DatagramCallback.IsSet(), error = kErrorNoRoute);
+        // Pass message to callback transferring its ownership.
         mReceiveIp4DatagramCallback.Invoke(message);
+        message = nullptr;
         ExitNow();
     }
 #endif
 
+    // Pass message to callback transferring its ownership.
     mReceiveIp6DatagramCallback.Invoke(message);
+    message = nullptr;
 
 #if OPENTHREAD_CONFIG_IP6_BR_COUNTERS_ENABLE
     UpdateBorderRoutingCounters(header, aMessage.GetLength(), /* aIsInbound */ false);
 #endif
 
 exit:
-
-    if ((error != kErrorNone) && (aMessageOwnership == Message::kTakeCustody))
-    {
-        aMessage.Free();
-    }
-
+    FreeMessage(message);
     return error;
 }