[Snort-devel] Defrag preprocessor crashing (was RE: [Snort-users] Stream4 and other stuff)

Eugene Tsyrklevich eugene at ...223...
Mon Jul 2 14:12:20 EDT 2001


Hi,

One of the things I've noticed is that variables such as fragmemuse are
sometimes incorrectly incremented before additional error checking is performed.
The patch also contains some comments (XXX) that should probably be removed
before the commit.

eugene


On Mon, Jul 02, 2001 at 08:31:20AM -0700, Dragos Ruiu wrote:
> The other option is to define DEBUG... but if you've got tham much
> fragmemuse...hmmm... that mugh debug output could be painful.
> Though if it hits that fast with the debug on (i.e. not much output before
> it get to that high number...  that tells one that it's an algorithm silly on
> my part....
> 
> I'm looking, likely culprit, I prolly did something silly....
> 
> cheers,
> --dr
> 
> 
> On Mon, 02 Jul 2001, Martin Roesch wrote: > Philip Mayers
> wrote: > > 
> > > Wow - what service! Has anyone ever told you you're a very helpful guy?
> > 
> > Well, I do what I can. :)
> > 
> > > (gdb) p fragmemuse
> > > $2 = 42387068
> > 
> > Hmm, that number doesn't look insane, it's just kind of suprising that
> > you'd be using that much RAM to store the packets that quickly.   Is
> > there a lot of fragmented traffic on your network for some reason?
> > 
> >      -Marty
> > 
> > > 
> > > Regards,
> > > Phil
> > > 
> > > +----------------------------------+
> > > | Phil Mayers, Network Support     |
> > > | Centre for Computing Services    |
> > > | Imperial College                 |
> > > +----------------------------------+
> > > 
> > > ----- Original Message -----
> > > From: "Martin Roesch" <roesch at ...402...>
> > > To: "Mayers, Philip J" <p.mayers at ...489...>
> > > Cc: "snort-users" <snort-users at lists.sourceforge.net>; "snort-dev"
> > > <snort-devel at lists.sourceforge.net>
> > > Sent: Monday, July 02, 2001 3:06 PM
> > > Subject: Re: [Snort-devel] Defrag preprocessor crashing (was RE:
> > > [Snort-users] Stream4 and other stuff)
> > > 
> > > >
> > > > Could you print out fragmemuse ('p fragmemuse' in gdb) and let me know
> > > > what it looks like?
> > > >
> > 
> > --
> > Martin Roesch
> > roesch at ...402...
> > http://www.sourcefire.com - http://www.snort.org
> > 
> > _______________________________________________
> > Snort-devel mailing list
> > Snort-devel at lists.sourceforge.net
> > http://lists.sourceforge.net/lists/listinfo/snort-devel
> -- 
> Dragos Ruiu <dr at ...9...>   dursec.com ltd. / kyx.net - we're from the future 
> gpg/pgp key on file at wwwkeys.pgp.net or at http://dursec.com/drkey.asc
> 
> _______________________________________________
> Snort-devel mailing list
> Snort-devel at lists.sourceforge.net
> http://lists.sourceforge.net/lists/listinfo/snort-devel
> 
-------------- next part --------------
Index: spp_defrag.c
===================================================================
RCS file: /cvsroot/snort/snort/spp_defrag.c,v
retrieving revision 1.18
diff -u -r1.18 spp_defrag.c
--- spp_defrag.c	2001/07/02 14:25:25	1.18
+++ spp_defrag.c	2001/07/02 18:16:12
@@ -99,7 +99,7 @@
 #define DADDR(x)    ( x->iph->ip_dst.s_addr )
 #define DATA(x)     ((u_int8_t *)x->iph+20)
 
-/* Uh-oh hope this wierdness is right :-) */
+/* Uh-oh hope this weirdness is right :-) */
 #define FOFF(x)     (u_int32_t)((x->frag_offset)<<3)
 #define DF(x)       (x->df)
 #define MF(x)       (x->mf)
@@ -131,7 +131,7 @@
 };
 
 List *garbagelist;
-char trashejectspinlock;
+int trashejectspinlock;
 	
 
 /*  These next declarations are for the fragment timeout and 
@@ -139,8 +139,10 @@
     an obscure piece of old code for a defunct video camera product
 */
 
+/* 10 seconds is too low, BSD uses ~30, we need to go even higher than that */
 #define FRAGTIMEOUTSEC      10      /* 10 seconds let's play safe for now */
 #define FRAGTIMEOUTUSEC      0      /* 0 micro seconds                  */
+/* XXX should be user definable */
 #define MEMHARDLIM     	  32000000  /* memory hardlimit threshold */
 
 int fragmemuse;
@@ -200,6 +202,8 @@
 		/* toss that puppy later by adding it to garbagelist */
 		List *new;
 		new = malloc(sizeof(List));
+		if (new == NULL)
+			return(-1);	/* XXX */
 		new->next = garbagelist;
 		new->key = j;
 		garbagelist = new;
@@ -392,6 +396,16 @@
     }
 
     new_tree_node = (Tree *) malloc (sizeof (Tree));
+
+    if(!new_tree_node || fragmemuse > MEMHARDLIM)
+    {
+        ErrorMessage("Ran out of space\n");
+	if (new_tree_node)
+		free(new_tree_node);
+        spacealert++;
+        return(t);
+    }
+
     fragmemuse += sizeof(Tree);
 #ifdef DEBUG
     fprintf(stderr, "+++++++++++++++++++++++++\n");
@@ -401,18 +415,12 @@
     mem_locked += sizeof(Tree);
 #endif
 
-    if(!new_tree_node || fragmemuse > MEMHARDLIM)
-    {
-        ErrorMessage("Ran out of space\n");
-        spacealert++;
-        return(t);
-    }
-
     /* alert hysterisis(?) */
     if (spacealert)
         spacealert -= ((unsigned int)spacealert) >> 1;
     if (spacealert < 0)
         spacealert = 0;
+
     if(!t)
     {
         new_tree_node->left = new_tree_node->right = NULL;
@@ -622,7 +630,7 @@
  
  
 /* shift based bit revrser --dr */
- 
+/* XXX not used? 
 unsigned int bitrev(x)
 unsigned int x;
 {
@@ -632,7 +640,7 @@
                         r |= (unsigned int)1<<((sizeof(unsigned int)*8)-i-1);
         return r;
 }
- 
+*/
  
 /* Function: fragbalance(i, t)                              */
 /* Splay using the key i (which may or may not be in the tree.) */
@@ -733,9 +741,9 @@
     printf("   =>  proto1 = %d, proto2 = %d\n", PROTO(i), PROTO(j));
 #endif    
 
-    if(( SADDR(i) == SADDR(j) )
+    if(( ID(i) == ID(j) ) 	/* XXX id is more likely to be different */
+       && ( SADDR(i) == SADDR(j) )
        && ( DADDR(i) == DADDR(j) )
-       && ( ID(i) == ID(j) )
        && ( PROTO(i) == PROTO(j) ))
     {
         return(1);
@@ -802,6 +810,14 @@
 #endif        
 
     p = (Packet *)malloc(sizeof(Packet));
+
+    if(!p)
+    {
+        ErrorMessage("[!] ERROR: Unable to allocate memory for "
+                     "fragment rebuild!\n");
+        return NULL;
+    }
+
     fragmemuse += sizeof(Packet);
 
 #ifdef DEBUG
@@ -812,13 +828,6 @@
     mem_locked += sizeof(Packet);
 #endif
 
-    if(!p)
-    {
-        ErrorMessage("[!] ERROR: Unable to allocate memory for "
-                     "fragment rebuild!\n");
-        return NULL;
-    }
-
     /* MFR: Why are we copying this header if we're in the reassembly phase?
      *  Why not use the original and modify its data in place?
      */
@@ -833,16 +842,7 @@
      */
     p->pkth = (struct pcap_pkthdr *)
                 malloc(psize + overhead + moreoverhead + sizeof(IPHdr) + 32);
-    fragmemuse += (psize + overhead + moreoverhead + sizeof(IPHdr) + 32);
 
-#ifdef DEBUG
-    fprintf(stderr, "+++++++++++++++++++++++++\n");
-    fprintf(stderr, "%p + frankenpacket Alloc\n", p->pkth);
-    fprintf(stderr, "+++++++++++++++++++++++++\n");
-    fflush(stderr);
-    mem_locked += sizeof(psize+overhead+moreoverhead+sizeof(IPHdr)+32);
-#endif
-
     if(!p->pkth)
     {
         ErrorMessage("[!] ERROR: Unable to allocate memory for fragment "
@@ -859,6 +859,16 @@
         return NULL;
     }
 
+    fragmemuse += (psize + overhead + moreoverhead + sizeof(IPHdr) + 32);
+
+#ifdef DEBUG
+    fprintf(stderr, "+++++++++++++++++++++++++\n");
+    fprintf(stderr, "%p + frankenpacket Alloc\n", p->pkth);
+    fprintf(stderr, "+++++++++++++++++++++++++\n");
+    fflush(stderr);
+    mem_locked += sizeof(psize+overhead+moreoverhead+sizeof(IPHdr)+32);
+#endif
+
     p->iph = (IPHdr *)((u_char*)p->pkth + overhead + moreoverhead);
     p->pkt = (u_char*)p->iph - moreoverhead;
 
@@ -1044,7 +1054,6 @@
     struct pcap_pkthdr *freetemp;
     int overhead;
     int cap;
-    u_char *tmp;
     Tree *new_froot;
 
 #ifdef DEBUG
@@ -1080,6 +1089,14 @@
             overhead = sizeof(struct pcap_pkthdr) + 2;
 
         packet_copy = malloc(sizeof(Packet));
+
+        if(!packet_copy)
+        {
+            ErrorMessage("[!] ERROR: Cannot allocate fragment "
+                         "buffer(usage 0x%X)\n",fragmemuse);
+            return;
+        }
+
         fragmemuse += sizeof(Packet);
 
 #ifdef DEBUG
@@ -1091,19 +1108,20 @@
         mem_locked += sizeof(Packet);
 #endif
 
-        if(!packet_copy)
+        memcpy(packet_copy, p, sizeof(Packet));
+        cap = p->pkth->caplen + overhead;
+        packet_copy->pkth = (struct pcap_pkthdr *) malloc(cap + 20);
+
+        if(!packet_copy->pkth)
         {
+            free(packet_copy);
+            fragmemuse -= sizeof(Packet);
             ErrorMessage("[!] ERROR: Cannot allocate fragment "
-                         "buffer(usage 0x%X)\n",fragmemuse);
+                         "buffer(usage %X)\n",fragmemuse);
             return;
         }
 
-
-        memcpy(packet_copy, p, sizeof(Packet));
-        cap = p->pkth->caplen + overhead;
-        tmp = calloc(cap + 20, sizeof(char));
         fragmemuse += (cap + 20);
-        packet_copy->pkth = (struct pcap_pkthdr *) tmp;
 
 #ifdef DEBUG
         fprintf(stderr, " && %p + packet Alloc\n", packet_copy->pkth);
@@ -1111,14 +1129,6 @@
         fflush(stderr);
         mem_locked += cap+20;
 #endif
-        if(!packet_copy->pkth)
-        {
-            free(packet_copy);
-            fragmemuse -= sizeof(Packet);
-            ErrorMessage("[!] ERROR: Cannot allocate fragment "
-                         "buffer(usage %X)\n",fragmemuse);
-            return;
-        }
 
         packet_copy->pkt = (u_char*)packet_copy->pkth + overhead;
         packet_copy->iph = (IPHdr*)((u_char*)packet_copy->pkt + 
@@ -1134,6 +1144,7 @@
 
         if(fragmemuse < 0)
             fragmemuse = 0;
+	/* XXX the timeouts should not depend on the amount of avail memory */
         fragtimeout.tv_sec = 
             (int)((float)FRAGTIMEOUTSEC*((MEMHARDLIM-fragmemuse)/
                                          (float)MEMHARDLIM));
@@ -1169,15 +1180,15 @@
         /* OK now eject the trash (fragment timeout :-) */
         if(froot)
         {
+            trashejectspinlock = TRUE;
+
             while(garbagelist)
             {
                 List *trash;
-                trashejectspinlock = TRUE;
-#ifdef DEBUG                    
-                fprintf(stderr, "Timeout cleanup in progress...\n");
-#endif
+
                 freetemp = garbagelist->key->pkth;
 #ifdef DEBUG    
+                fprintf(stderr, "Timeout cleanup in progress...\n");
                 fprintf(stderr, "------------------------\n");
                 fprintf(stderr, "%p - packet Free\n", freetemp);
                 fprintf(stderr, "------------------------\n");
@@ -1196,13 +1207,10 @@
                 free(trash);
                 fragmemuse -= sizeof(List);
             }
-            trashejectspinlock = FALSE;
 
+            trashejectspinlock = FALSE;
         }
     }
 
     return;
 }
-
-
-


More information about the Snort-devel mailing list