<p>Jenkins2 <strong>merged</strong> this change.</p><p><a href="https://gerrit.asterisk.org/6315">View Change</a></p><div style="white-space:pre-wrap">Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Jenkins2: Approved for Submit

</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">voicemail: Fix various abuses of mkstemp<br><br>mkstemp() returns a unique filename, but appending an extension to that<br>filename does not guarantee uniqueness. Instead, use mkdtemp() and we<br>can put whatever extension we want on the files that we create inside<br>the directory.<br><br>In the case of app_minivm, we also now properly clean up any temporary<br>files that we create.<br><br>ASTERISK-20858 #close<br>Reported by: Walter Doekes<br><br>Change-Id: I30ad04f0e115f0b11693ff678ba5184d8b938e43<br>---<br>M apps/app_minivm.c<br>M apps/app_voicemail.c<br>2 files changed, 136 insertions(+), 77 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">diff --git a/apps/app_minivm.c b/apps/app_minivm.c<br>index 9359f82..15449ad 100644<br>--- a/apps/app_minivm.c<br>+++ b/apps/app_minivm.c<br>@@ -1232,6 +1232,8 @@<br>  * \brief Send voicemail with audio file as an attachment */<br> static int sendmail(struct minivm_template *template, struct minivm_account *vmu, char *cidnum, char *cidname, const char *filename, char *format, int duration, int attach_user_voicemail, enum mvm_messagetype type, const char *counter)<br> {<br>+    RAII_VAR(struct ast_str *, str1, ast_str_create(16), ast_free);<br>+      RAII_VAR(struct ast_str *, str2, ast_str_create(16), ast_free);<br>       FILE *p = NULL;<br>       int pfd;<br>      char email[256] = "";<br>@@ -1241,20 +1243,18 @@<br>      char fname[PATH_MAX];<br>         char dur[PATH_MAX];<br>   char tmp[80] = "/tmp/astmail-XXXXXX";<br>-      char tmp2[PATH_MAX];<br>- char newtmp[PATH_MAX]; /* Only used with volgain */<br>+  char mail_cmd_buffer[PATH_MAX];<br>+      char sox_gain_tmpdir[PATH_MAX] = ""; /* Only used with volgain */<br>+  char *file_to_delete = NULL, *dir_to_delete = NULL;<br>   struct timeval now;<br>   struct ast_tm tm;<br>     struct minivm_zone *the_zone = NULL;<br>- struct ast_channel *ast;<br>-     char *finalfilename = "";<br>-  struct ast_str *str1 = ast_str_create(16), *str2 = ast_str_create(16);<br>+       struct ast_channel *chan = NULL;<br>      char *fromaddress;<br>    char *fromemail;<br>+     int res;<br> <br>   if (!str1 || !str2) {<br>-                ast_free(str1);<br>-              ast_free(str2);<br>               return -1;<br>    }<br> <br>@@ -1269,9 +1269,7 @@<br> <br>        if (ast_strlen_zero(email)) {<br>                 ast_log(LOG_WARNING, "No address to send message to.\n");<br>-          ast_free(str1);<br>-              ast_free(str2);<br>-              return -1;      <br>+             return -1;<br>    }<br> <br>  ast_debug(3, "Sending mail to %s@%s - Using template %s\n", vmu->username, vmu->domain, template->name);<br>@@ -1279,35 +1277,30 @@<br>  if (!strcmp(format, "wav49"))<br>               format = "WAV";<br> <br>-<br>       /* If we have a gain option, process it now with sox */<br>       if (type == MVM_MESSAGE_EMAIL && (vmu->volgain < -.001 || vmu->volgain > .001) ) {<br>-               char tmpcmd[PATH_MAX];<br>-               int tmpfd;<br>+           char sox_gain_cmd[PATH_MAX];<br> <br>-              ast_copy_string(newtmp, "/tmp/XXXXXX", sizeof(newtmp));<br>-            ast_debug(3, "newtmp: %s\n", newtmp);<br>-              tmpfd = mkstemp(newtmp);<br>-             if (tmpfd < 0) {<br>-                  ast_log(LOG_WARNING, "Failed to create temporary file for volgain: %d\n", errno);<br>-                  ast_free(str1);<br>-                      ast_free(str2);<br>+              ast_copy_string(sox_gain_tmpdir, "/tmp/minivm-gain-XXXXXX", sizeof(sox_gain_tmpdir));<br>+              ast_debug(3, "sox_gain_tmpdir: %s\n", sox_gain_tmpdir);<br>+            if (!mkdtemp(sox_gain_tmpdir)) {<br>+                     ast_log(LOG_WARNING, "Failed to create temporary directory for volgain: %d\n", errno);<br>                      return -1;<br>            }<br>-            snprintf(tmpcmd, sizeof(tmpcmd), "sox -v %.4f %s.%s %s.%s", vmu->volgain, filename, format, newtmp, format);<br>-            ast_safe_system(tmpcmd);<br>-             close(tmpfd);<br>-                finalfilename = newtmp;<br>+              snprintf(fname, sizeof(fname), "%s/output.%s", sox_gain_tmpdir, format);<br>+           snprintf(sox_gain_cmd, sizeof(sox_gain_cmd), "sox -v %.4f %s.%s %s", vmu->volgain, filename, format, fname);<br>+            ast_safe_system(sox_gain_cmd);<br>                ast_debug(3, "VOLGAIN: Stored at: %s.%s - Level: %.4f - Mailbox: %s\n", filename, format, vmu->volgain, vmu->username);<br>+<br>+           /* Mark some things for deletion */<br>+          file_to_delete = fname;<br>+              dir_to_delete = sox_gain_tmpdir;<br>      } else {<br>-             finalfilename = ast_strdupa(filename);<br>+               snprintf(fname, sizeof(fname), "%s.%s", filename, format);<br>  }<br> <br>- /* Create file name */<br>-       snprintf(fname, sizeof(fname), "%s.%s", finalfilename, format);<br>-<br>  if (template->attachment)<br>-         ast_debug(1, "Attaching file '%s', format '%s', uservm is '%d'\n", finalfilename, format, attach_user_voicemail);<br>+          ast_debug(1, "Attaching file '%s', format '%s', uservm is '%d'\n", fname, format, attach_user_voicemail);<br> <br>        /* Make a temporary file instead of piping directly to sendmail, in case the mail<br>        command hangs */<br>@@ -1322,16 +1315,12 @@<br>  }<br>     if (!p) {<br>             ast_log(LOG_WARNING, "Unable to open temporary file '%s'\n", tmp);<br>-         ast_free(str1);<br>-              ast_free(str2);<br>-              return -1;<br>+           goto out;<br>     }<br>     /* Allocate channel used for chanvar substitution */<br>- ast = ast_dummy_channel_alloc();<br>-     if (!ast) {<br>-          ast_free(str1);<br>-              ast_free(str2);<br>-              return -1;<br>+   chan = ast_dummy_channel_alloc();<br>+    if (!chan) {<br>+         goto out;<br>     }<br> <br>  snprintf(dur, sizeof(dur), "%d:%02d", duration / 60, duration % 60);<br>@@ -1359,9 +1348,8 @@<br>         /* Set date format for voicemail mail */<br>      ast_strftime(date, sizeof(date), template->dateformat, &tm);<br> <br>-<br>     /* Populate channel with channel variables for substitution */<br>-       prep_email_sub_vars(ast, vmu, cidnum, cidname, dur, date, counter);<br>+  prep_email_sub_vars(chan, vmu, cidnum, cidname, dur, date, counter);<br> <br>       /* Find email address to use */<br>       /* If there's a server e-mail address in the account, use that, othterwise template */<br>@@ -1386,7 +1374,7 @@<br>             fprintf(p, "From: Asterisk PBX <%s>\n", who);<br>         } else {<br>              ast_debug(4, "Fromaddress template: %s\n", fromaddress);<br>-           ast_str_substitute_variables(&str1, 0, ast, fromaddress);<br>+                ast_str_substitute_variables(&str1, 0, chan, fromaddress);<br>                if (check_mime(ast_str_buffer(str1))) {<br>                       int first_line = 1;<br>                   char *ptr;<br>@@ -1429,7 +1417,7 @@<br>     }<br> <br>  if (!ast_strlen_zero(template->subject)) {<br>-                ast_str_substitute_variables(&str1, 0, ast, template->subject);<br>+               ast_str_substitute_variables(&str1, 0, chan, template->subject);<br>               if (check_mime(ast_str_buffer(str1))) {<br>                       int first_line = 1;<br>                   char *ptr;<br>@@ -1462,7 +1450,7 @@<br>     fprintf(p, "--%s\n", bound);<br>        fprintf(p, "Content-Type: text/plain; charset=%s\nContent-Transfer-Encoding: 8bit\n\n", template->charset);<br>      if (!ast_strlen_zero(template->body)) {<br>-           ast_str_substitute_variables(&str1, 0, ast, template->body);<br>+          ast_str_substitute_variables(&str1, 0, chan, template->body);<br>          ast_debug(3, "Message now: %s\n-----\n", ast_str_buffer(str1));<br>             fprintf(p, "%s\n", ast_str_buffer(str1));<br>   } else {<br>@@ -1489,14 +1477,45 @@<br>             fprintf(p, "\n\n--%s--\n.\n", bound);<br>       }<br>     fclose(p);<br>-   snprintf(tmp2, sizeof(tmp2), "( %s < %s ; rm -f %s ) &", global_mailcmd, tmp, tmp);<br>- ast_safe_system(tmp2);<br>-       ast_debug(1, "Sent message to %s with command '%s' - %s\n", vmu->email, global_mailcmd, template->attachment ? "(media attachment)" : "");<br>-       ast_debug(3, "Actual command used: %s\n", tmp2);<br>-   ast = ast_channel_unref(ast);<br>-        ast_free(str1);<br>-      ast_free(str2);<br>-      return 0;<br>+<br>+ chan = ast_channel_unref(chan);<br>+<br>+   if (file_to_delete && dir_to_delete) {<br>+               /* We can't delete these files ourselves because the mail command will execute in<br>+                   the background and we'll end up deleting them out from under it. */<br>+           res = snprintf(mail_cmd_buffer, sizeof(mail_cmd_buffer),<br>+                                        "( %s < %s ; rm -f %s %s ; rmdir %s ) &",<br>+                                           global_mailcmd, tmp, tmp, file_to_delete, dir_to_delete);<br>+ } else {<br>+             res = snprintf(mail_cmd_buffer, sizeof(mail_cmd_buffer),<br>+                                        "( %s < %s ; rm -f %s ) &",<br>+                                         global_mailcmd, tmp, tmp);<br>+        }<br>+<br>+ if (res < sizeof(mail_cmd_buffer)) {<br>+              file_to_delete = dir_to_delete = NULL;<br>+       } else {<br>+             ast_log(LOG_ERROR, "Could not send message, command line too long\n");<br>+             res = -1;<br>+            goto out;<br>+    }<br>+<br>+ ast_safe_system(mail_cmd_buffer);<br>+    ast_debug(1, "Sent message to %s with command '%s'%s\n", vmu->email, global_mailcmd, template->attachment ? " - (media attachment)" : "");<br>+       ast_debug(3, "Actual command used: %s\n", mail_cmd_buffer);<br>+<br>+     res = 0;<br>+<br>+out:<br>+   if (file_to_delete) {<br>+                unlink(file_to_delete);<br>+      }<br>+<br>+ if (dir_to_delete) {<br>+         rmdir(dir_to_delete);<br>+        }<br>+<br>+ return res;<br> }<br> <br> /*!\internal<br>diff --git a/apps/app_voicemail.c b/apps/app_voicemail.c<br>index f1b8bd1..a9b8fe3 100644<br>--- a/apps/app_voicemail.c<br>+++ b/apps/app_voicemail.c<br>@@ -5370,55 +5370,95 @@<br> <br> static int add_email_attachment(FILE *p, struct ast_vm_user *vmu, char *format, char *attach, char *greeting_attachment, char *mailbox, char *bound, char *filename, int last, int msgnum)<br> {<br>-      char tmpdir[256], newtmp[256];<br>-       char fname[256];<br>-     char tmpcmd[256];<br>-    int tmpfd = -1;<br>-      int soxstatus = 0;<br>+   char fname[PATH_MAX] = "";<br>+ char *file_to_delete = NULL, *dir_to_delete = NULL;<br>+  int res;<br> <br>   /* Eww. We want formats to tell us their own MIME type */<br>-    char *ctype = (!strcasecmp(format, "ogg")) ? "application/" : "audio/x-";<br>+      char *mime_type = (!strcasecmp(format, "ogg")) ? "application/" : "audio/x-";<br> <br>-       if (vmu->volgain < -.001 || vmu->volgain > .001) {<br>+       /* This 'while' loop will only execute once. We use it so that we can 'break' */<br>+     while (vmu->volgain < -.001 || vmu->volgain > .001) {<br>+            char tmpdir[PATH_MAX];<br>+               char sox_gain_tmpdir[PATH_MAX];<br>+<br>            create_dirpath(tmpdir, sizeof(tmpdir), vmu->context, vmu->mailbox, "tmp");<br>-           snprintf(newtmp, sizeof(newtmp), "%s/XXXXXX", tmpdir);<br>-             tmpfd = mkstemp(newtmp);<br>-             chmod(newtmp, VOICEMAIL_FILE_MODE & ~my_umask);<br>-          ast_debug(3, "newtmp: %s\n", newtmp);<br>-              if (tmpfd > -1) {<br>-                 snprintf(tmpcmd, sizeof(tmpcmd), "sox -v %.4f %s.%s %s.%s", vmu->volgain, attach, format, newtmp, format);<br>-                      if ((soxstatus = ast_safe_system(tmpcmd)) == 0) {<br>-                            attach = newtmp;<br>-                             ast_debug(3, "VOLGAIN: Stored at: %s.%s - Level: %.4f - Mailbox: %s\n", attach, format, vmu->volgain, mailbox);<br>+<br>+              res = snprintf(sox_gain_tmpdir, sizeof(sox_gain_tmpdir), "%s/vm-gain-XXXXXX", tmpdir);<br>+             if (res >= sizeof(sox_gain_tmpdir)) {<br>+                     ast_log(LOG_ERROR, "Failed to create temporary directory path %s: Out of buffer space\n", tmpdir);<br>+                 break;<br>+               }<br>+<br>+         if (mkdtemp(sox_gain_tmpdir)) {<br>+                      int soxstatus = 0;<br>+                   char sox_gain_cmd[PATH_MAX];<br>+<br>+                      ast_debug(3, "sox_gain_tmpdir: %s\n", sox_gain_tmpdir);<br>+<br>+                 /* Save for later */<br>+                 dir_to_delete = sox_gain_tmpdir;<br>+<br>+                  res = snprintf(fname, sizeof(fname), "%s/output.%s", sox_gain_tmpdir, format);<br>+                     if (res >= sizeof(fname)) {<br>+                               ast_log(LOG_ERROR, "Failed to create filename buffer for %s/output.%s: Too long\n", sox_gain_tmpdir, format);<br>+                              break;<br>+                       }<br>+<br>+                 res = snprintf(sox_gain_cmd, sizeof(sox_gain_cmd), "sox -v %.4f %s.%s %s",<br>+                                            vmu->volgain, attach, format, fname);<br>+                  if (res >= sizeof(sox_gain_cmd)) {<br>+                                ast_log(LOG_ERROR, "Failed to generate sox command, out of buffer space\n");<br>+                               break;<br>+                       }<br>+<br>+                 soxstatus = ast_safe_system(sox_gain_cmd);<br>+                   if (!soxstatus) {<br>+                            /* Save for later */<br>+                         file_to_delete = fname;<br>+                              ast_debug(3, "VOLGAIN: Stored at: %s - Level: %.4f - Mailbox: %s\n", fname, vmu->volgain, mailbox);<br>                      } else {<br>-                             ast_log(LOG_WARNING, "Sox failed to re-encode %s.%s: %s (have you installed support for all sox file formats?)\n", attach, format,<br>-                                 soxstatus == 1 ? "Problem with command line options" : "An error occurred during file processing");<br>+                              ast_log(LOG_WARNING, "Sox failed to re-encode %s: %s (have you installed support for all sox file formats?)\n",<br>+                                            fname,<br>+                                               soxstatus == 1 ? "Problem with command line options" : "An error occurred during file processing");<br>                               ast_log(LOG_WARNING, "Voicemail attachment will have no volume gain.\n");<br>                   }<br>             }<br>+<br>+         break;<br>        }<br>+<br>+ if (!file_to_delete) {<br>+               res = snprintf(fname, sizeof(fname), "%s.%s", attach, format);<br>+             if (res >= sizeof(fname)) {<br>+                       ast_log(LOG_ERROR, "Failed to create filename buffer for %s.%s: Too long\n", attach, format);<br>+                      return -1;<br>+           }<br>+    }<br>+<br>  fprintf(p, "--%s" ENDL, bound);<br>     if (msgnum > -1)<br>-          fprintf(p, "Content-Type: %s%s; name=\"%s\"" ENDL, ctype, format, filename);<br>+             fprintf(p, "Content-Type: %s%s; name=\"%s\"" ENDL, mime_type, format, filename);<br>  else<br>-         fprintf(p, "Content-Type: %s%s; name=\"%s.%s\"" ENDL, ctype, format, greeting_attachment, format);<br>+               fprintf(p, "Content-Type: %s%s; name=\"%s.%s\"" ENDL, mime_type, format, greeting_attachment, format);<br>    fprintf(p, "Content-Transfer-Encoding: base64" ENDL);<br>       fprintf(p, "Content-Description: Voicemail sound attachment." ENDL);<br>        if (msgnum > -1)<br>           fprintf(p, "Content-Disposition: attachment; filename=\"%s\"" ENDL ENDL, filename);<br>       else<br>          fprintf(p, "Content-Disposition: attachment; filename=\"%s.%s\"" ENDL ENDL, greeting_attachment, format);<br>-        snprintf(fname, sizeof(fname), "%s.%s", attach, format);<br>    base_encode(fname, p);<br>        if (last)<br>             fprintf(p, ENDL ENDL "--%s--" ENDL "." ENDL, bound);<br>-     if (tmpfd > -1) {<br>-         if (soxstatus == 0) {<br>-                        unlink(fname);<br>-               }<br>-            close(tmpfd);<br>-                unlink(newtmp);<br>+<br>+   if (file_to_delete) {<br>+                unlink(file_to_delete);<br>       }<br>+<br>+ if (dir_to_delete) {<br>+         rmdir(dir_to_delete);<br>+        }<br>+<br>  return 0;<br> }<br> <br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/6315">change 6315</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/6315"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: merged </div>
<div style="display:none"> Gerrit-Change-Id: I30ad04f0e115f0b11693ff678ba5184d8b938e43 </div>
<div style="display:none"> Gerrit-Change-Number: 6315 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Sean Bright <sean.bright@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>