packet-radius.c - avp specific dissectors broken

This issue was migrated from bug 2974 in our old bug tracker.

Original bug information:

Reporter: Florian Lohoff
Status: RESOLVED FIXED
Product: Wireshark
Component: GTK+ UI
OS: All
Platform: x86
Version: Git

Attachments:

patch-init-avp-dissect.diff: change to register avp dissectors directly after loading the dictionary
radius-dict-load.patch: Dont leak preadded radius avps but use their hash/memory
radius-dict-load.patch: take2 of the dict load init patch

Florian Lohoff said:

Build Information: Version 1.1.2 (SVN Rev 26485)

Copyright 1998-2008 Gerald Combs <gerald@‍wireshark.org> and contributors. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Compiled with GTK+ 2.12.8, with GLib 2.16.2, with libpcap 0.9.8, with libz 1.2.3.3, with POSIX capabilities (Linux), with libpcre 7.6, without SMI, without c-ares, with ADNS, without Lua, with GnuTLS 2.4.1, with Gcrypt 1.4.0, with MIT Kerberos, with PortAudio V19-devel (built Oct 25 2007), without AirPcap.

Running on Linux 2.6.24-rc8, with libpcap version 0.9.8.

Built using gcc 4.2.2 (Debian 4.2.2-1).

Hi,

the radius avp specific dissectors registered with radius_register_avp_dissector are broken. The problem is that they are registered before the dictionary loads. The loading of the dictionary then replaces the previously added avps in the g_hash_table.

This functionality is used by packet-gtp.c, packet-radius_packetcable.c and from what i found it packet-radius.c itself.

I have used the following patch to solve this (as i am implementing a ascend-data-filter specific dissector right now).

I am unshure this is the right way as the order of init is not fixed as i understand it. Probably fixing dictionary loading to not overwrite the full avp but rather copying the dissector over would be more robust.

Flo

added uigtk versiondev labels

Florian Lohoff said:

Created attachment 2372 change to register avp dissectors directly after loading the dictionary

Jeff Morriss said:

Comment on attachment 2372 change to register avp dissectors directly after loading the dictionary

mark for review

Bill Meier said:

After looking at this:

I think the problem is that register_radius_fields() (which does the dictionary load) is being registered as a "delayed" field registration routine (via proto_register_prefix) and is also called during an actual dissection.
Therefore it would see that register_radius_fields [and thus radius_load_dictionary()]is called *after* all the proto_reg_handoff functions and thus apparently messes up any and all radius_register_avp_dissector calls done in proto_reg_handoff...

Thus: I suspect the patch fixes things for packet_radius itself but not for any other dissector which calls radius_register_avp_dissector() in a proto_reg_handoff function.

I'll leave this to someone who knows what packet-radius.c is doing.... (I've no clue).

Bill

Florian Lohoff said:

Created attachment 2376 Dont leak preadded radius avps but use their hash/memory

Florian Lohoff said:

Hi, patch 2 now has a different approach by not dropping previously added atributes and vendors on the floor (leak their memory) when the dictionary loads. As we typically dont know the vendor or attribute name thats the only thing we overwrite from the previous registering.

Flo

Florian Lohoff said:

Created attachment 2377 take2 of the dict load init patch

Fix the g_free of the name which is precomputed unknown-%‍d of the attribute or vendor. Init most/all of the parameters of the attribute as the dummy init for the dissector just add false/unknown and the dictionary should know better.

Anders Broman said:

Committed revision 27126.

closed