Wireshark · Ethereal-dev: Re: dcerpc patch (was Re: [Ethereal-dev] Is someone working on a DCOM/ORPC dissector?)

[Todd Sabin <tas@xxxxxxxxxxx>]

Guy Harris <gharris@xxxxxxxxx> writes:

> On Mon, Jul 09, 2001 at 08:03:02AM -0400, Todd Sabin wrote:
> > Could someone double-check the conversation/hash stuff for leaks?  I
> > used pretty much the same approach used by packet-rpc.c,
> 
> But not exactly the same approach.
> [...]
> "rpc_init_protocol()" not only destroys and reallocates the hash tables,
> it also destroys and reallocates the memory chunks, so that the hash
> tables *and* the keys and values are freed.
> 
> The patched DCE RPC dissector didn't appear to be freeing any of the
> keys - destroying a hash table *doesn't* free up the keys (and *can't*,
> as they could be statically-allocated data structures, for example).
> 
> I'd suggest allocating the call keys and values, UUID keys and values,
> and conversation keys and values from memory chunks, and destroying and
> recreating the chunks in "dcerpc_init_protocol()".
> 
> ("dcerpc_init_protocol()" doesn't destroy and recreate the UUID hash
> table; is the theory here that the first "U" stands for "unique" so that
> there's no reason to destroy the table just because you're processing a
> different capture?)
> 

Thanks,

I wasn't sure about that, because packet-rpc.c uses both g_malloc and
g_mem_chunk_alloc.  I now see why.  It uses g_malloc for rpc_procs and
rpc_progs, which are where it keeps track of its subdissectors, which
don't change for different captures.  It also doesn't recreate those
per capture.  dcerpc_uuids is analogous to those, which is why I'm not
destroying/recreating it.  For the other hashes, packet-rpc.c is using
g_mem_chunk_alloc, as you say.  Here's a patch to the previous patch
to bring packet-dcerpc.c in line with that.


Todd


--- packet-dcerpc.c~	Mon Jul  9 06:32:03 2001
+++ packet-dcerpc.c	Tue Jul 10 06:57:21 2001
@@ -207,11 +207,15 @@
     guint16 ctx_id;
 } dcerpc_conv_key;
 
+static GMemChunk *dcerpc_conv_key_chunk;
+
 typedef struct _dcerpc_conv_value {
     e_uuid_t uuid;
     guint16 ver;
 } dcerpc_conv_value;
 
+static GMemChunk *dcerpc_conv_value_chunk;
+
 static gint
 dcerpc_conv_equal (gconstpointer k1, gconstpointer k2)
 {
@@ -241,12 +245,16 @@
     guint32 call_id;
 } dcerpc_call_key;
 
+static GMemChunk *dcerpc_call_key_chunk;
+
 typedef struct _dcerpc_call_value {
     e_uuid_t uuid;
     guint16 ver;
     guint16 opnum;
 } dcerpc_call_value;
 
+static GMemChunk *dcerpc_call_value_chunk;
+
 static gint
 dcerpc_call_equal (gconstpointer k1, gconstpointer k2)
 {
@@ -267,8 +275,8 @@
 dcerpc_call_add_map (guint32 call_id, conversation_t *conv,
                      guint16 opnum, guint16 ver, e_uuid_t *uuid)
 {
-    dcerpc_call_key *key = g_malloc (sizeof (*key));
-    dcerpc_call_value *value = g_malloc (sizeof (*value));
+    dcerpc_call_key *key = g_mem_chunk_alloc (dcerpc_call_key_chunk);
+    dcerpc_call_value *value = g_mem_chunk_alloc (dcerpc_call_value_chunk);
 
     key->call_id = call_id;
     key->conv = conv;
@@ -534,11 +542,11 @@
                                  pinfo->srcport, pinfo->destport, NULL, 0);
     }
 
-    key = g_malloc (sizeof (*key));
+    key = g_mem_chunk_alloc (dcerpc_conv_key_chunk);
     key->conv = conv;
     key->ctx_id = ctx_id;
 
-    value = g_malloc (sizeof (*value));
+    value = g_mem_chunk_alloc (dcerpc_conv_value_chunk);
     value->uuid = if_id;
     value->ver = if_ver;
 
@@ -1049,13 +1057,37 @@
 static void
 dcerpc_init_protocol (void)
 {
-    if (dcerpc_convs != NULL)
+    if (dcerpc_convs)
         g_hash_table_destroy (dcerpc_convs);
-    if (dcerpc_calls != NULL)
+    if (dcerpc_calls)
         g_hash_table_destroy (dcerpc_calls);
+    if (dcerpc_conv_key_chunk)
+        g_mem_chunk_destroy (dcerpc_conv_key_chunk);
+    if (dcerpc_conv_value_chunk)
+        g_mem_chunk_destroy (dcerpc_conv_value_chunk);
+    if (dcerpc_call_key_chunk)
+        g_mem_chunk_destroy (dcerpc_call_key_chunk);
+    if (dcerpc_call_value_chunk)
+        g_mem_chunk_destroy (dcerpc_call_value_chunk);
 
     dcerpc_convs = g_hash_table_new (dcerpc_conv_hash, dcerpc_conv_equal);
     dcerpc_calls = g_hash_table_new (dcerpc_call_hash, dcerpc_call_equal);
+    dcerpc_conv_key_chunk = g_mem_chunk_new ("dcerpc_conv_key_chunk",
+                                             sizeof (dcerpc_conv_key),
+                                             200 * sizeof (dcerpc_conv_key),
+                                             G_ALLOC_ONLY);
+    dcerpc_conv_value_chunk = g_mem_chunk_new ("dcerpc_conv_value_chunk",
+                                             sizeof (dcerpc_conv_value),
+                                             200 * sizeof (dcerpc_conv_value),
+                                             G_ALLOC_ONLY);
+    dcerpc_call_key_chunk = g_mem_chunk_new ("dcerpc_call_key_chunk",
+                                             sizeof (dcerpc_call_key),
+                                             200 * sizeof (dcerpc_call_key),
+                                             G_ALLOC_ONLY);
+    dcerpc_call_value_chunk = g_mem_chunk_new ("dcerpc_call_value_chunk",
+                                             sizeof (dcerpc_call_value),
+                                             200 * sizeof (dcerpc_call_value),
+                                             G_ALLOC_ONLY);
 }
 
 void
@@ -1206,8 +1238,6 @@
     register_init_routine (dcerpc_init_protocol);
 
     dcerpc_uuids = g_hash_table_new (dcerpc_uuid_hash, dcerpc_uuid_equal);
-    dcerpc_convs = g_hash_table_new (dcerpc_conv_hash, dcerpc_conv_equal);
-    dcerpc_calls = g_hash_table_new (dcerpc_call_hash, dcerpc_call_equal);
 }
 
 void