[Asterisk-code-review] CLI: Address multiple issues. (asterisk[13])

Joshua Colp asteriskteam at digium.com
Wed Dec 20 08:34:37 CST 2017


Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/7663 )

Change subject: CLI: Address multiple issues.
......................................................................

CLI: Address multiple issues.

* listen uses the variable `s` for the result from ast_poll() then
  overwrites it with the result of accept().  Create a separate variable
  poll_result to avoid confusion since ast_poll does not return a file
  descriptor.
* Resolve fd leak that would occur if setsockopt failed in listen.
* Reserve an extra byte while processing completion results from remote
  daemon.  This fixes a bug where completion processing used strstr() on
  a string that was not '\0' terminated.  This was no risk to the Asterisk
  daemon, the bug was only reachable the remote console process.
* Resolve leak in handle_showchan when the channel is not found.
* Multiple leaks and a deadlock in pbx_config CLI completion.
* Fix leaks in "manager show command".

Change-Id: I8f633ceb1714867ae30ef4e421858f77c14485a9
---
M main/asterisk.c
M main/cli.c
M main/manager.c
M pbx/pbx_config.c
4 files changed, 59 insertions(+), 29 deletions(-)

Approvals:
  Richard Mudgett: Looks good to me, but someone else must approve
  Joshua Colp: Looks good to me, approved; Approved for Submit



diff --git a/main/asterisk.c b/main/asterisk.c
index 015c9f3..f8e31d5 100644
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -1695,17 +1695,21 @@
 	int s;
 	socklen_t len;
 	int x;
+	int poll_result;
 	struct pollfd fds[1];
+
 	for (;;) {
-		if (ast_socket < 0)
+		if (ast_socket < 0) {
 			return NULL;
+		}
 		fds[0].fd = ast_socket;
 		fds[0].events = POLLIN;
-		s = ast_poll(fds, 1, -1);
+		poll_result = ast_poll(fds, 1, -1);
 		pthread_testcancel();
-		if (s < 0) {
-			if (errno != EINTR)
+		if (poll_result < 0) {
+			if (errno != EINTR) {
 				ast_log(LOG_WARNING, "poll returned error: %s\n", strerror(errno));
+			}
 			continue;
 		}
 		len = sizeof(sunaddr);
@@ -1719,6 +1723,7 @@
 			/* turn on socket credentials passing. */
 			if (setsockopt(s, SOL_SOCKET, SO_PASSCRED, &sckopt, sizeof(sckopt)) < 0) {
 				ast_log(LOG_WARNING, "Unable to turn on socket credentials passing\n");
+				close(s);
 			} else
 #endif
 			{
@@ -3195,20 +3200,10 @@
 #define CMD_MATCHESARRAY "_COMMAND MATCHESARRAY \"%s\" \"%s\""
 		char *mbuf;
 		char *new_mbuf;
-		int mlen = 0, maxmbuf = 2048;
+		int mlen = 0;
+		int maxmbuf = ast_asprintf(&mbuf, CMD_MATCHESARRAY, lf->buffer, ptr);
 
-		/* Start with a 2048 byte buffer */
-		mbuf = ast_malloc(maxmbuf);
-
-		/* This will run snprintf twice at most. */
-		while (mbuf && (mlen = snprintf(mbuf, maxmbuf, CMD_MATCHESARRAY, lf->buffer, ptr)) > maxmbuf) {
-			/* Return value does not include space for NULL terminator. */
-			maxmbuf = mlen + 1;
-			ast_free(mbuf);
-			mbuf = ast_malloc(maxmbuf);
-		}
-
-		if (!mbuf) {
+		if (maxmbuf == -1) {
 			*((char *) lf->cursor) = savechr;
 
 			return (char *)(CC_ERROR);
@@ -3221,9 +3216,9 @@
 
 		while (!strstr(mbuf, AST_CLI_COMPLETE_EOF) && res != -1) {
 			if (mlen + 1024 > maxmbuf) {
-				/* Expand buffer to the next 1024 byte increment. */
+				/* Expand buffer to the next 1024 byte increment plus a NULL terminator. */
 				maxmbuf = mlen + 1024;
-				new_mbuf = ast_realloc(mbuf, maxmbuf);
+				new_mbuf = ast_realloc(mbuf, maxmbuf + 1);
 				if (!new_mbuf) {
 					ast_free(mbuf);
 					*((char *) lf->cursor) = savechr;
@@ -3236,6 +3231,7 @@
 			res = read(ast_consock, mbuf + mlen, 1024);
 			if (res > 0) {
 				mlen += res;
+				mbuf[mlen] = '\0';
 			}
 		}
 		mbuf[mlen] = '\0';
diff --git a/main/cli.c b/main/cli.c
index 7039b72..fe20c34 100644
--- a/main/cli.c
+++ b/main/cli.c
@@ -1522,17 +1522,20 @@
 		return CLI_FAILURE;
 	}
 
-	output = ast_str_create(8192);
-	if (!output) {
-		return CLI_FAILURE;
-	}
-
 	chan = ast_channel_get_by_name(a->argv[3]);
 	if (!chan) {
 		ast_cli(a->fd, "%s is not a known channel\n", a->argv[3]);
+
 		return CLI_SUCCESS;
 	}
 
+	output = ast_str_create(8192);
+	if (!output) {
+		ast_channel_unref(chan);
+
+		return CLI_FAILURE;
+	}
+
 	now = ast_tvnow();
 	ast_channel_lock(chan);
 
diff --git a/main/manager.c b/main/manager.c
index 8105265..890a975 100644
--- a/main/manager.c
+++ b/main/manager.c
@@ -2344,10 +2344,11 @@
 		AST_RWLIST_UNLOCK(&actions);
 		return ret;
 	}
-	authority = ast_str_alloca(MAX_AUTH_PERM_STRING);
 	if (a->argc < 4) {
 		return CLI_SHOWUSAGE;
 	}
+
+	authority = ast_str_alloca(MAX_AUTH_PERM_STRING);
 
 #ifdef AST_XML_DOCS
 	/* setup the titles */
@@ -2376,6 +2377,22 @@
 					char *seealso = ast_xmldoc_printable(S_OR(cur->seealso, "Not available"), 1);
 					char *privilege = ast_xmldoc_printable(S_OR(auth_str, "Not available"), 1);
 					char *responses = ast_xmldoc_printable("None", 1);
+
+					if (!syntax || !synopsis || !description || !arguments
+							|| !seealso || !privilege || !responses) {
+						ast_free(syntax);
+						ast_free(synopsis);
+						ast_free(description);
+						ast_free(arguments);
+						ast_free(seealso);
+						ast_free(privilege);
+						ast_free(responses);
+						ast_cli(a->fd, "Allocation failure.\n");
+						AST_RWLIST_UNLOCK(&actions);
+
+						return CLI_FAILURE;
+					}
+
 					ast_cli(a->fd, "%s%s\n\n%s%s\n\n%s%s\n\n%s%s\n\n%s%s\n\n%s%s\n\n%s",
 						syntax_title, syntax,
 						synopsis_title, synopsis,
@@ -2403,6 +2420,14 @@
 						ast_cli(a->fd, "Event: %s\n", cur->final_response->name);
 						print_event_instance(a, cur->final_response);
 					}
+
+					ast_free(syntax);
+					ast_free(synopsis);
+					ast_free(description);
+					ast_free(arguments);
+					ast_free(seealso);
+					ast_free(privilege);
+					ast_free(responses);
 				} else
 #endif
 				{
diff --git a/pbx/pbx_config.c b/pbx/pbx_config.c
index c4a0e6c..1a1c73c 100644
--- a/pbx/pbx_config.c
+++ b/pbx/pbx_config.c
@@ -303,8 +303,10 @@
 				while ( (nc = ast_walk_contexts(nc)) && nc != c && !already_served)
 					already_served = lookup_ci(nc, i_name);
 
-				if (!already_served && ++which > a->n)
+				if (!already_served && ++which > a->n) {
 					res = strdup(i_name);
+					break;
+				}
 			}
 			ast_unlock_context(c);
 		}
@@ -1523,17 +1525,21 @@
 		}
 
 		for (c = NULL; !ret && (c = ast_walk_contexts(c)); ) {
-			if (ast_rdlock_context(c))	/* fail, skip it */
+			if (ast_rdlock_context(c)) {
+				/* fail, skip it */
 				continue;
-			if (!partial_match(ast_get_context_name(c), a->word, len))
+			}
+			if (!partial_match(ast_get_context_name(c), a->word, len)) {
+				ast_unlock_context(c);
 				continue;
+			}
 			if (lookup_c_ip(c, ignorepat) && ++which > a->n)
 				ret = strdup(ast_get_context_name(c));
 			ast_unlock_context(c);
 		}
 		ast_unlock_contexts();
 		free(dupline);
-		return NULL;
+		return ret;
 	}
 
 	return NULL;

-- 
To view, visit https://gerrit.asterisk.org/7663
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: merged
Gerrit-Change-Id: I8f633ceb1714867ae30ef4e421858f77c14485a9
Gerrit-Change-Number: 7663
Gerrit-PatchSet: 4
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171220/9c34603f/attachment-0001.html>


More information about the asterisk-code-review mailing list