[ovs-dev] [PATCH] datapath-windows: Fix potential memory leak while creating conntrack entry

[Sairam Venugopal]

I have addressed this in V2. The invalid label is not necessary.




On 6/20/17, 2:46 PM, "Shashank Ram" <rams at vmware.com> wrote:

>Thanks for the patch, please find comments inline.
>________________________________________
>From: ovs-dev-bounces at openvswitch.org <ovs-dev-bounces at openvswitch.org> on behalf of Sairam Venugopal <vsairam at vmware.com>
>Sent: Tuesday, June 20, 2017 1:36 PM
>To: dev at openvswitch.org
>Subject: [ovs-dev] [PATCH] datapath-windows: Fix potential memory leak while    creating conntrack entry
>
>OvsCtAddEntry returns TRUE or FALSE depending on whether
>OvsNatTranslateCtEntry was successful or not. In the case of an
>unsuccesful NAT translation, this will fail to insert the newly created
>entry to the Conntrack Table. This entry needs to be freed and the states
>should be accordingly in the flowKey instead of returning out.
>
>Consolidated the parentEntry lookup and assignment portion across
>different protocols and some minor refactoring to make the code more
>readable.
>
>Tests Done: Enabled driver verifier and tested the following:
>- TCP & ICMP traffic through Conntrack Module.
>- Flushed Conntrack Entries while traffic was flowing.
>- Uninstalled and re-installed the driver when traffic was in progress.
>
>Signed-off-by: Sairam Venugopal <vsairam at vmware.com>
>---
> datapath-windows/ovsext/Conntrack.c | 59 +++++++++++++++++--------------------
> 1 file changed, 27 insertions(+), 32 deletions(-)
>
>diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
>index 68ed395..bf9c4f4 100644
>--- a/datapath-windows/ovsext/Conntrack.c
>+++ b/datapath-windows/ovsext/Conntrack.c
>@@ -214,9 +214,18 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
>                  BOOLEAN *entryCreated)
> {
>     POVS_CT_ENTRY entry = NULL;
>-    *entryCreated = FALSE;
>     UINT32 state = 0;
>+    POVS_CT_ENTRY parentEntry;
>     PNET_BUFFER_LIST curNbl = fwdCtx->curNbl;
>+
>+    *entryCreated = FALSE;
>+    state |= OVS_CS_F_NEW;
>+
>+    parentEntry = OvsCtRelatedLookup(ctx->key, currentTime);
>+    if (parentEntry != NULL) {
>+        state |= OVS_CS_F_RELATED;
>+    }
>+
>     switch (ipProto)
>     {
>         case IPPROTO_TCP:
>@@ -228,23 +237,8 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
>                 goto invalid;
>             }
>
>-            state |= OVS_CS_F_NEW;
>-            POVS_CT_ENTRY parentEntry;
>-            parentEntry = OvsCtRelatedLookup(ctx->key, currentTime);
>-            if (parentEntry != NULL) {
>-                state |= OVS_CS_F_RELATED;
>-            }
>-
>             if (commit) {
>                 entry = OvsConntrackCreateTcpEntry(tcp, curNbl, currentTime);
>-                if (!entry) {
>-                    return NULL;
>-                }
>-
>-                /* Set parent entry for related FTP connections */
>-                entry->parent = parentEntry;
>-
>-                *entryCreated = TRUE;
>             }
>             break;
>         }
>@@ -257,14 +251,8 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
>                 goto invalid;
>             }
>
>-            state |= OVS_CS_F_NEW;
>             if (commit) {
>                 entry = OvsConntrackCreateIcmpEntry(currentTime);
>-                if (entry) {
>-                    /* XXX Add support for ICMP-Related */
>-                    entry->parent = NULL;
>-                }
>-                *entryCreated = TRUE;
>             }
>             break;
>         }
>@@ -273,11 +261,6 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
>             state |= OVS_CS_F_NEW;
>>>> This is not needed since its initialized at the start?
>
>
>             if (commit) {
>                 entry = OvsConntrackCreateOtherEntry(currentTime);
>-                if (entry) {
>-                    /* Default UDP related to NULL until TFTP is supported */
>-                    entry->parent = NULL;
>-                }
>-                *entryCreated = TRUE;
>             }
>             break;
>         }
>@@ -285,12 +268,24 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
>             goto invalid;
>     }
>
>-    if (commit && !entry) {
>-        return NULL;
>-    }
>-    if (entry && !OvsCtAddEntry(entry, ctx, natInfo, currentTime)) {
>-        return NULL;
>+    if (commit) {
>+        if (entry) {
>+            entry->parent = parentEntry;
>+            if (OvsCtAddEntry(entry, ctx, natInfo, currentTime)) {
>+                *entryCreated = TRUE;
>+            } else {
>+                /* Unable to add entry to the list */
>+                OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG);
>+                state = OVS_CS_F_INVALID;
>+                entry = NULL;
>>>> jump to "invalid" instead?
>
>
>+            }
>+        } else {
>+            /* OvsAllocateMemoryWithTag returned NULL; treat as invalid */
>+            state = OVS_CS_F_INVALID;
>>>> jump to "invalid" instead?
>
>
>+        }
>     }
>+
>+
>     OvsCtUpdateFlowKey(key, state, ctx->key.zone, 0, NULL);
>     return entry;
> invalid:
>--