[Bro-Dev] newly found segfaults from metrics framework

[Jonathan Siwek]

> Have you guys tried valgrind? That can often pinpoint memory
> corruoption right at the source. It can be slow, but prefiltering the
> trace for SSL might help (if that's what's triggering it).

Yeah, valgrind is pointing out invalid accesses in CompHash.cc, reproducible for me by running the metrics/ssl-example.bro on the 2009-M57-day11-21.trace from bro-testing filtered for "tcp port 443".

Looked like it tried to stuff a string value someplace inside the CompositeHash::key buffer when it hasn't been sized to hold that much:

==27445== Invalid write of size 4
==27445==    at 0x4027C3D: memcpy (mc_replace_strmem.c:635)
==27445==    by 0x82229C5: CompositeHash::SingleValHash(int, char*, BroType*, Val*, bool) const (CompHash.cc:211)
...

I didn't think it could be sized right in the CompositeHash ctor since it's only got Types and not Vals to work with at that point, so I changed the computation to allocate and use a buffer on the fly if the pre-allocated one from the ctor isn't large enough to hold the key computed from the index vals.

$ git diff
diff --git a/src/CompHash.cc b/src/CompHash.cc
index 605949b..c3c65e3 100644
--- a/src/CompHash.cc
+++ b/src/CompHash.cc
@@ -241,16 +241,13 @@ HashKey* CompositeHash::ComputeHash(const Val* v, int type
                }
 
        char* k = key;
+       int sz = ComputeKeySize(v, type_check);
+       if ( sz == 0 )
+               return 0;
+       type_check = 0; // no need to type-check again.
 
-       if ( ! k )
-               {
-               int sz = ComputeKeySize(v, type_check);
-               if ( sz == 0 )
-                       return 0;
-
+       if ( ! k || sz != size )
                k = reinterpret_cast<char*>(new double[sz/sizeof(double) + 1]);
-               type_check = 0; // no need to type-check again.
-               }
 
        const type_list* tl = type->Types();
 
That made the metrics framework happy for me, but does that seem like a reasonable approach?

- Jon