Wireshark · Wireshark-dev: Re: [Wireshark-dev] Patch for bug 1377 that produces a modal dialog with garbage

["Peter Johansson" <peterjohansson73@xxxxxxxxx>]

2007/3/30, Jeff Morriss <jeff.morriss@xxxxxxxxxxx>:

Peter Johansson wrote:
>     I compiled Wireshark with HAVE_AIRPDCAP by mistake (since I do not
>     have AirPcap). This leads to a runtime problem however. When
>     choosing "options" from the "Capture interfaces" dialog, I receive a
>     modal dialogue with an OK button with a textual description that is
>     only garbage (uninitialized memory).
>
>     The provided patch adds a new error - AIRPCAP_NOT_LOADED (2) code to
>     the airpcap loader that also adds the text "AirPcap was expected to
>     be loaded but is not" to the modal dialogue instead of the
>     uninitialized string.

Hmm, I think you found the problem of bug 1377:

http://bugs.wireshark.org/bugzilla/show_bug.cgi?id=1377

Having a quick look, though, I am a bit confused because it doesn't
appear anybody ever allocates any space for 'err_str' before calling
though maybe I'm missing it.  Anyway I don't really have time to dig
further for the moment.

Another thing that needs a look is it appears that it's normal that
AirPcap is compiled in (I didn't change my setup and my Windoze builds
say "with Airpcap") though in that case I'm not sure it should be
complaining at all if it doesn't find an AirPcap device or whatever.

After having read the description for bug 1377 I agree. This patch fixes that problem.

err_str should not be allocated prior to calling get_airpcap_interface_list(&err, &err_str). err_str gets assigned a pointer to allocated memory (g_strdup) in the called function that gets freed when returning from the function.

Thank you for having me review my changes once more. It turns out that my original change will make the code call g_free for the newly added statically allocated error description which would be a new error.
I have attached a new set of files to be supplied as a solution for bug 1377. This time my changes allocate the error description using g_strdup just like the rest of the code, making it possible to call g_free upon return from the get_airpcap_interface_list(...) function.

I was also wondering how to handle this case, should it not produce a dialogue at all? I am unsure. If a dialogue is not displayed, the user will never understand why the AirPcap devices do not show up if AirPcap is not loaded but Wireshark is compiled with support for it. On the other hand, I guess most users do not have AirPcap, which means that it would be annoying to get the dialogue every time when entering the options dialogue.

Therefor I have changed the code in airpcap_loader.c even further to produce the modal dialogue stating that AirPcap is not loaded only once per Wireshark session. Hence it will not get displayed more than once unless the user restarts Wireshark. Unfortunately I had to make changes to more files than I would have wanted to be able to do this.

Is that good enough?

Regards, Peter

Index: C:/wireshark-win32-libs/airpcap_loader.c
===================================================================
--- C:/wireshark-win32-libs/airpcap_loader.c	(revision 21279)
+++ C:/wireshark-win32-libs/airpcap_loader.c	(working copy)
@@ -1138,7 +1138,11 @@
     char errbuf[PCAP_ERRBUF_SIZE];
 
     if (!AirpcapLoaded)
-	return il;
+    {
+        *err = AIRPCAP_NOT_LOADED;
+        *err_str = g_strdup("AirPcap was expected to be loaded but is not");
+        return il;
+    }
 
     if (!g_PAirpcapGetDeviceList(&devsList, errbuf))
     {

Index: C:/wireshark-win32-libs/airpcap_loader.h
===================================================================
--- C:/wireshark-win32-libs/airpcap_loader.h	(revision 21279)
+++ C:/wireshark-win32-libs/airpcap_loader.h	(working copy)
@@ -33,6 +33,7 @@
 /* Error values from "get_airpcap_interface_list()". */
 #define	CANT_GET_AIRPCAP_INTERFACE_LIST	0	/* error getting list */
 #define	NO_AIRPCAP_INTERFACES_FOUND	1	/* list is empty */
+#define	AIRPCAP_NOT_LOADED	2	/* AirPcap not loaded */
 
 #define AIRPCAP_CHANNEL_ANY_NAME "ANY"
 

Index: C:/wireshark-win32-libs/gtk/capture_dlg.c
===================================================================
--- C:/wireshark-win32-libs/gtk/capture_dlg.c	(revision 21279)
+++ C:/wireshark-win32-libs/gtk/capture_dlg.c	(working copy)
@@ -639,7 +639,9 @@
   decryption_cm = OBJECT_GET_DATA(airpcap_tb,AIRPCAP_TOOLBAR_DECRYPTION_KEY);
   update_decryption_mode_list(decryption_cm);
 
-  if (airpcap_if_list == NULL && err == CANT_GET_AIRPCAP_INTERFACE_LIST) {
+  if (airpcap_if_list == NULL &&
+      (err == CANT_GET_AIRPCAP_INTERFACE_LIST ||
+       err == AIRPCAP_NOT_LOADED)) {
     simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK, "%s", err_str);
     g_free(err_str);
   }

Index: C:/wireshark-win32-libs/gtk/capture_if_dlg.c
===================================================================
--- C:/wireshark-win32-libs/gtk/capture_if_dlg.c	(revision 21279)
+++ C:/wireshark-win32-libs/gtk/capture_if_dlg.c	(working copy)
@@ -459,7 +459,9 @@
   decryption_cm = OBJECT_GET_DATA(airpcap_tb,AIRPCAP_TOOLBAR_DECRYPTION_KEY);
   update_decryption_mode_list(decryption_cm);
 
-  if (airpcap_if_list == NULL && err == CANT_GET_AIRPCAP_INTERFACE_LIST) {
+  if (airpcap_if_list == NULL &&
+      (err == CANT_GET_AIRPCAP_INTERFACE_LIST ||
+       err == AIRPCAP_NOT_LOADED)) {
     simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK, "%s", err_str);
     g_free(err_str);
   }

Index: C:/wireshark-win32-libs/gtk/main.c
===================================================================
--- C:/wireshark-win32-libs/gtk/main.c	(revision 21279)
+++ C:/wireshark-win32-libs/gtk/main.c	(working copy)
@@ -2180,7 +2180,9 @@
     /* load the airpcap interfaces */
     airpcap_if_list = get_airpcap_interface_list(&err, &err_str);
 
-    if (airpcap_if_list == NULL && err == CANT_GET_AIRPCAP_INTERFACE_LIST) {
+    if (airpcap_if_list == NULL &&
+        (err == CANT_GET_AIRPCAP_INTERFACE_LIST ||
+         err == AIRPCAP_NOT_LOADED)) {
       simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK, "%s", err_str);
       g_free(err_str);
     }