On Fri, Sep 7, 2012 at 6:09 PM, Evan Huus <eapache@xxxxxxxxx> wrote:
> On Fri, Sep 7, 2012 at 5:47 PM, Maynard, Chris
> <Christopher.Maynard@xxxxxxxxx> wrote:
>> Recently another old proprietary protocol (I’ll call it FOO) was brought to
>> my attention, and I was asked to write a dissector for it. In doing so, I
>> discovered a conflict with another dissector, namely SNA. Initially I
>> thought that simply disabling SNA when analyzing FOO would be good enough
>> for our purposes; however, even after disabling SNA, the FOO dissector was
>> never called.
>>
>>
>>
>> The conflict arises because both dissectors, SNA and FOO, register as
>> follows:
>>
>>
>>
>> dissector_add_uint("llc.dsap", SAP_SNA2, [sna|foo]_handle);
>>
>>
>>
>> I found that even with SNA disabled, I had to comment out the above line of
>> code from the packet-sna.c source file before LLC would successfully hand
>> off dissection to FOO.
>>
>>
>>
>> To illustrate this, I’ve attached a very stripped down and slightly modified
>> version of packet-foo.c, along with a simple foo24.pcap file in case anyone
>> would care to take a look at it. I have since found an alternate solution,
>> but I was surprised that disabling a protocol does not seem to have the
>> completely desired effect. While the packet does not get dissected as SNA,
>> other dissectors are apparently not given the opportunity to dissect the
>> packet even when the SNA dissector is disabled.
>>
>>
>>
>> But beyond LLC and SNA, I was thinking/wondering that maybe this is a
>> general problem affecting all dissectors and that some general solution
>> might be needed?
>
> A couple of problems here:
> - When two protocols register with the same uint value in a table, the
> second one just overwrites the first.
> - When two protocols register with the same uint value in a table, we
> leak memory. It's visible in valgrind, as there are already a couple
> in the default build. Just run "libtool --mode=execute valgrind
> --leak-check=full ./tshark" and grep for "dissector_add_uint".
> - Even disabled protocols are registered, and thus are added to the
> appropriate table. This can overwrite enabled protocols, depending on
> registration order.
>
> I've got somewhere to be right now, but I'll try and do further
> analysis this weekend.
There is already a (commented-out) function called
dissector_add_uint_sanity_check which does warn on duplicate port
registrations and on registrations to port 0. It produces 157 warnings
when enabled in the default build. I don't know how many duplicate
string registrations there are, but there are definitely some based on
valgrind's output.
I have fixed the memory leaks (about 2K by default) in revision 44814.
This doesn't stop us from overwriting existing registrations, it just
prevents us from leaking memory when we do overwrite existing
registrations.
The fundamental problem is that registration happens once right when
the program starts, while protocols can be enabled/disabled
arbitrarily while an instance is running. Triggering a reregistration
on a protocol enable/disable would be painful, and I don't even know
if it would solve the problem.
A possible solution would be to store a linked list of dtbl_entries
for each registration instead of the single entry we currently store.
Then dissector_try_uint() can iterate through and try all of the
dtbl_entries whose associated protocols are marked as enabled. This
might lead to odd behaviour when multiple protocols are enabled for
the same port though... we have no way of knowing which one to pick,
so we'd have to be careful to avoid bad things (multiple protocols
dissecting the same tvb subset, or worse?). New-style dissectors that
do heuristics first might make that manageable though.
Thoughts?
Evan
