Tshark crashes when g_free is called when duplicate Radius vendor present

Question

In function add_vendor the below code gets executed if vendor info was already there. Why when a pointer is freed after a valid vendor info is found, g_free() crashing?

    if (v->name)
            g_free((gpointer) v->name);
    v->name = g_strdup(name);

We are seeing crash when g_free is called.

(gdb) where
#0  0xf77d1430 in __kernel_vsyscall ()
#1  0x41051daf in raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64
#2  0x41055305 in abort () at abort.c:91
#3  0x41095d2b in malloc_printerr (action=<value optimized out>, str=<value optimized out>, ptr=0xf4e48d00) at malloc.c:5012
#4  0x4109a05b in __libc_free (mem=0xf4e48d00) at malloc.c:2959
#5  0x4128344b in standard_free (mem=0xf4e48d00) at gmem.c:98
#6  0x41283670 in g_free (mem=0xf4e48d00) at gmem.c:252
#7  0xf5dc9580 in add_vendor (name=0x97f7c00 "3GPP", vendor_id=10415, vendor_type_octets=1, vendor_length_octets=1, vendor_has_flags=0)
    at ../../../../tshark/wireshark-1.2.9/epan/radius_dict.l:318

Answer 1

1

[Updated after noticing this was Radius not Diameter--oops!]

Well, looking at the code it appears that:

The vendor has been found based on the vendor ID (not name).
The code wants to use whatever the latest vendor information it read was (so it overwrites the old one, this includes changing the name--note that the name may be different).
(But, because the name is g_ allocated, it has to be g_free'd or it would leak memory.)

This code is different in the current development version--and it was changed to fix a crash (see change 5265).

You've got a couple options:

Don't put duplicate vendor IDs in the XML
Upgrade to a modern version of Wireshark (1.2.9 is ancient). The current development version is likely your best bet.

answered 23 Apr '15, 13:49

JeffMorriss ♦
6.2k●5●72
accept rate: 27%

edited 23 Apr '15, 13:56

Ok, if it has already found vendor that means it has a valid vendor name right? Why g_free has to crash when freeing a valid pointer? Is it a good idea to replace char * with a static array?

(23 Apr '15, 14:23) universini

It's not clear why it's crashing. Note that glib should actually be printing something (to stderr) to explain what's wrong with the free though that may not help explain too much.

The problem with a static char array is it's more restrictive: what happens if your vendor name is too long (Wireshark currently has a Diameter vendor whose name is "NokiaSolutionsAndNetworks").

As mentioned, the crash may be related to the fix described or it may not. But I don't think it's worth spending a lot of time investigating such an ancient version, especially when there's a simple solution: don't have a duplicated vendor.

If it crashes in the currently-supported versions and/or development version then it would be worth investigating (I tried it with a duplicated VENDOR line and it worked fine--Valgrind didn't complain either).

(23 Apr '15, 14:50) JeffMorriss ♦

It says Abort with signal 6

(23 Apr '15, 15:39) universini

By the way, I am confused with .l and .c versions of the same file. From my understanding it looks like changes should only be done in .l file and .c will be automatically generated when built. Is that right? or don't we care .c at all?

(24 Apr '15, 22:30) universini

It's a (f)lex parser. So humans write the .l file which (f)lex turns into the .c file which the compiler turns into an object file.

We (humans) don't need to worry about the .c file--unless you're working on the build system (Makefiles).

(27 Apr '15, 07:53) JeffMorriss ♦

So always make changes in .l file and make sure new .c file is generated with latest changes, right? If .c file isn't recreated with new changes, then changes won't work. Is that the way it is?

(27 Apr '15, 11:18) universini

So always make changes in .l file and make sure new .c file is generated with latest changes, right?

Correct.

If .c file isn't recreated with new changes, then changes won't work.

Correct.

Is that the way it is?

Yes.

(27 Apr '15, 12:20) Guy Harris ♦♦

showing 5 of 7 show 2 more comments