[PATCH] cleanup of global variables server/client_version_string in sshconnect.c

[Markus Schmidt]

In sshconnect.c there are two global variables for server_version_string 
client_version_string.

These are used just in a few functions and can easily be passed as 
parameters.

Also, there is a strange construct, where their memory is  allocated to 
the global pointers, then copies of these pointers are assigned to the 
kex structure. The kex_free finally frees them via cleanup of the kex 
structure while the global pointers remain.

This can easily be rearranged so that is clearer who owns which memory 
and who is thus responsible for freeing.

Also, the the ssh_login function leaks the memory allocated to the host 
variable.

Also, there is another global variable (previous_host_key) which could 
be removed by placing it into the ssh/active_state structure, but which, 
for now, could at least be made module-static.


Proposal: ssh-connect allocates the initial memory and creates values 
and passes these as parameters to the relevant functions.  It passes 
them also to ssh_kex2 which makes a true copy.  ssh-connect frees its 
memory and the kex system finally frees its copy.

Along the way, fix the host leak also and make previous_host_key static 
because both changes are trivial.



The patch is attached.  It's from the portable version but applies to 
the original version with a few lines offset.

-------------- next part --------------
diff --git a/sshconnect.c b/sshconnect.c
index 6d81927..7736bc2 100644
--- a/sshconnect.c
+++ b/sshconnect.c
@@ -69,9 +69,7 @@
 #include "ssherr.h"
 #include "authfd.h"
 
-char *client_version_string = NULL;
-char *server_version_string = NULL;
-struct sshkey *previous_host_key = NULL;
+static struct sshkey *previous_host_key = NULL;
 
 static int matching_host_key_dns = 0;
 
@@ -605,16 +603,16 @@ ssh_connect(struct ssh *ssh, const char *host, struct addrinfo *addrs,
 }
 
 static void
-send_client_banner(int connection_out, int minor1)
+send_client_banner(int connection_out, int minor1, char **client_version_stringp)
 {
 	/* Send our own protocol version identification. */
-	xasprintf(&client_version_string, "SSH-%d.%d-%.100s\r\n",
+	xasprintf(client_version_stringp, "SSH-%d.%d-%.100s\r\n",
 	    PROTOCOL_MAJOR_2, PROTOCOL_MINOR_2, SSH_VERSION);
-	if (atomicio(vwrite, connection_out, client_version_string,
-	    strlen(client_version_string)) != strlen(client_version_string))
+	if (atomicio(vwrite, connection_out, *client_version_stringp,
+	    strlen(*client_version_stringp)) != strlen(*client_version_stringp))
 		fatal("write: %.100s", strerror(errno));
-	chop(client_version_string);
-	debug("Local version string %.100s", client_version_string);
+	chop(*client_version_stringp);
+	debug("Local version string %.100s", *client_version_stringp);
 }
 
 /*
@@ -622,7 +620,7 @@ send_client_banner(int connection_out, int minor1)
  * identification string.
  */
 void
-ssh_exchange_identification(int timeout_ms)
+ssh_exchange_identification(int timeout_ms, char **client_version_stringp, char **server_version_stringp)
 {
 	char buf[256], remote_version[256];	/* must be same size! */
 	int remote_major, remote_minor, mismatch;
@@ -632,7 +630,7 @@ ssh_exchange_identification(int timeout_ms)
 	size_t len;
 	int rc;
 
-	send_client_banner(connection_out, 0);
+	send_client_banner(connection_out, 0, client_version_stringp);
 
 	/* Read other side's version identification. */
 	for (n = 0;;) {
@@ -673,13 +671,13 @@ ssh_exchange_identification(int timeout_ms)
 			break;
 		debug("ssh_exchange_identification: %s", buf);
 	}
-	server_version_string = xstrdup(buf);
+	*server_version_stringp = xstrdup(buf);
 
 	/*
 	 * Check that the versions match.  In future this might accept
 	 * several versions and set appropriate flags to handle them.
 	 */
-	if (sscanf(server_version_string, "SSH-%d.%d-%[^\n]\n",
+	if (sscanf(*server_version_stringp, "SSH-%d.%d-%[^\n]\n",
 	    &remote_major, &remote_minor, remote_version) != 3)
 		fatal("Bad remote protocol version identification: '%.100s'", buf);
 	debug("Remote protocol version %d.%d, remote software version %.100s",
@@ -705,7 +703,7 @@ ssh_exchange_identification(int timeout_ms)
 	if ((datafellows & SSH_BUG_RSASIGMD5) != 0)
 		logit("Server version \"%.100s\" uses unsafe RSA signature "
 		    "scheme; disabling use of RSA keys", remote_version);
-	chop(server_version_string);
+	chop(*server_version_stringp);
 }
 
 /* defaults to 'no' */
@@ -1406,6 +1404,8 @@ ssh_login(Sensitive *sensitive, const char *orighost,
 {
 	char *host;
 	char *server_user, *local_user;
+	char *client_version_string = NULL;
+	char *server_version_string = NULL;
 
 	local_user = xstrdup(pw->pw_name);
 	server_user = options.user ? options.user : local_user;
@@ -1415,7 +1415,7 @@ ssh_login(Sensitive *sensitive, const char *orighost,
 	lowercase(host);
 
 	/* Exchange protocol version identification strings with the server. */
-	ssh_exchange_identification(timeout_ms);
+	ssh_exchange_identification(timeout_ms, &client_version_string, &server_version_string);
 
 	/* Put the connection into non-blocking mode. */
 	packet_set_nonblocking();
@@ -1423,9 +1423,12 @@ ssh_login(Sensitive *sensitive, const char *orighost,
 	/* key exchange */
 	/* authenticate user */
 	debug("Authenticating to %s:%d as '%s'", host, port, server_user);
-	ssh_kex2(host, hostaddr, port);
+	ssh_kex2(host, hostaddr, port, client_version_string, server_version_string);
 	ssh_userauth2(local_user, server_user, host, sensitive);
+	free(client_version_string);
+	free(server_version_string);
 	free(local_user);
+	free(host);
 }
 
 void
diff --git a/sshconnect.h b/sshconnect.h
index 890d857..d6de9fe 100644
--- a/sshconnect.h
+++ b/sshconnect.h
@@ -40,7 +40,7 @@ void	 ssh_kill_proxy_command(void);
 void	 ssh_login(Sensitive *, const char *, struct sockaddr *, u_short,
     struct passwd *, int);
 
-void	 ssh_exchange_identification(int);
+void	 ssh_exchange_identification(int, char **, char **);
 
 int	 verify_host_key(char *, struct sockaddr *, struct sshkey *);
 
@@ -48,7 +48,7 @@ void	 get_hostfile_hostname_ipaddr(char *, struct sockaddr *, u_short,
     char **, char **);
 
 void	 ssh_kex(char *, struct sockaddr *);
-void	 ssh_kex2(char *, struct sockaddr *, u_short);
+void	 ssh_kex2(char *, struct sockaddr *, u_short, const char *, const char *);
 
 void	 ssh_userauth1(const char *, const char *, char *, Sensitive *);
 void	 ssh_userauth2(const char *, const char *, char *, Sensitive *);
diff --git a/sshconnect2.c b/sshconnect2.c
index cc527e1..8a9b6c0 100644
--- a/sshconnect2.c
+++ b/sshconnect2.c
@@ -78,8 +78,6 @@
 #endif
 
 /* import */
-extern char *client_version_string;
-extern char *server_version_string;
 extern Options options;
 
 /*
@@ -155,7 +153,7 @@ order_hostkeyalgs(char *host, struct sockaddr *hostaddr, u_short port)
 }
 
 void
-ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port)
+ssh_kex2(char *host, struct sockaddr *hostaddr, u_short port, const char *client_version_string, const char *server_version_string)
 {
 	char *myproposal[PROPOSAL_MAX] = { KEX_CLIENT };
 	char *s, *all_key;