Wireshark · Wireshark-dev: Re: [Wireshark-dev] Patch for empty SSH env variables

[Jakub Zawadzki <darkjames@xxxxxxxxxxxxxxxx>]

Hi,

On Mon, Oct 05, 2009 at 05:37:00PM -0400, Charles Rumford wrote:
> A friend of mine had been having the same issue, and we decided to dive into
> the code and figure out what was going one. It turns out that ssh-agent set
> the SSH_CONNECTION and SSH_CLIENT environment variables to the empty string
> when on the local machine.  Because of this in util.c, in *get_conn_cfilter(),
> tokens[0] is null and there is no check to get around this.
> 
> I have attached patch to fix this problem. Does this seem like a reasonable
> fix for this problem?

> -		if (tokens[3]) {
> +		if (tokens[3] && tokens[0]) {

I'd prefer to use if (g_strv_length(tokens) == 4) in SSH_CONNECTION,
and if (g_strv_length(tokens) == 3) in SSH_CLIENT

And there are missing g_strfreev(tokens) call.

I rewritten get_conn_cfilter() to use g_strdup_printf() instead of static g_string.
(I removed const from declaration, and make it return NULL when it fail to create filter instad of "")

And when it fail to obtain filter from one source, it will try with another one.
(in DISPLAY block there was lot of returns, so I moved SESSIONNAME above)

Later we can try to create filter from multiple sources:
  like SSH && DISPLAY.

I think it should work, but I don't have time to test it :)

Regards.

diff --git gtk/main.c gtk/main.c
index 714d9d9..6155d9e 100644
--- gtk/main.c
+++ gtk/main.c
@@ -1820,7 +1820,7 @@ main(int argc, char *argv[])
   gboolean             arg_error = FALSE;
 
   extern int           splash_register_freq;  /* Found in about_dlg.c */
-  const gchar         *filter;
+  gchar               *filter;
 
 #ifdef _WIN32
   WSADATA 	       wsaData;
@@ -2129,8 +2129,10 @@ main(int argc, char *argv[])
 
   /* Non-blank filter means we're remote. Throttle splash screen updates. */
   filter = get_conn_cfilter();
-  if ( *filter != '\0' )
+  if (filter != NULL) {
     splash_register_freq = 1000;  /* Milliseconds */
+    g_free(filter);
+  }
 
   /* We won't come till here, if we had a "console only" command line parameter. */
   splash_win = splash_new("Loading Wireshark ...");
@@ -2727,7 +2729,10 @@ main(int argc, char *argv[])
     /* if the user didn't supplied a capture filter, use the one to filter out remote connections like SSH */
     if (!start_capture && strlen(global_capture_opts.cfilter) == 0) {
       g_free(global_capture_opts.cfilter);
-      global_capture_opts.cfilter = g_strdup(get_conn_cfilter());
+      global_capture_opts.cfilter = get_conn_cfilter();
+
+      if (!global_capture_opts.cfilter)
+        global_capture_opts.cfilter = g_strdup("");
     }
 #else /* HAVE_LIBPCAP */
     show_main_window(FALSE);
diff --git util.c util.c
index a86b48a..d628ef9 100644
--- util.c
+++ util.c
@@ -138,40 +138,59 @@ compute_timestamp_diff(gint *diffsec, gint *diffusec,
    SESSIONNAME (terminal server): <remote name>
  */
 
-const gchar *get_conn_cfilter(void) {
-	static GString *filter_str = NULL;
+gchar *get_conn_cfilter(void) {
 	gchar *env, **tokens;
 	char *lastp, *lastc, *p;
 	char *pprotocol = NULL;
 	char *phostname = NULL;
 	size_t hostlen;
 
-	if (filter_str == NULL) {
-		filter_str = g_string_new("");
-	}
-	if ((env = getenv("SSH_CONNECTION")) != NULL) {
+	gchar *res = NULL;
+
+	if (/* !res && */ (env = getenv("SSH_CONNECTION")) != NULL) {
 		tokens = g_strsplit(env, " ", 4);
-		if (tokens[3]) {
-			g_string_printf(filter_str, "not (tcp port %s and %s host %s "
-							 "and tcp port %s and %s host %s)", tokens[1], host_ip_af(tokens[0]), tokens[0],
-				tokens[3], host_ip_af(tokens[2]), tokens[2]);
-			return filter_str->str;
-		}
-	} else if ((env = getenv("SSH_CLIENT")) != NULL) {
+		if (g_strv_length(tokens) == 4)
+			res = g_strdup_printf("not (tcp port %s and %s host %s and tcp port %s and %s host %s)", 
+					tokens[1], host_ip_af(tokens[0]), tokens[0],
+					tokens[3], host_ip_af(tokens[2]), tokens[2]);
+		g_strfreev(tokens);
+	}
+
+	if (!res && (env = getenv("SSH_CLIENT")) != NULL) {
 		tokens = g_strsplit(env, " ", 3);
-		g_string_printf(filter_str, "not (tcp port %s and %s host %s "
-			"and tcp port %s)", tokens[1], host_ip_af(tokens[0]), tokens[0], tokens[2]);
-		return filter_str->str;
-	} else if ((env = getenv("REMOTEHOST")) != NULL) {
+		if (g_strv_length(tokens) == 3)
+			res = g_strdup_printf("not (tcp port %s and %s host %s and tcp port %s)", 
+					tokens[1], host_ip_af(tokens[0]),
+					tokens[0], tokens[2]);
+		g_strfreev(tokens);
+	}
+
+	if (!res && (env = getenv("REMOTEHOST")) != NULL) {
 		/* FreeBSD 7.0 sets REMOTEHOST to an empty string */
-		if (g_ascii_strcasecmp(env, "localhost") == 0 ||
-		    strcmp(env, "127.0.0.1") == 0 ||
-		    strcmp(env, "") == 0) {
-			return "";
+		if (g_ascii_strcasecmp(env, "localhost") != 0 &&
+		    strcmp(env, "127.0.0.1") != 0 &&
+		    strcmp(env, "") != 0)
+		{
+			res = g_strdup_printf("not %s host %s", host_ip_af(env), env);
 		}
-		g_string_printf(filter_str, "not %s host %s", host_ip_af(env), env);
-		return filter_str->str;
-	} else if ((env = getenv("DISPLAY")) != NULL) {
+	}
+
+	if (!res && (env = getenv("SESSIONNAME")) != NULL) {
+		/* Apparently the KB article at
+		 * http://technet2.microsoft.com/WindowsServer/en/library/6caf87bf-3d70-4801-9485-87e9ec3df0171033.mspx?mfr=true
+		 * is incorrect.  There are _plenty_ of cases where CLIENTNAME
+		 * and SESSIONNAME are set outside of a Terminal Terver session.
+		 * It looks like Terminal Server sets SESSIONNAME to RDP-TCP#<number>
+		 * for "real" sessions.
+		 *
+		 * XXX - There's a better way to do this described at
+		 * http://www.microsoft.com/technet/archive/termsrv/maintain/featusability/tsrvapi.mspx?mfr=true
+		 */
+		if (g_ascii_strncasecmp(env, "rdp", 3) == 0)
+			res = g_strdup_printf("not tcp port 3389");
+	}
+
+	if (!res && (env = getenv("DISPLAY")) != NULL) {
 		/*
 		 * This mirrors what _X11TransConnectDisplay() does.
 		 * Note that, on some systems, the hostname can
@@ -215,7 +234,7 @@ const gchar *get_conn_cfilter(void) {
 		for (lastp = p; *p != '\0' && *p != ':' && *p != '/'; p++)
 			;
 		if (*p == '\0')
-			return "";	/* must have a colon */
+			return NULL;	/* must have a colon */
 
 		if (p != lastp && *p != ':') {	/* protocol given? */
 			/* Yes */
@@ -223,7 +242,7 @@ const gchar *get_conn_cfilter(void) {
 
 			/* Is it TCP? */
 			if (p - lastp != 3 || g_ascii_strncasecmp(lastp, "tcp", 3) != 0)
-				return "";	/* not TCP */
+				return NULL;	/* not TCP */
 			p++;			/* skip the '/' */
 		} else
 			p = env;		/* reset the pointer in
@@ -243,21 +262,19 @@ const gchar *get_conn_cfilter(void) {
 				lastc = p;
 
 		if (lastc == NULL)
-			return "";		/* must have a colon */
+			return NULL;		/* must have a colon */
 
 		if ((lastp != lastc) && (*(lastc - 1) == ':')
 		    && (((lastc - 1) == lastp) || (*(lastc - 2) != ':'))) {
 		    	/* DECnet display specified */
-		    	return "";
+		    	return NULL;
 		} else
 			hostlen = lastc - lastp;
 
 		if (hostlen == 0)
-			return "";	/* no hostname supplied */
+			return NULL;	/* no hostname supplied */
 
-		phostname = g_malloc(hostlen + 1);
-		memcpy(phostname, lastp, hostlen);
-		phostname[hostlen] = '\0';
+		phostname = g_strndup(lastp, hostlen);
 
 		if (pprotocol == NULL) {
 			/*
@@ -275,7 +292,7 @@ const gchar *get_conn_cfilter(void) {
 			if (g_ascii_strcasecmp(phostname, "localhost") == 0 ||
 			    strcmp(phostname, "127.0.0.1") == 0) {
 			    	g_free(phostname);
-				return "";
+				return NULL;
 			}
 
 			/*
@@ -284,7 +301,7 @@ const gchar *get_conn_cfilter(void) {
 			 */
 			if (strcmp(phostname, "unix") == 0) {
 			    	g_free(phostname);
-				return "";
+				return NULL;
 			}
 
 			/*
@@ -294,29 +311,13 @@ const gchar *get_conn_cfilter(void) {
 			 */
 			if (phostname[0] == '/') {
 				g_free(phostname);
-				return "";
+				return NULL;
 			}
 		}
 
-		g_string_printf(filter_str, "not %s host %s",
-			host_ip_af(phostname), phostname);
+		res = g_strdup_printf("not %s host %s",
+				host_ip_af(phostname), phostname);
 		g_free(phostname);
-		return filter_str->str;
-	} else if ((env = getenv("SESSIONNAME")) != NULL) {
-		/* Apparently the KB article at
-		 * http://technet2.microsoft.com/WindowsServer/en/library/6caf87bf-3d70-4801-9485-87e9ec3df0171033.mspx?mfr=true
-		 * is incorrect.  There are _plenty_ of cases where CLIENTNAME
-		 * and SESSIONNAME are set outside of a Terminal Terver session.
-		 * It looks like Terminal Server sets SESSIONNAME to RDP-TCP#<number>
-		 * for "real" sessions.
-		 *
-		 * XXX - There's a better way to do this described at
-		 * http://www.microsoft.com/technet/archive/termsrv/maintain/featusability/tsrvapi.mspx?mfr=true
-		 */
-		if (g_ascii_strncasecmp(env, "rdp", 3) == 0) {
-			g_string_printf(filter_str, "not tcp port 3389");
-			return filter_str->str;
-		}
 	}
-	return "";
+	return res;
 }
diff --git util.h util.h
index 2849b05..1dead08 100644
--- util.h
+++ util.h
@@ -50,7 +50,7 @@ void compute_timestamp_diff(gint *diffsec, gint *diffusec,
    DISPLAY (x11): [remote name]:<display num>
    CLIENTNAME (terminal server): <remote name>
  */
-const char *get_conn_cfilter(void);
+gchar *get_conn_cfilter(void);
 
 
 #ifdef __cplusplus