Project

General

Profile

Actions

Bug #5748

closed

iprep/ipv6: memory leak on same input in different forms

Added by Victor Julien over 1 year ago. Updated 10 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

2001:0000:0000:0000:0000:0000:0000:0000/16,2,100
2001:0470:73f7:0000:0000:0000:0000:0007/16,2,100

Leads to:
=================================================================
==601433==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 64 byte(s) in 1 object(s) allocated from:
    #0 0x564d1381fc9e in malloc (/home/victor/sync/devel/suricata-6.0.x/src/suricata+0x7fac9e) (BuildId: 7a62c6750ba2caf5a87fe129f2c27374db7f1145)
    #1 0x564d1498fe01 in SCMallocFunc /home/victor/devel/suricata-6.0.x/src/util-mem.c:30:20
    #2 0x564d14667671 in SRepCIDRAddNetblock /home/victor/devel/suricata-6.0.x/src/reputation.c:84:22
    #3 0x564d14663041 in SRepSplitLine /home/victor/devel/suricata-6.0.x/src/reputation.c:322:9
    #4 0x564d14661e4a in SRepLoadFileFromFD /home/victor/devel/suricata-6.0.x/src/reputation.c:467:17
    #5 0x564d146642ec in SRepLoadFile /home/victor/devel/suricata-6.0.x/src/reputation.c:430:9
    #6 0x564d14663aaf in SRepInit /home/victor/devel/suricata-6.0.x/src/reputation.c:645:25
    #7 0x564d13dd2f96 in DetectEngineCtxInitReal /home/victor/devel/suricata-6.0.x/src/detect-engine.c:2057:11
    #8 0x564d13dd320d in DetectEngineCtxInit /home/victor/devel/suricata-6.0.x/src/detect-engine.c:2093:12
    #9 0x564d14880a4d in PostConfLoadedDetectSetup /home/victor/devel/suricata-6.0.x/src/suricata.c:2441:22
    #10 0x564d14884560 in SuricataMain /home/victor/devel/suricata-6.0.x/src/suricata.c:2900:5
    #11 0x564d1385aa1e in main /home/victor/devel/suricata-6.0.x/src/main.c:22:12
    #12 0x7faf8e382d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

SUMMARY: AddressSanitizer: 64 byte(s) leaked in 1 allocation(s).
Actions #1

Updated by Giuseppe Longo about 1 year ago

Memory leak happens also with IPV4 addresses:

192.168.1.0/16,2,100
192.168.2.0/16,2,100

I'm not sure if an IP address should be added again if it is already present.
In that case, checking if the ip it's already in the tree before adding it prevent the memory to be leaked.
I've quickly made these changes below, and the leaks are fixed.

diff --git a/src/reputation.c b/src/reputation.c
index f22f021c6..3a4d6a1cb 100644
--- a/src/reputation.c
+++ b/src/reputation.c
@@ -101,6 +101,7 @@ static void SRepCIDRAddNetblock(SRepCIDRTree *cidr_ctx, char *ip, int cat, uint8
         SCLogDebug("adding ipv6 host %s", ip);
         if (SCRadixAddKeyIPV6String(ip, cidr_ctx->srepIPV6_tree[cat], (void *)user_data) == NULL) {
             SCLogWarning("failed to add ipv6 host %s", ip);
+            SCFree(user_data);
         }

     } else {
@@ -116,6 +117,7 @@ static void SRepCIDRAddNetblock(SRepCIDRTree *cidr_ctx, char *ip, int cat, uint8
         SCLogDebug("adding ipv4 host %s", ip);
         if (SCRadixAddKeyIPV4String(ip, cidr_ctx->srepIPV4_tree[cat], (void *)user_data) == NULL) {
             SCLogWarning("failed to add ipv4 host %s", ip);
+            SCFree(user_data);
         }
     }
 }
diff --git a/src/util-radix-tree.c b/src/util-radix-tree.c
index 6192bd480..be7869596 100644
--- a/src/util-radix-tree.c
+++ b/src/util-radix-tree.c
@@ -1007,6 +1007,11 @@ SCRadixNode *SCRadixAddKeyIPV4String(const char *str, SCRadixTree *tree, void *u
         SCRadixValidateIPv4Key((uint8_t *)&ip, netmask);
 #endif
     }
+    if (SCRadixFindKeyIPV4Netblock((uint8_t *)&ip, tree, netmask, user) != NULL) {
+        SCLogWarning("IP already added, skipping it.");
+        return NULL;
+    }
+
     return SCRadixAddKey((uint8_t *)&ip, 32, tree, user, netmask);
 }

@@ -1072,6 +1077,10 @@ SCRadixNode *SCRadixAddKeyIPV6String(const char *str, SCRadixTree *tree, void *u
         SCRadixValidateIPv6Key((uint8_t *)&addr.s6_addr, netmask);
 #endif
     }
+    if (SCRadixFindKeyIPV6Netblock(addr.s6_addr, tree, netmask, user) != NULL) {
+        SCLogWarning("IP already added, skipping it.");
+        return NULL;
+    }

     return SCRadixAddKey(addr.s6_addr, 128, tree, user, netmask);
 }
Actions #2

Updated by Jeff Lucovsky about 1 year ago

  • Status changed from New to In Review
  • Assignee changed from OISF Dev to Jeff Lucovsky
Actions #3

Updated by Jeff Lucovsky 10 months ago

  • Status changed from In Review to Closed
Actions

Also available in: Atom PDF