[Snort-devel] Memory allocation bug (patch included)

Erik de Castro Lopo erik.de.castro.lopo at ...2292...
Wed Aug 4 13:26:12 EDT 2004


Hi all,

I've found something in Snort which has the potential to lead
to some very obscure bugs. I have not seen any symptoms which
I even suspect are related to this issue, but I think this
is definitely worth fixing before any symtpoms are seen.

The tool used to find this was Valgrind:

     http://valgrind.kde.org/

which is an extremely useful tool for finding memory allocation 
errors and related problems. Unfortunately, I think it only runs 
on x86 Linux.

Anyway, the problem reported by valgrind is as follows (this
is one of about 20 instances):

   ==16146== Invalid read of size 1
   ==16146==    at 0x8094B5A: HASH16 (mwm.c:630)
   ==16146==    by 0x80957A3: mwmPrepPatterns (mwm.c:1279)
   ==16146==    by 0x805AE4B: BuildMultiPatternGroups (fpcreate.c:655)
   ==16146==    by 0x805AF45: fpCreateFastPacketDetection (fpcreate.c:1088)
   ==16146==  Address 0x3CEE3DD1 is 0 bytes after a block of size 1 alloc'd
   ==16146==    at 0x3C01F40D: malloc (vg_replace_malloc.c:105)
   ==16146==    by 0x8094961: mwmAddPatternEx (mwm.c:469)
   ==16146==    by 0x805AC75: BuildMultiPatGroup (fpcreate.c:577)
   ==16146==    by 0x805AE4B: BuildMultiPatternGroups (fpcreate.c:655)

The report shows that a 1 byte block of memory is allocated
at address 0x3C01F40D in function mwmAddPatternEx(), but
later, in the HASH16() function, the byte at  0x3C01F401
is read, having never been allocated and never initialised.

If a later call to malloc returned 0x3C01F401, would be
likely to cause the HASH16() function to give different
results before and after the second malloc.

In mwmAddPatternEx(), the malloc looks like:

    p->psPat =  (unsigned char*)malloc( m );

and I've added debugging printf statements to show that m is
sometimes 1. The fix for this is to replace the above (and a 
couple of similar calls) with:

    p->psPat =  (unsigned char*)calloc( 1, m == 1 ? 2 : m );

which allocates a minimum of two bytes and sets the memory
to zero.

Two line patch below.

Cheers,
Erik

-----------------------------------------------------------------
diff -u -r1.3 mwm.c
--- src/sfutil/mwm.c    17 Dec 2003 21:25:14 -0000      1.3
+++ src/sfutil/mwm.c    29 Jul 2004 01:20:14 -0000
@@ -466,7 +466,7 @@
     
 
     /* Allocate and store the Pattern  'P' with NO CASE info*/
-    p->psPat =  (unsigned char*)malloc( m );
+    p->psPat =  (unsigned char*)calloc( 1, m == 1 ? 2 : m );
     if( !p->psPat ) return -1;
 
     memcpy(p->psPat, P, m );
@@ -474,7 +474,7 @@
     ConvCaseToUpper( p->psPat, m );
 
     /* Allocate and store the Pattern  'P' with CASE info*/
-    p->psPatCase =  (unsigned char*)malloc( m );
+    p->psPatCase =  (unsigned char*)calloc( 1, m == 1 ? 2 : m );
     if( !p->psPatCase ) return -1;
 
     memcpy( p->psPatCase, P, m );
-----------------------------------------------------------------


-- 
------------------------------------------------------
[N] Erik de Castro Lopo, Senior Computer Engineer
[E] erik.de.castro.lopo at ...2292...
[W] http://www.sensorynetworks.com
[T] +61 2 83022726 
[F] +61 2 94750316 
[A] L4/140 William St, East Sydney NSW 2011, Australia
------------------------------------------------------
A good debugger is no substitute for a good test suite.





More information about the Snort-devel mailing list