Crash when loading capture file and Preferences: NO Info column

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

Original bug information:

Reporter: Sandra Fiehe
Status: RESOLVED FIXED
Product: Wireshark
Component: GTK+ UI
OS: Windows XP
Platform: x86
Version: unspecified

Attachments:

col_info.diff: missing check_col() before calling col_xxx() functions
dissector_bug.diff: replace g_assert() with DISSECTOR_ASSERT() in column util functions

Sandra Fiehe said:

Build Information: Tested with different Versions: --------------------- Wireshark Version 0.99.6a (SVN Rev 22276)

Copyright 1998-2007 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.10.12, with GLib 2.12.12, with WinPcap (version unknown), with libz 1.2.3, with libpcre 6.4, with Net-SNMP 5.4, with ADNS, with Lua 5.1, with GnuTLS 1.6.1, with Gcrypt 1.2.3, with MIT Kerberos, with PortAudio PortAudio V19-devel, with AirPcap.

Running on Windows XP Service Pack 3, build 2600, with WinPcap version 4.0.2 (packet.dll version 4.0.0.1040), based on libpcap version 0.9.5, without AirPcap.

Built using Microsoft Visual C++ 6.0 build 8804

----------

Wireshark dev. Version Version 1.1.0 (SVN Rev 26170)

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.14.1, with GLib 2.18.0, with WinPcap (version unknown), with libz 1.2.3, without POSIX capabilities, with libpcre 7.0, with SMI 0.4.8, with c-ares 1.5.2, without ADNS, with Lua 5.1, with GnuTLS 2.3.8, with Gcrypt 1.4.1, with MIT Kerberos, with PortAudio V19-devel (built Sep 10 2008), with AirPcap.

Running on Windows XP Service Pack 3, build 2600, with WinPcap version 4.0.2 (packet.dll version 4.0.0.1040), based on libpcap version 0.9.5, without AirPcap.

Built using Microsoft Visual C++ 9.0 build 30729

----------

Wireshark Version 1.0.3 (SVN Rev 26134)

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.14.6, with WinPcap (version unknown), with libz 1.2.3, without POSIX capabilities, with libpcre 7.0, with SMI 0.4.8, with ADNS, with Lua 5.1, with GnuTLS 2.3.8, with Gcrypt 1.4.1, with MIT Kerberos, with PortAudio V19-devel, with AirPcap.

Running on Windows XP Service Pack 3, build 2600, with WinPcap version 4.1 beta (packet.dll version 4.1.0.902), based on libpcap version 0.9.6 branch, with AirPcap 3.2.1 build 1069.

Built using Microsoft Visual C++ 6.0 build 8804

Overview: Wireshark crashes during opening/loading a capture-file if the info column is deleted via Preferences/other Profile.

------

Detailed Description:
Wireshark on computer with AirPcap (where trace was taken)
Version 1.0.3 (SVN Rev 26134)
Problem:
When loading/opening *-pcap file
- with default preferences: ok, tracefile completely loadable
- with more columns but WITHOUT Info column: Freeze when loading capture file.
Wireshark Debug Console: Err  file column-utils.c: line 542: assertion failed: (cinfo->
col_first[el] >= 0)
---
Wireshark dev.Version 1.1.0 (SVN Rev 26170)
Problem:
When loading/opening same *-pcap file, Wireshark crashes.
Description:
Window appears, Windowname: "Microsoft Visual C++ Runtime Library"
Error message: "Runtime Error!
Program: C:\Program Files\Wireshark110\wireshark.exe
This application has requested the Runtime to terminate it in an unusual way. Please contact the application's support team for more information."
---
Wireshark Version 0.99.6a (SVN Rev 22276):
Problem:
When loading/opening same *-pcap file, Wireshark crashes.
Wireshark Debug Console:
10:01:11          Err  file column-utils.c: line 472: assertion failed: (cinfo->
col_first[el] >= 0)

------

This problem occurs with some (about 20-30%) of my 802.11n capture files taken of Iperf UDP traffic, captured with AirPcapN and Wireshark.

This error is 100% reproducible with the concerned file(s). On all listed Versions of Wireshark.

added crash oswindows uigtk labels

Stephen Fisher said:

Would it be possible for you to attach a capture file or two that exhibits this problem? My first guess is that the problem is in the dissector for a particular protocol that doesn't check for the info column being present before displaying something in it. We can find out for sure with capture files and testing on our local systems. Thanks!

Jaap Keuter said:

Sandra, we would really like to see a capture showing this behavior. Otherwise it's virtually impossible to track down the cause Stephen described.

didier gautheron said:

(In reply to comment #2)  
> Sandra, we would really like to see a capture showing this behavior. Otherwise  
> it's virtually impossible to track down the cause Stephen described.  
>  

there's at least:
packet-ms-mms.c,  packet-rdt.c, packet-vj.c, typo: test is done on COL_PROTOCOL
(for vj.c I think it's the other way, COL_INFO is used in col_set_str when it should be COL_PROTOCOL).

plugins/infiniband/packet-infiniband.c

and maybe plugins like wimax, some functions test only if pinfo->cinfo != NULL rather than using check_col() but they aren't always called from the same source file so I'm not sure (my checker isn't smart enough).

Jaap Keuter said:

Well, that's at least a start. Thanks Didier.

Jaap Keuter said:

Revision 27191 addresses the normal plugins. Revision 27192 addresses WiMax. InfiniBand still in the works.

Have we found any more?

didier gautheron said:

(In reply to comment #5)  
> Revision 27191 addresses the normal plugins.  
> Revision 27192 addresses WiMax.  
> InfiniBand still in the works.  
>  
> Have we found any more?  
>  

I will rerun my checker but I get a lot of suspicious files flagged so I have to manually recheck them.

BTW why aren't we using DISSECTOR_ASSERT()? It'd print the offending dissector name.

Jaap Keuter said:

Revision 27199 addresses InfiniBand.

didier gautheron said:

Created attachment 2675
missing check_col() before calling col_xxx() functions

I'm not sure about tshark
./tap-protocolinfo.c in protocolinfo_packet()

and a patch for:
 ./plugins/m2m/packet-m2m.c
 ./epan/dissectors/packet-ncp2222.inc (COL_PROTOCOL)

Jaap Keuter said:

Yes, all three needed to be addressed. Committed 27222, 27223 & 27224.

didier gautheron said:

Created attachment 2678 replace g_assert() with DISSECTOR_ASSERT() in column util functions

For the next time or if this one is still not fixed... DISSECTOR_ASSERT() is more verbose.

Jaap Keuter said:

Why not put the check into the API? This check has to be done always, so it might as well be done there. It removes the burden of checking the columns writability from the application (dissector/tap), just like was done for the proto_tree_add_xxx(tree, ...) where the check for tree!=NULL was made obsolete by adding it to the API, hence cleaning up the code a lot.

didier gautheron said:

(In reply to comment #11)  
> Why not put the check into the API? This check has to be done always, so it  
> might as well be done there. It removes the burden of checking the columns  
> writability from the application (dissector/tap), just like was done for the  
> proto_tree_add_xxx(tree, ...) where the check for tree!=NULL was made obsolete  
> by adding it to the API, hence cleaning up the code a lot.  
>  

Yes it's an option.

But:
- by moving check_col inside col_xxx functions there's a small semantic change. something like:
if (check_col(...) {
   while(foo) {
      col_append_xxx(....),
      if (bar) col_set_writable(..., FALSE);
      ...
   }
}
is currently possible.

- if we clean up the code, it could make backporting patches to the 1.0 branch harder.

Jaap Keuter said:

(In reply to comment #12)  
> (In reply to comment #11)  
> > Why not put the check into the API?  
>  
> Yes it's an option.  
>  
> But:  
> - by moving check_col inside col_xxx functions there's a small semantic change.  
> something like:  
> if (check_col(...) {  
>    while(foo) {  
>       col_append_xxx(....),  
>       if (bar) col_set_writable(..., FALSE);  
>       ...  
>    }  
> }  
> is currently possible.  

Yes, but is that actually used? And even when it is, it should be easily fixed.

> - if we clean up the code, it could make backporting patches to the 1.0 branch  
> harder.  

I would not like backporting issues holding us back from development of our APIs. And even then, when adding this check into the API we can leave the existing dissector code 'as is', and recommend this only for new code.
That saves us clean up work (we just don't do that) and we keep easier backportable code. In short: Work less, gain more :)

Stig Bjørlykke said:

(In reply to comment #‍11)
> Why not put the check into the API?

I have added this check in revision 27806.

Stig Bjørlykke said:

Comment on attachment 2678 replace g_assert() with DISSECTOR_ASSERT() in column util functions

This is no longer needed, as we use check_col() instead.

Stig Bjørlykke said:

Comment on attachment 2675 missing check_col() before calling col_xxx() functions

This has already been committed.

Jaap Keuter said:

From bug #3870 (closed): a new variant pops up in packet-dns.c and packet-nbns.c. Just a check on (cinfo != NULL), no specific column check. This must be fixed in 1.2.2 especially.

Stig Bjørlykke said:

(In reply to comment #‍17)
> From bug #3870 (closed): a new variant pops up in packet-dns.c and packet-nbns.c. Just a
> check on (cinfo != NULL), no specific column check.

Please note that cinfo is only set if having COL_INFO (dissect_dns_common and dissect_nbns), and is used for more than just checking if the column is present.

I think the code in packet-dns.c and packet-nbns.c is valid as it is.