<p>Sean Bright has uploaded this change for <strong>review</strong>.</p><p><a href="https://gerrit.asterisk.org/6312">View Change</a></p><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;">git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/12/6312/1</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 43cd7a2..caf9f01 100644<br>--- a/apps/app_minivm.c<br>+++ b/apps/app_minivm.c<br>@@ -1234,6 +1234,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>@@ -1243,20 +1245,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>@@ -1271,9 +1271,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>@@ -1281,35 +1279,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>@@ -1324,16 +1317,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>@@ -1361,9 +1350,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>@@ -1388,7 +1376,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>@@ -1431,7 +1419,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>@@ -1464,7 +1452,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>@@ -1491,14 +1479,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 58b4e64..f954e27 100644<br>--- a/apps/app_voicemail.c<br>+++ b/apps/app_voicemail.c<br>@@ -5372,55 +5372,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/6312">change 6312</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/6312"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: newchange </div>
<div style="display:none"> Gerrit-Change-Id: I30ad04f0e115f0b11693ff678ba5184d8b938e43 </div>
<div style="display:none"> Gerrit-Change-Number: 6312 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Sean Bright <sean.bright@gmail.com> </div>