extcap: Use stderr to print error message

This patch reads out the stderr messages from an extcap
utility and displays it to an user. It was tested on Qt
but not on GTK, but should work their as well.

On Mac OS/X and Windows the child_watch does not behave
as it was intended. Therefore in extcap_cleanup, the callbacks
are called manually, if and only if, they have not been
called already.

The reason why it displays two error messages is, that
by the time the first one is being displayed, glib has not
returned from the spawned process on Linux yet. So there
is no way to add the stderr correctly, and putting a handler
to stderr into interface_opts will lead to memory errors,
cause then the code tries to access memory outside of its
protection.

Bug: 11892
Change-Id: I2db60dd480fed3e01428b91a705057e4f088bd15
Reviewed-on: https://code.wireshark.org/review/12954
Reviewed-by: Roland Knall <rknall@gmail.com>
Petri-Dish: Roland Knall <rknall@gmail.com>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Dario Lombardo <lomato@gmail.com>
Reviewed-by: Anders Broman <a.broman58@gmail.com>

8 files changed, 231 insertions, 31 deletions

diff --git a/capchild/capture_sync.c b/capchild/capture_sync.c
index 28c34a5542..5c62e2c437 100644
--- a/capchild/capture_sync.c
+++ b/capchild/capture_sync.c
@@ -1733,7 +1733,7 @@ sync_pipe_input_cb(gint source, gpointer user_data)
 #endif
 #ifdef HAVE_EXTCAP
         g_log(LOG_DOMAIN_CAPTURE, G_LOG_LEVEL_DEBUG, "sync_pipe_input_cb: cleaning extcap pipe");
-        extcap_if_cleanup(cap_session->capture_opts);
+        extcap_if_cleanup(cap_session->capture_opts, &primary_msg);
 #endif
         capture_input_closed(cap_session, primary_msg);
         g_free(primary_msg);
diff --git a/capture_opts.c b/capture_opts.c
index 2eb5b630d1..e014290f35 100644
--- a/capture_opts.c
+++ b/capture_opts.c
@@ -66,6 +66,7 @@ capture_opts_init(capture_options *capture_opts)
     capture_opts->default_options.extcap          = NULL;
     capture_opts->default_options.extcap_fifo     = NULL;
     capture_opts->default_options.extcap_args     = NULL;
+    capture_opts->default_options.extcap_userdata = NULL;
     capture_opts->default_options.extcap_pid      = INVALID_EXTCAP_PID;
 #endif
 #ifdef CAN_SET_CAPTURE_BUFFER_SIZE
@@ -1158,6 +1159,7 @@ collect_ifaces(capture_options *capture_opts)
 #ifdef HAVE_EXTCAP
             interface_opts.extcap = g_strdup(device.if_info.extcap);
             interface_opts.extcap_fifo = NULL;
+            interface_opts.extcap_userdata = NULL;
             interface_opts.extcap_args = device.external_cap_args_settings;
             interface_opts.extcap_pid = INVALID_EXTCAP_PID;
             if (interface_opts.extcap_args)
diff --git a/doc/extcap_example.py b/doc/extcap_example.py
index ab9967c489..9007e26bba 100755
--- a/doc/extcap_example.py
+++ b/doc/extcap_example.py
@@ -40,6 +40,7 @@ other script-based formates beside VBScript
 
 """
 
+from __future__ import print_function
 import os
 import sys
 import signal
@@ -52,12 +53,40 @@ from threading import Thread
 
 ERROR_USAGE		= 0
 ERROR_ARG 		= 1
-ERROR_INTERFACE = 2
+ERROR_INTERFACE	= 2
 ERROR_FIFO 		= 3
+ERROR_DELAY		= 4
 
 doExit = False
 globalinterface = 0
 
+"""
+This code has been taken from http://stackoverflow.com/questions/5943249/python-argparse-and-controlling-overriding-the-exit-status-code - originally developed by Rob Cowie http://stackoverflow.com/users/46690/rob-cowie
+"""
+class ArgumentParser(argparse.ArgumentParser):
+	def _get_action_from_name(self, name):
+		"""Given a name, get the Action instance registered with this parser.
+		If only it were made available in the ArgumentError object. It is
+		passed as it's first arg...
+		"""
+		container = self._actions
+		if name is None:
+			return None
+		for action in container:
+			if '/'.join(action.option_strings) == name:
+				return action
+			elif action.metavar == name:
+				return action
+			elif action.dest == name:
+				return action
+
+	def error(self, message):
+		exc = sys.exc_info()[1]
+		if exc:
+			exc.argument = self._get_action_from_name(exc.argument_name)
+			raise exc
+		super(ArgumentParser, self).error(message)
+
 def signalHandler(signal, frame):
 	global doExit
 	doExit = True
@@ -214,6 +243,23 @@ def extcap_capture(interface, fifo, delay, verify, message, remote, fake_ip):
 
 	fh.close()
 
+def extcap_close_fifo(interface, fifo):
+	global doExit
+
+	signal.signal(signal.SIGINT, signalHandler)
+	signal.signal(signal.SIGTERM , signalHandler)
+
+	tdelay = delay if delay != 0 else 5
+
+	try:
+		os.stat(fifo)
+	except OSError:
+		doExit = True
+		print ( "Fifo does not exist!" )
+
+	fh = open(fifo, 'w+b', 0 )
+	fh.close()
+
 ####
 
 def usage():
@@ -227,7 +273,7 @@ if __name__ == '__main__':
 	message = ""
 	fake_ip = ""
 
-	parser = argparse.ArgumentParser(
+	parser = ArgumentParser(
 		prog="Extcap Example",
 		description="Extcap example program for python"
 		)
@@ -243,12 +289,26 @@ if __name__ == '__main__':
 
 	# Interface Arguments
 	parser.add_argument("--verify", help="Demonstrates a verification bool flag", action="store_true" )
-	parser.add_argument("--delay", help="Demonstrates an integer variable", type=int, default=0, choices=[0, 1, 2, 3, 4, 5] )
+	parser.add_argument("--delay", help="Demonstrates an integer variable", type=int, default=0, choices=[0, 1, 2, 3, 4, 5, 6] )
 	parser.add_argument("--remote", help="Demonstrates a selector choice", default="if1", choices=["if1", "if2"] )
 	parser.add_argument("--message", help="Demonstrates string variable", nargs='?', default="" )
 	parser.add_argument("--fake_ip", help="Add a fake sender IP adress", nargs='?', default="127.0.0.1" )
 
-	args, unknown = parser.parse_known_args()
+	try:
+		args, unknown = parser.parse_known_args()
+	except argparse.ArgumentError, exc:
+		print( "%s: %s" % ( exc.argument.dest, exc.message ), file=sys.stderr)
+		fifo_found = 0
+		fifo = ""
+		for arg in sys.argv:
+			if (arg == "--fifo" or arg == "--extcap-fifo") :
+				fifo_found = 1
+			elif ( fifo_found == 1 ):
+				fifo = arg
+				break
+		extcap_close_fifo("", fifo)
+		sys.exit(ERROR_ARG)
+
 	if ( len(sys.argv) <= 1 ):
 		parser.exit("No arguments given!")
 
@@ -282,6 +342,12 @@ if __name__ == '__main__':
 	elif args.capture:
 		if args.fifo is None:
 			sys.exit(ERROR_FIFO)
+		# The following code demonstrates error management with extcap
+		if args.delay > 5:
+			print("Value for delay [%d] too high" % args.delay, file=sys.stderr)
+			extcap_close_fifo(interface, args.fifo)
+			sys.exit(ERROR_DELAY)
+
 		extcap_capture(interface, args.fifo, args.delay, args.verify, message, args.remote, fake_ip)
 	else:
 		usage()
diff --git a/dumpcap.c b/dumpcap.c
index 3d736ebfc0..935e52d9ca 100644
--- a/dumpcap.c
+++ b/dumpcap.c
@@ -1466,12 +1466,14 @@ cap_pipe_open_live(char *pipename,
 #else /* _WIN32 */
     char    *pncopy, *pos;
     wchar_t *err_str;
-    interface_options interface_opts;
 #ifdef HAVE_EXTCAP
     char* extcap_pipe_name;
-    gboolean extcap_pipe;
 #endif
 #endif
+#ifdef HAVE_EXTCAP
+    gboolean extcap_pipe = FALSE;
+#endif
+    interface_options interface_opts;
     ssize_t  b;
     int      fd = -1, sel_ret;
     size_t   bytes_read;
@@ -1498,7 +1500,15 @@ cap_pipe_open_live(char *pipename,
           return;
        }
     } else {
+
+        interface_opts = g_array_index(global_capture_opts.ifaces, interface_options, 0);
+
 #ifndef _WIN32
+#ifdef HAVE_EXTCAP
+        if ( g_strrstr(interface_opts.name, EXTCAP_PIPE_PREFIX) != NULL )
+            extcap_pipe = TRUE;
+#endif
+
         if (ws_stat64(pipename, &pipe_stat) < 0) {
             if (errno == ENOENT || errno == ENOTDIR)
                 pcap_opts->cap_pipe_err = PIPNEXIST;
@@ -1585,6 +1595,7 @@ cap_pipe_open_live(char *pipename,
             }
             return;
         }
+
 #else /* _WIN32 */
 #define PIPE_STR "\\pipe\\"
         /* Under Windows, named pipes _must_ have the form
@@ -1606,8 +1617,6 @@ cap_pipe_open_live(char *pipename,
             pcap_opts->cap_pipe_err = PIPNEXIST;
             return;
         }
-
-        interface_opts = g_array_index(global_capture_opts.ifaces, interface_options, 0);
 #ifdef HAVE_EXTCAP
         extcap_pipe_name = g_strconcat("\\\\.\\pipe\\", EXTCAP_PIPE_PREFIX, NULL);
         extcap_pipe = strstr(interface_opts.name, extcap_pipe_name) ? TRUE : FALSE;
@@ -1678,6 +1687,10 @@ cap_pipe_open_live(char *pipename,
                 b = cap_pipe_read(fd, ((char *)&magic)+bytes_read,
                                   sizeof magic-bytes_read,
                                   pcap_opts->from_cap_socket);
+                /* jump messaging, if extcap had an error, stderr will provide the correct message */
+                if (extcap_pipe && b <= 0)
+                    goto error;
+
                 if (b <= 0) {
                     if (b == 0)
                         g_snprintf(errmsg, errmsgl, "End of file on pipe magic during open.");
@@ -1704,6 +1717,10 @@ cap_pipe_open_live(char *pipename,
         /* We don't have to worry about cap_pipe_read_mtx here */
         g_async_queue_push(pcap_opts->cap_pipe_pending_q, pcap_opts->cap_pipe_buf);
         g_async_queue_pop(pcap_opts->cap_pipe_done_q);
+        /* jump messaging, if extcap had an error, stderr will provide the correct message */
+        if (pcap_opts->cap_pipe_bytes_read <= 0 && extcap_pipe)
+            goto error;
+
         if (pcap_opts->cap_pipe_bytes_read <= 0) {
             if (pcap_opts->cap_pipe_bytes_read == 0)
                 g_snprintf(errmsg, errmsgl, "End of file on pipe magic during open.");
diff --git a/extcap.c b/extcap.c
index 4fbf5d79e0..6087224df4 100644
--- a/extcap.c
+++ b/extcap.c
@@ -58,6 +58,7 @@ static HANDLE pipe_h = NULL;
 #endif
 
 #define EXTCAP_PREF_SIZE 256
+static void extcap_child_watch_cb(GPid pid, gint status _U_, gpointer user_data);
 
 /* internal container, for all the extcap interfaces that have been found.
  * will be resetted by every call to extcap_interface_list() and is being
@@ -643,9 +644,44 @@ extcap_has_configuration(const char * ifname, gboolean is_required) {
     return found;
 }
 
-void extcap_if_cleanup(capture_options * capture_opts) {
+/* taken from capchild/capture_sync.c */
+static gboolean pipe_data_available(int pipe_fd) {
+#ifdef _WIN32 /* PeekNamedPipe */
+    HANDLE hPipe = (HANDLE) _get_osfhandle(pipe_fd);
+    DWORD bytes_avail;
+
+    if (hPipe == INVALID_HANDLE_VALUE)
+        return FALSE;
+
+    if (! PeekNamedPipe(hPipe, NULL, 0, NULL, &bytes_avail, NULL))
+        return FALSE;
+
+    if (bytes_avail > 0)
+        return TRUE;
+    return FALSE;
+#else /* select */
+    fd_set rfds;
+    struct timeval timeout;
+
+    FD_ZERO(&rfds);
+    FD_SET(pipe_fd, &rfds);
+    timeout.tv_sec = 0;
+    timeout.tv_usec = 0;
+
+    if (select(pipe_fd+1, &rfds, NULL, NULL, &timeout) > 0)
+        return TRUE;
+
+    return FALSE;
+#endif
+}
+
+void extcap_if_cleanup(capture_options * capture_opts, gchar ** errormsg) {
     interface_options interface_opts;
+    extcap_userdata * userdata;
     guint icnt = 0;
+    gboolean overwrite_exitcode;
+    gchar * buffer;
+#define STDERR_BUFFER_SIZE 1024
 
     for (icnt = 0; icnt < capture_opts->ifaces->len; icnt++) {
         interface_opts = g_array_index(capture_opts->ifaces, interface_options,
@@ -655,6 +691,8 @@ void extcap_if_cleanup(capture_options * capture_opts) {
         if (interface_opts.if_type != IF_EXTCAP)
             continue;
 
+        overwrite_exitcode = FALSE;
+
         g_log(LOG_DOMAIN_CAPTURE, G_LOG_LEVEL_DEBUG,
                 "Extcap [%s] - Cleaning up fifo: %s; PID: %d", interface_opts.name,
                 interface_opts.extcap_fifo, interface_opts.extcap_pid);
@@ -681,6 +719,66 @@ void extcap_if_cleanup(capture_options * capture_opts) {
                 "Extcap [%s] - Closing spawned PID: %d", interface_opts.name,
                 interface_opts.extcap_pid);
 
+        userdata = (extcap_userdata *) interface_opts.extcap_userdata;
+        if ( userdata )
+        {
+            if (userdata->extcap_stderr_rd > 0 && pipe_data_available(userdata->extcap_stderr_rd) )
+            {
+                buffer = (gchar * )g_malloc0(sizeof(gchar) * STDERR_BUFFER_SIZE);
+#ifdef _WIN32
+                win32_readfrompipe((HANDLE)_get_osfhandle(userdata->extcap_stderr_rd), STDERR_BUFFER_SIZE, buffer);
+#else
+                if (read(userdata->extcap_stderr_rd, buffer, sizeof(gchar) * STDERR_BUFFER_SIZE) <= 0 )
+                    buffer[0] = '\0';
+#endif
+                if ( strlen ( buffer) > 0 )
+                {
+                    userdata->extcap_stderr = g_strdup_printf("%s", buffer);
+                    userdata->exitcode = 1;
+                }
+                g_free(buffer);
+            }
+        }
+
+#ifndef _WIN32
+        /* Final child watch may not have been called */
+        if ( interface_opts.extcap_child_watch != 0 )
+        {
+            extcap_child_watch_cb(userdata->pid, 0, capture_opts);
+            /* it will have changed in extcap_child_watch_cb */
+            interface_opts = g_array_index(capture_opts->ifaces, interface_options,
+                            icnt);
+        }
+#endif
+
+        if ( userdata->extcap_stderr != NULL )
+            overwrite_exitcode = TRUE;
+
+        if ( overwrite_exitcode || userdata->exitcode != 0 )
+        {
+            if ( userdata->extcap_stderr != 0 )
+            {
+                if ( *errormsg == NULL )
+                    *errormsg = g_strdup_printf("Error by extcap pipe: %s", userdata->extcap_stderr);
+                else
+                {
+                    gchar * temp = g_strconcat ( *errormsg, "\nError by extcap pipe: " ,userdata->extcap_stderr, NULL );
+                    g_free(*errormsg);
+                    *errormsg = temp;
+                }
+                g_free (userdata->extcap_stderr );
+            }
+
+            userdata->extcap_stderr = NULL;
+            userdata->exitcode = 0;
+        }
+
+        if (interface_opts.extcap_child_watch > 0)
+        {
+            g_source_remove(interface_opts.extcap_child_watch);
+            interface_opts.extcap_child_watch = 0;
+        }
+
         if (interface_opts.extcap_pid != INVALID_EXTCAP_PID)
         {
 #ifdef _WIN32
@@ -688,6 +786,9 @@ void extcap_if_cleanup(capture_options * capture_opts) {
 #endif
             g_spawn_close_pid(interface_opts.extcap_pid);
             interface_opts.extcap_pid = INVALID_EXTCAP_PID;
+
+            g_free(interface_opts.extcap_userdata);
+            interface_opts.extcap_userdata = NULL;
         }
 
         /* Make sure modified interface_opts is saved in capture_opts. */
@@ -713,11 +814,16 @@ extcap_add_arg_and_remove_cb(gpointer key, gpointer value, gpointer data) {
     return FALSE;
 }
 
-static void extcap_child_watch_cb(GPid pid, gint status _U_, gpointer user_data)
+void extcap_child_watch_cb(GPid pid, gint status _U_, gpointer user_data)
 {
     guint i;
     interface_options interface_opts;
-    capture_options *capture_opts = (capture_options *)user_data;
+    GError * error = 0;
+    extcap_userdata * userdata = NULL;
+    capture_options * capture_opts = (capture_options *)(user_data);
+
+    if ( capture_opts == NULL || capture_opts->ifaces == NULL || capture_opts->ifaces->len == 0 )
+        return;
 
     /* Close handle to child process. */
     g_spawn_close_pid(pid);
@@ -728,7 +834,16 @@ static void extcap_child_watch_cb(GPid pid, gint status _U_, gpointer user_data)
         interface_opts = g_array_index(capture_opts->ifaces, interface_options, i);
         if (interface_opts.extcap_pid == pid)
         {
-            interface_opts.extcap_pid = INVALID_EXTCAP_PID;
+            userdata = (extcap_userdata *)interface_opts.extcap_userdata;
+            if ( userdata != NULL )
+            {
+                interface_opts.extcap_pid = INVALID_EXTCAP_PID;
+                userdata->exitcode = 0;
+                if ( ! g_spawn_check_exit_status(status, &error) )
+                    userdata->exitcode = error->code;
+                if ( status == 0 && userdata->extcap_stderr != NULL )
+                    userdata->exitcode = 1;
+            }
             g_source_remove(interface_opts.extcap_child_watch);
             interface_opts.extcap_child_watch = 0;
 
@@ -844,6 +959,7 @@ extcap_init_interfaces(capture_options *capture_opts)
 {
     guint i;
     interface_options interface_opts;
+    extcap_userdata * userdata;
 
     for (i = 0; i < capture_opts->ifaces->len; i++)
     {
@@ -863,22 +979,23 @@ extcap_init_interfaces(capture_options *capture_opts)
         /* Create extcap call */
         args = extcap_prepare_arguments(interface_opts);
 
-        interface_opts.extcap_userdata = NULL;
+        userdata = g_new0(extcap_userdata, 1);
 
-        pid = extcap_spawn_async(&interface_opts, args );
+        pid = extcap_spawn_async(userdata, args );
 
         g_ptr_array_foreach(args, (GFunc)g_free, NULL);
         g_ptr_array_free(args, TRUE);
 
         if ( pid == INVALID_EXTCAP_PID )
+        {
+            g_free(userdata);
             continue;
+        }
 
         interface_opts.extcap_pid = pid;
 
         interface_opts.extcap_child_watch =
             g_child_watch_add(pid, extcap_child_watch_cb, (gpointer)capture_opts);
-        capture_opts->ifaces = g_array_remove_index(capture_opts->ifaces, i);
-        g_array_insert_val(capture_opts->ifaces, i, interface_opts);
 
 #ifdef _WIN32
         /* On Windows, wait for extcap to connect to named pipe.
@@ -894,7 +1011,13 @@ extcap_init_interfaces(capture_options *capture_opts)
         {
             extcap_wait_for_pipe(pipe_h, pid);
         }
+
 #endif
+
+        interface_opts.extcap_userdata = (gpointer) userdata;
+
+        capture_opts->ifaces = g_array_remove_index(capture_opts->ifaces, i);
+        g_array_insert_val(capture_opts->ifaces, i, interface_opts);
     }
 
     return TRUE;
diff --git a/extcap.h b/extcap.h
index d8e94b9fad..254bd9fa3a 100644
--- a/extcap.h
+++ b/extcap.h
@@ -28,8 +28,6 @@
 
 #include <glib.h>
 
-#include "ws_symbol_export.h"
-
 #ifdef _WIN32
 #include <windows.h>
 #include <wsutil/unicode-utils.h>
@@ -102,7 +100,7 @@ extcap_create_pipe(char ** fifo);
 
 /* Clean up all if related stuff */
 void
-extcap_if_cleanup(capture_options * capture_opts _U_);
+extcap_if_cleanup(capture_options * capture_opts _U_, gchar ** errormsg);
 
 struct preference *
 extcap_pref_for_argument(const gchar *ifname, struct _extcap_arg * arg);
diff --git a/extcap_spawn.c b/extcap_spawn.c
index 555958c687..484269370c 100644
--- a/extcap_spawn.c
+++ b/extcap_spawn.c
@@ -197,7 +197,7 @@ gboolean extcap_spawn_sync ( gchar * dirname, gchar * command, gint argc, gchar
     return result;
 }
 
-GPid extcap_spawn_async(interface_options * interface, GPtrArray * args)
+GPid extcap_spawn_async(extcap_userdata * userdata, GPtrArray * args)
 {
     GPid pid = INVALID_EXTCAP_PID;
 
@@ -221,12 +221,6 @@ GPid extcap_spawn_async(interface_options * interface, GPtrArray * args)
     const gchar * oldpath = g_getenv("PATH");
     gchar * newpath = NULL;
 
-#endif
-
-    extcap_userdata * userdata = NULL;
-    userdata = (extcap_userdata *) g_malloc0(sizeof(extcap_userdata));
-
-#ifdef _WIN32
     newpath = g_strdup_printf("%s;%s", g_strescape(get_progfile_dir(), NULL), oldpath);
     g_setenv("PATH", newpath, TRUE);
 
@@ -279,12 +273,12 @@ GPid extcap_spawn_async(interface_options * interface, GPtrArray * args)
 
     g_setenv("PATH", oldpath, TRUE);
 #else
-    g_spawn_async(NULL, (gchar **)args->pdata, NULL, (GSpawnFlags) G_SPAWN_DO_NOT_REAP_CHILD, NULL, NULL,
-            &pid, NULL);
+    g_spawn_async_with_pipes(NULL, (gchar **)args->pdata, NULL,
+            (GSpawnFlags) G_SPAWN_DO_NOT_REAP_CHILD, NULL, NULL,
+            &pid, NULL, &userdata->extcap_stdout_rd, &userdata->extcap_stderr_rd, NULL);
 #endif
 
     userdata->pid = pid;
-    interface->extcap_userdata = userdata;
 
     return pid;
 }
diff --git a/extcap_spawn.h b/extcap_spawn.h
index b9b2ef491e..16d8a97a58 100644
--- a/extcap_spawn.h
+++ b/extcap_spawn.h
@@ -44,7 +44,7 @@ typedef struct _extcap_userdata {
 
 gboolean extcap_spawn_sync ( gchar * dirname, gchar * command, gint argc, gchar ** argv, gchar ** command_output );
 
-GPid extcap_spawn_async ( interface_options * interface, GPtrArray * args );
+GPid extcap_spawn_async ( extcap_userdata * userdata, GPtrArray * args );
 
 #ifdef _WIN32
 gboolean extcap_wait_for_pipe(HANDLE pipe_h, HANDLE pid);