Evan Huus <eapache@...> writes:
> > A couple of problems here:
> > - When two protocols register with the same uint value in a table, the
> > second one just overwrites the first.
If the first one is disabled, I'd say that's OK, but if not, then maybe the
better thing to do is not to register the new one? At least this way there
would be a way to disable dissectors, thus giving other dissectors a chance at
decoding packets, even if this means having to restart Wireshark to achieve
this. But I'd say this would probably fall into a decent enough temporary
solution, but it's not ideal.
> > - Even disabled protocols are registered, and thus are added to the
> > appropriate table. This can overwrite enabled protocols, depending on
> > registration order.
Yeah, it would be nice if at least this situation could be avoided.
> 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.
My understanding of dissectors registering to port 0 was simply a method for
allowing "Decode As" to work. Those *probably* aren't a problem. The others
would likely not yield expected results though.
> 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.
Thanks! :)
> 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.
I agree, this would probably be tedious, essentially adding de-registration
routines to all dissectors, which are then called whenever the respective
protocol is disabled. But I would think that it would solve the basic problem
though, even if it means having to potentially disable multiple protocols in
order to finally get to the one you want, depending upon how many dissectors are
registered to the same uint value. That isn't all that elegant of course, but
it would work, which is better than what we have today.
> 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
Maintaining a linked list seems like a good idea to me and a lot easier than
adding de-registration routines to 1000+ dissectors. Until all dissectors are
fully converted to new-style ones, maybe they could all be "forcibly" converted
by simply having them return the number of bytes in the tvb that are handed to
them? Then slowly they could really be converted to actually perform heuristics
and return the actual number of bytes dissected.
- Chris