[asterisk-commits] mjordan: trunk r362361 - in /trunk: ./ main/asterisk.c main/manager.c

[SVN commits to the Asterisk project]

Author: mjordan
Date: Tue Apr 17 16:08:05 2012
New Revision: 362361

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=362361
Log:
Fix places in main where a negative return value could impact execution

This patch addresses a number of modules in main that did not handle the
negative return value from function calls adequately, or were not sufficiently
clear that the conditions leading to improper handling of the return values
could not occur.  This includes:

* asterisk.c: A negative return value from the read function would be used
directly as an index into a buffer.  We now check for success of the read
function prior to using its result as an index.

* manager.c: Check for failures in mkstemp and lseek when handling the
temporary file created for processing data returned from a CLI command in
action_command.  Also check that the result of an lseek is sanitized prior
to using it as the size of a memory map to allocate.

(issue ASTERISK-19655)
Reported by: Matt Jordan

Review: https://reviewboard.asterisk.org/r/1863/
........

Merged revisions 362359 from http://svn.asterisk.org/svn/asterisk/branches/1.8
........

Merged revisions 362360 from http://svn.asterisk.org/svn/asterisk/branches/10

Modified:
    trunk/   (props changed)
    trunk/main/asterisk.c
    trunk/main/manager.c

Propchange: trunk/
------------------------------------------------------------------------------
Binary property 'branch-10-merged' - no diff available.

Modified: trunk/main/asterisk.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/asterisk.c?view=diff&rev=362361&r1=362360&r2=362361
==============================================================================
--- trunk/main/asterisk.c (original)
+++ trunk/main/asterisk.c Tue Apr 17 16:08:05 2012
@@ -2311,6 +2311,7 @@
 						quit_handler(0, SHUTDOWN_FAST, 0);
 					}
 				}
+				continue;
 			}
 
 			buf[res] = '\0';
@@ -2621,7 +2622,9 @@
 	if (ast_opt_remote) {
 		snprintf(buf, sizeof(buf), "_COMMAND NUMMATCHES \"%s\" \"%s\"", lf->buffer, ptr);
 		fdsend(ast_consock, buf);
-		res = read(ast_consock, buf, sizeof(buf) - 1);
+		if ((res = read(ast_consock, buf, sizeof(buf) - 1)) < 0) {
+			return (char*)(CC_ERROR);
+		}
 		buf[res] = '\0';
 		nummatches = atoi(buf);
 

Modified: trunk/main/manager.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/manager.c?view=diff&rev=362361&r1=362360&r2=362361
==============================================================================
--- trunk/main/manager.c (original)
+++ trunk/main/manager.c Tue Apr 17 16:08:05 2012
@@ -3737,7 +3737,7 @@
 {
 	const char *cmd = astman_get_header(m, "Command");
 	const char *id = astman_get_header(m, "ActionID");
-	char *buf, *final_buf;
+	char *buf = NULL, *final_buf = NULL;
 	char template[] = "/tmp/ast-ami-XXXXXX";	/* template for temporary file */
 	int fd;
 	off_t l;
@@ -3752,7 +3752,11 @@
 		return 0;
 	}
 
-	fd = mkstemp(template);
+	if ((fd = mkstemp(template)) < 0) {
+		ast_log(AST_LOG_WARNING, "Failed to create temporary file for command: %s\n", strerror(errno));
+		astman_send_error(s, m, "Command response construction error");
+		return 0;
+	}
 
 	astman_append(s, "Response: Follows\r\nPrivilege: Command\r\n");
 	if (!ast_strlen_zero(id)) {
@@ -3760,30 +3764,45 @@
 	}
 	/* FIXME: Wedge a ActionID response in here, waiting for later changes */
 	ast_cli_command(fd, cmd);	/* XXX need to change this to use a FILE * */
-	l = lseek(fd, 0, SEEK_END);	/* how many chars available */
+	/* Determine number of characters available */
+	if ((l = lseek(fd, 0, SEEK_END)) < 0) {
+		ast_log(LOG_WARNING, "Failed to determine number of characters for command: %s\n", strerror(errno));
+		goto action_command_cleanup;
+	}
 
 	/* This has a potential to overflow the stack.  Hence, use the heap. */
-	buf = ast_calloc(1, l + 1);
-	final_buf = ast_calloc(1, l + 1);
-	if (buf) {
-		lseek(fd, 0, SEEK_SET);
-		if (read(fd, buf, l) < 0) {
-			ast_log(LOG_WARNING, "read() failed: %s\n", strerror(errno));
-		}
-		buf[l] = '\0';
-		if (final_buf) {
-			term_strip(final_buf, buf, l);
-			final_buf[l] = '\0';
-		}
-		astman_append(s, "%s", S_OR(final_buf, buf));
-		ast_free(buf);
-	}
+	buf = ast_malloc(l + 1);
+	final_buf = ast_malloc(l + 1);
+
+	if (!buf || !final_buf) {
+		ast_log(LOG_WARNING, "Failed to allocate memory for temporary buffer\n");
+		goto action_command_cleanup;
+	}
+
+	if (lseek(fd, 0, SEEK_SET) < 0) {
+		ast_log(LOG_WARNING, "Failed to set position on temporary file for command: %s\n", strerror(errno));
+		goto action_command_cleanup;
+	}
+
+	if (read(fd, buf, l) < 0) {
+		ast_log(LOG_WARNING, "read() failed: %s\n", strerror(errno));
+		goto action_command_cleanup;
+	}
+
+	buf[l] = '\0';
+	term_strip(final_buf, buf, l);
+	final_buf[l] = '\0';
+	astman_append(s, "%s", final_buf);
+
+action_command_cleanup:
+
 	close(fd);
 	unlink(template);
 	astman_append(s, "--END COMMAND--\r\n\r\n");
-	if (final_buf) {
-		ast_free(final_buf);
-	}
+
+	ast_free(buf);
+	ast_free(final_buf);
+
 	return 0;
 }
 
@@ -5951,7 +5970,7 @@
 	fprintf(s->f, "%c", 0);
 	fflush(s->f);
 
-	if ((l = ftell(s->f))) {
+	if ((l = ftell(s->f)) > 0) {
 		if (MAP_FAILED == (buf = mmap(NULL, l, PROT_READ | PROT_WRITE, MAP_PRIVATE, s->fd, 0))) {
 			ast_log(LOG_WARNING, "mmap failed.  Manager output was not processed\n");
 		} else {