From 7024d6ce061d57af65fe3a068803212581552f96 Mon Sep 17 00:00:00 2001

From: "Alan T. DeKok" <aland@freeradius.org>

Date: Fri, 10 Mar 2017 09:11:03 -0500

Subject: [PATCH] Fix some issues found with static analyzers

Fix some issues found with static analyzers. Includes the following.

Coverity.  Closes #1937

(cherry picked from commit 521e2a9bd3b1b49555bcd9fb90b03c456f616070)

Allo session resumption for RadSec connectins.  Closes #1936

(cherry picked from commit 43efa4321d7cd9fca1184f999e1cadeff3afda02)

request->packet cannot be NULL. Helps with #1935

(cherry picked from commit 7f22c30476be495438d5bc4dbec2f618f09c0b15)

remove unused variable

(cherry picked from commit d9bfc70efbf575258425d2ca86160490e0c36a45)

close open FDs on error, and use error path in more situations

(cherry picked from commit e51af914bc5fdf879f821e6a1ecfe700bff937ca)

return RLM_MODULE_FAIL for default switch statement

(cherry picked from commit cdfa6e15065a4a616c96af516936117124a1c293)

Remove always-false condition in rlm_eap_fast

(cherry picked from commit 96d7a5e2bb393b4fd1b6cb6e0a6858e6c18eb96a)

Remove always-false condition from cf_item_parse

(cherry picked from commit 92624adf8170fb133b330fe02d8940a8bac86189)

Ensure that error is always initialized

(cherry picked from commit c483d8456e44747621334b318483c3a33752aaac)

---

 src/main/command.c                                | 15 ++++++++-------

 src/main/conffile.c                               |  2 --

 src/main/process.c                                |  5 +++--

 src/main/tls.c                                    | 12 ++++++------

 src/main/xlat.c                                   |  6 +++++-

 src/modules/rlm_cache/rlm_cache.c                 |  3 ++-

 src/modules/rlm_eap/types/rlm_eap_fast/eap_fast.c |  3 ---

 src/modules/rlm_mschap/rlm_mschap.c               |  2 +-

 8 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/src/main/command.c b/src/main/command.c

index d3b729f9a..34c5268d7 100644

--- a/src/main/command.c

+++ b/src/main/command.c

@@ -345,7 +345,7 @@ static int fr_server_domain_socket_perm(UNUSED char const *path, UNUSED uid_t ui

  */

 static int fr_server_domain_socket_perm(char const *path, uid_t uid, gid_t gid)

 {

-	int			dir_fd = -1, path_fd = -1, sock_fd = -1, parent_fd = -1;

+	int			dir_fd = -1, sock_fd = -1, parent_fd = -1;

 	char const		*name;

 	char			*buff = NULL, *dir = NULL, *p;

@@ -392,8 +392,9 @@ static int fr_server_domain_socket_perm(char const *path, uid_t uid, gid_t gid)

 		fr_strerror_printf("Failed determining parent directory");

 	error:

 		talloc_free(dir);

-		close(dir_fd);

-		close(path_fd);

+		if (sock_fd >= 0) close(sock_fd);

+		if (dir_fd >= 0) close(dir_fd);

+		if (parent_fd >= 0) close(parent_fd);

 		return -1;

 	}

@@ -459,7 +460,7 @@ static int fr_server_domain_socket_perm(char const *path, uid_t uid, gid_t gid)

 		if (ret < 0) {

 			fr_strerror_printf("Failed changing ownership of control socket directory: %s",

 					   fr_syserror(errno));

-			return -1;

+			goto error;

 		}

 	/*

 	 *	Control socket dir already exists, but we still need to

@@ -527,7 +528,7 @@ static int fr_server_domain_socket_perm(char const *path, uid_t uid, gid_t gid)

 		if (client_fd >= 0) {

 			fr_strerror_printf("Control socket '%s' is already in use", path);

 			close(client_fd);

-			return -1;

+			goto error;

 		}

 	}

@@ -676,8 +677,8 @@ static int fr_server_domain_socket_perm(char const *path, uid_t uid, gid_t gid)

 	if (uid != (uid_t)-1) rad_seuid(euid);

 	if (gid != (gid_t)-1) rad_segid(egid);

-	close(dir_fd);

-	close(path_fd);

+	if (dir_fd >= 0) close(dir_fd);

+	if (parent_fd >= 0) close(parent_fd);

 	return sock_fd;

 }

diff --git a/src/main/conffile.c b/src/main/conffile.c

index df78184bd..10c029a0e 100644

--- a/src/main/conffile.c

+++ b/src/main/conffile.c

@@ -1474,7 +1474,6 @@ int cf_item_parse(CONF_SECTION *cs, char const *name, unsigned int type, void *d

 	if (!value) {

 		if (required) {

-		is_required:

 			cf_log_err(c_item, "Configuration item \"%s\" must have a value", name);

 			return -1;

@@ -1620,7 +1619,6 @@ int cf_item_parse(CONF_SECTION *cs, char const *name, unsigned int type, void *d

 			}

 		}

-		if (required && !value) goto is_required;

 		if (cant_be_empty && (value[0] == '\0')) goto cant_be_empty;

 		if (attribute) {

diff --git a/src/main/process.c b/src/main/process.c

index c5a690672..c3856c7a1 100644

--- a/src/main/process.c

+++ b/src/main/process.c

@@ -2122,8 +2122,9 @@ static void remove_from_proxy_hash_nl(REQUEST *request, bool yank)

 	}

 #ifdef WITH_TCP

-	rad_assert(request->proxy_listener != NULL);

-	request->proxy_listener->count--;

+	if (request->proxy_listener) {

+		request->proxy_listener->count--;

+	}

 #endif

 	request->proxy_listener = NULL;

diff --git a/src/main/tls.c b/src/main/tls.c

index caa7e62ed..a72be2b63 100644

--- a/src/main/tls.c

+++ b/src/main/tls.c

@@ -1360,7 +1360,7 @@ static int cbtls_new_session(SSL *ssl, SSL_SESSION *sess)

 		blob_len = i2d_SSL_SESSION(sess, NULL);

 		if (blob_len < 1) {

 			/* something went wrong */

-			RWDEBUG("Session serialisation failed, couldn't determine required buffer length");

+			if (request) RWDEBUG("Session serialisation failed, couldn't determine required buffer length");

 			return 0;

 		}

@@ -1375,7 +1375,7 @@ static int cbtls_new_session(SSL *ssl, SSL_SESSION *sess)

 		p = sess_blob;

 		rv = i2d_SSL_SESSION(sess, &p);

 		if (rv != blob_len) {

-			RWDEBUG("Session serialisation failed");

+			if (request) RWDEBUG("Session serialisation failed");

 			goto error;

 		}

@@ -1384,8 +1384,8 @@ static int cbtls_new_session(SSL *ssl, SSL_SESSION *sess)

 			 conf->session_cache_path, FR_DIR_SEP, buffer);

 		fd = open(filename, O_RDWR|O_CREAT|O_EXCL, 0600);

 		if (fd < 0) {

-			RERROR("Session serialisation failed, failed opening session file %s: %s",

-			      filename, fr_syserror(errno));

+			if (request) RERROR("Session serialisation failed, failed opening session file %s: %s",

+					    filename, fr_syserror(errno));

 			goto error;

 		}

@@ -1409,7 +1409,7 @@ static int cbtls_new_session(SSL *ssl, SSL_SESSION *sess)

 		while (todo > 0) {

 			rv = write(fd, p, todo);

 			if (rv < 1) {

-				RWDEBUG("Failed writing session: %s", fr_syserror(errno));

+				if (request) RWDEBUG("Failed writing session: %s", fr_syserror(errno));

 				close(fd);

 				goto error;

 			}

@@ -1417,7 +1417,7 @@ static int cbtls_new_session(SSL *ssl, SSL_SESSION *sess)

 			todo -= rv;

 		}

 		close(fd);

-		RWDEBUG("Wrote session %s to %s (%d bytes)", buffer, filename, blob_len);

+		if (request) RWDEBUG("Wrote session %s to %s (%d bytes)", buffer, filename, blob_len);

 	}

 error:

diff --git a/src/main/xlat.c b/src/main/xlat.c

index 31987289c..aeac3a4c3 100644

--- a/src/main/xlat.c

+++ b/src/main/xlat.c

@@ -1787,7 +1787,10 @@ static ssize_t xlat_tokenize_request(REQUEST *request, char const *fmt, xlat_exp

 	 *	much faster.

 	 */

 	tokens = talloc_typed_strdup(request, fmt);

-	if (!tokens) return -1;

+	if (!tokens) {

+		error = "Out of memory";

+		return -1;

+	}

 	slen = xlat_tokenize_literal(request, tokens, head, false, &error);

@@ -1806,6 +1809,7 @@ static ssize_t xlat_tokenize_request(REQUEST *request, char const *fmt, xlat_exp

 	 */

 	if (slen < 0) {

 		talloc_free(tokens);

+

 		rad_assert(error != NULL);

 		REMARKER(fmt, -slen, error);

diff --git a/src/modules/rlm_cache/rlm_cache.c b/src/modules/rlm_cache/rlm_cache.c

index 248de8bf9..54449747f 100644

--- a/src/modules/rlm_cache/rlm_cache.c

+++ b/src/modules/rlm_cache/rlm_cache.c

@@ -126,7 +126,8 @@ static void CC_HINT(nonnull) cache_merge(rlm_cache_t *inst, REQUEST *request, rl

 	RDEBUG2("Merging cache entry into request");

-	if (c->packet && request->packet) {

+	if (c->packet) {

+		rad_assert(request->packet != NULL);

 		rdebug_pair_list(L_DBG_LVL_2, request, c->packet, "&request:");

 		radius_pairmove(request, &request->packet->vps, fr_pair_list_copy(request->packet, c->packet), false);

 	}

diff --git a/src/modules/rlm_eap/types/rlm_eap_fast/eap_fast.c b/src/modules/rlm_eap/types/rlm_eap_fast/eap_fast.c

index dba2c1462..95e521718 100644

--- a/src/modules/rlm_eap/types/rlm_eap_fast/eap_fast.c

+++ b/src/modules/rlm_eap/types/rlm_eap_fast/eap_fast.c

@@ -1235,9 +1235,6 @@ PW_CODE eap_fast_process(eap_handler_t *eap_session, tls_session_t *tls_session)

 		eap_fast_append_result(tls_session, code);

-		if (code == PW_CODE_ACCESS_REJECT)

-			break;

-

 		if (t->pac.send) {

 			RDEBUG("Peer requires new PAC");

 			eap_fast_send_pac_tunnel(request, tls_session);

diff --git a/src/modules/rlm_mschap/rlm_mschap.c b/src/modules/rlm_mschap/rlm_mschap.c

index aba15f826..c702f1b45 100644

--- a/src/modules/rlm_mschap/rlm_mschap.c

+++ b/src/modules/rlm_mschap/rlm_mschap.c

@@ -1471,7 +1471,7 @@ static rlm_rcode_t mschap_error(rlm_mschap_t *inst, REQUEST *request, unsigned c

 		break;

 	default:

-		rad_assert(0);

+		return RLM_MODULE_FAIL;

 	}

 	mschap_add_reply(request, ident, "MS-CHAP-Error", buffer, strlen(buffer));

-- 

2.11.0