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.

* translate.c: Note in the appropriate locations where powerof cannot return
a negative value, due to proper checks placed on the inputs to that function.

(issue ASTERISK-19655)
Reported by: Matt Jordan

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

git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.8@362359 65c4cc65-6c06-0410-ace0-fbb531ad65f3

diff --git a/main/asterisk.c b/main/asterisk.c
index f5f176bc4d51f9a56b72aa536ec53ee416bd368a..d95ae2dfedfbd2b67f7aad5b4efb5d8e0eb53023 100644 (file)
--- a/main/asterisk.c
+++ b/main/asterisk.c
@@ -2256,6 +2256,7 @@ static int ast_el_read_char(EditLine *editline, char *cp)
                                                quit_handler(0, SHUTDOWN_FAST, 0);
                                        }
                                }
+                               continue;
                        }
 
                        buf[res] = '\0';
@@ -2548,7 +2549,9 @@ static char *cli_complete(EditLine *editline, int ch)
        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);
 

diff --git a/main/manager.c b/main/manager.c
index 50c3a1136bca6a3c50932f697db135e088211528..6d491aac609b47895b6452a785fb0e2b278f14be 100644 (file)
--- a/main/manager.c
+++ b/main/manager.c
@@ -3593,7 +3593,7 @@ static int action_command(struct mansession *s, const struct message *m)
 {
        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;
@@ -3608,7 +3608,11 @@ static int action_command(struct mansession *s, const struct message *m)
                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)) {
@@ -3616,30 +3620,45 @@ static int action_command(struct mansession *s, const struct message *m)
        }
        /* 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;
 }
 
@@ -5710,7 +5729,7 @@ static void process_output(struct mansession *s, struct ast_str **out, struct as
        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 {

diff --git a/main/translate.c b/main/translate.c
index e11427d20d2591d1af3d1efa3d26e0682c325027..64caba0c423b443bbeac003a4014d0d574450af2 100644 (file)
--- a/main/translate.c
+++ b/main/translate.c
@@ -718,6 +718,11 @@ static char *handle_cli_core_show_translation(struct ast_cli_entry *e, int cmd,
                        if (!(format_list[i].bits & AST_FORMAT_AUDIO_MASK) || (format_list[i].bits == input_src)) {
                                continue;
                        }
+                       /* Note that dst can never be -1, as an element of format_list will have
+                        * at least one bit set.  src cannot be -1 as well, as it is previously
+                        * sanitized - hence it is safe to directly index tr_matrix with the results
+                        * of powerof.
+                        */
                        dst = powerof(format_list[i].bits);
                        src = powerof(input_src);
                        ast_str_reset(str);
@@ -1068,6 +1073,7 @@ format_t ast_translate_available_formats(format_t dest, format_t src)
        format_t x;
        format_t src_audio = src & AST_FORMAT_AUDIO_MASK;
        format_t src_video = src & AST_FORMAT_VIDEO_MASK;
+       format_t x_bits;
 
        /* if we don't have a source format, we just have to try all
           possible destination formats */
@@ -1075,12 +1081,19 @@ format_t ast_translate_available_formats(format_t dest, format_t src)
                return dest;
 
        /* If we have a source audio format, get its format index */
-       if (src_audio)
+       if (src_audio) {
                src_audio = powerof(src_audio);
+       }
 
        /* If we have a source video format, get its format index */
-       if (src_video)
+       if (src_video) {
                src_video = powerof(src_video);
+       }
+
+       /* Note that src_audio and src_video are guaranteed to not be
+        * negative at this point, as we ensured they were non-zero.  It is
+        * safe to use the return value of powerof as an index into tr_matrix.
+        */
 
        AST_RWLIST_RDLOCK(&translators);
 
@@ -1103,14 +1116,16 @@ format_t ast_translate_available_formats(format_t dest, format_t src)
                        continue;
 
                /* if we don't have a translation path from the src
-                  to this format, remove it from the result */
-               if (!tr_matrix[src_audio][powerof(x)].step) {
+                  to this format, remove it from the result.  Note that x_bits
+                  cannot be less than 0 as x will always have one bit set to 1 */
+               x_bits = powerof(x);
+               if (!tr_matrix[src_audio][x_bits].step) {
                        res &= ~x;
                        continue;
                }
 
                /* now check the opposite direction */
-               if (!tr_matrix[powerof(x)][src_audio].step)
+               if (!tr_matrix[x_bits][src_audio].step)
                        res &= ~x;
        }
 
@@ -1133,14 +1148,16 @@ format_t ast_translate_available_formats(format_t dest, format_t src)
                        continue;
 
                /* if we don't have a translation path from the src
-                  to this format, remove it from the result */
-               if (!tr_matrix[src_video][powerof(x)].step) {
+                  to this format, remove it from the result.  Note that x_bits
+                  cannot be less than 0 as x will always have one bit set to 1 */
+               x_bits = powerof(x);
+               if (!tr_matrix[src_video][x_bits].step) {
                        res &= ~x;
                        continue;
                }
 
                /* now check the opposite direction */
-               if (!tr_matrix[powerof(x)][src_video].step)
+               if (!tr_matrix[x_bits][src_video].step)
                        res &= ~x;
        }