[asterisk-commits] voicemail: Fix various abuses of mkstemp (asterisk[14])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Tue Aug 29 05:16:01 CDT 2017
Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/6313 )
Change subject: voicemail: Fix various abuses of mkstemp
......................................................................
voicemail: Fix various abuses of mkstemp
mkstemp() returns a unique filename, but appending an extension to that
filename does not guarantee uniqueness. Instead, use mkdtemp() and we
can put whatever extension we want on the files that we create inside
the directory.
In the case of app_minivm, we also now properly clean up any temporary
files that we create.
ASTERISK-20858 #close
Reported by: Walter Doekes
Change-Id: I30ad04f0e115f0b11693ff678ba5184d8b938e43
---
M apps/app_minivm.c
M apps/app_voicemail.c
2 files changed, 136 insertions(+), 77 deletions(-)
Approvals:
Joshua Colp: Looks good to me, but someone else must approve
George Joseph: Looks good to me, approved
Jenkins2: Approved for Submit
diff --git a/apps/app_minivm.c b/apps/app_minivm.c
index aeac14a..41fd5e7 100644
--- a/apps/app_minivm.c
+++ b/apps/app_minivm.c
@@ -1234,6 +1234,8 @@
* \brief Send voicemail with audio file as an attachment */
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)
{
+ RAII_VAR(struct ast_str *, str1, ast_str_create(16), ast_free);
+ RAII_VAR(struct ast_str *, str2, ast_str_create(16), ast_free);
FILE *p = NULL;
int pfd;
char email[256] = "";
@@ -1243,20 +1245,18 @@
char fname[PATH_MAX];
char dur[PATH_MAX];
char tmp[80] = "/tmp/astmail-XXXXXX";
- char tmp2[PATH_MAX];
- char newtmp[PATH_MAX]; /* Only used with volgain */
+ char mail_cmd_buffer[PATH_MAX];
+ char sox_gain_tmpdir[PATH_MAX] = ""; /* Only used with volgain */
+ char *file_to_delete = NULL, *dir_to_delete = NULL;
struct timeval now;
struct ast_tm tm;
struct minivm_zone *the_zone = NULL;
- struct ast_channel *ast;
- char *finalfilename = "";
- struct ast_str *str1 = ast_str_create(16), *str2 = ast_str_create(16);
+ struct ast_channel *chan = NULL;
char *fromaddress;
char *fromemail;
+ int res;
if (!str1 || !str2) {
- ast_free(str1);
- ast_free(str2);
return -1;
}
@@ -1271,9 +1271,7 @@
if (ast_strlen_zero(email)) {
ast_log(LOG_WARNING, "No address to send message to.\n");
- ast_free(str1);
- ast_free(str2);
- return -1;
+ return -1;
}
ast_debug(3, "Sending mail to %s@%s - Using template %s\n", vmu->username, vmu->domain, template->name);
@@ -1281,35 +1279,30 @@
if (!strcmp(format, "wav49"))
format = "WAV";
-
/* If we have a gain option, process it now with sox */
if (type == MVM_MESSAGE_EMAIL && (vmu->volgain < -.001 || vmu->volgain > .001) ) {
- char tmpcmd[PATH_MAX];
- int tmpfd;
+ char sox_gain_cmd[PATH_MAX];
- ast_copy_string(newtmp, "/tmp/XXXXXX", sizeof(newtmp));
- ast_debug(3, "newtmp: %s\n", newtmp);
- tmpfd = mkstemp(newtmp);
- if (tmpfd < 0) {
- ast_log(LOG_WARNING, "Failed to create temporary file for volgain: %d\n", errno);
- ast_free(str1);
- ast_free(str2);
+ ast_copy_string(sox_gain_tmpdir, "/tmp/minivm-gain-XXXXXX", sizeof(sox_gain_tmpdir));
+ ast_debug(3, "sox_gain_tmpdir: %s\n", sox_gain_tmpdir);
+ if (!mkdtemp(sox_gain_tmpdir)) {
+ ast_log(LOG_WARNING, "Failed to create temporary directory for volgain: %d\n", errno);
return -1;
}
- snprintf(tmpcmd, sizeof(tmpcmd), "sox -v %.4f %s.%s %s.%s", vmu->volgain, filename, format, newtmp, format);
- ast_safe_system(tmpcmd);
- close(tmpfd);
- finalfilename = newtmp;
+ snprintf(fname, sizeof(fname), "%s/output.%s", sox_gain_tmpdir, format);
+ snprintf(sox_gain_cmd, sizeof(sox_gain_cmd), "sox -v %.4f %s.%s %s", vmu->volgain, filename, format, fname);
+ ast_safe_system(sox_gain_cmd);
ast_debug(3, "VOLGAIN: Stored at: %s.%s - Level: %.4f - Mailbox: %s\n", filename, format, vmu->volgain, vmu->username);
+
+ /* Mark some things for deletion */
+ file_to_delete = fname;
+ dir_to_delete = sox_gain_tmpdir;
} else {
- finalfilename = ast_strdupa(filename);
+ snprintf(fname, sizeof(fname), "%s.%s", filename, format);
}
- /* Create file name */
- snprintf(fname, sizeof(fname), "%s.%s", finalfilename, format);
-
if (template->attachment)
- ast_debug(1, "Attaching file '%s', format '%s', uservm is '%d'\n", finalfilename, format, attach_user_voicemail);
+ ast_debug(1, "Attaching file '%s', format '%s', uservm is '%d'\n", fname, format, attach_user_voicemail);
/* Make a temporary file instead of piping directly to sendmail, in case the mail
command hangs */
@@ -1324,16 +1317,12 @@
}
if (!p) {
ast_log(LOG_WARNING, "Unable to open temporary file '%s'\n", tmp);
- ast_free(str1);
- ast_free(str2);
- return -1;
+ goto out;
}
/* Allocate channel used for chanvar substitution */
- ast = ast_dummy_channel_alloc();
- if (!ast) {
- ast_free(str1);
- ast_free(str2);
- return -1;
+ chan = ast_dummy_channel_alloc();
+ if (!chan) {
+ goto out;
}
snprintf(dur, sizeof(dur), "%d:%02d", duration / 60, duration % 60);
@@ -1361,9 +1350,8 @@
/* Set date format for voicemail mail */
ast_strftime(date, sizeof(date), template->dateformat, &tm);
-
/* Populate channel with channel variables for substitution */
- prep_email_sub_vars(ast, vmu, cidnum, cidname, dur, date, counter);
+ prep_email_sub_vars(chan, vmu, cidnum, cidname, dur, date, counter);
/* Find email address to use */
/* If there's a server e-mail address in the account, use that, othterwise template */
@@ -1388,7 +1376,7 @@
fprintf(p, "From: Asterisk PBX <%s>\n", who);
} else {
ast_debug(4, "Fromaddress template: %s\n", fromaddress);
- ast_str_substitute_variables(&str1, 0, ast, fromaddress);
+ ast_str_substitute_variables(&str1, 0, chan, fromaddress);
if (check_mime(ast_str_buffer(str1))) {
int first_line = 1;
char *ptr;
@@ -1431,7 +1419,7 @@
}
if (!ast_strlen_zero(template->subject)) {
- ast_str_substitute_variables(&str1, 0, ast, template->subject);
+ ast_str_substitute_variables(&str1, 0, chan, template->subject);
if (check_mime(ast_str_buffer(str1))) {
int first_line = 1;
char *ptr;
@@ -1464,7 +1452,7 @@
fprintf(p, "--%s\n", bound);
fprintf(p, "Content-Type: text/plain; charset=%s\nContent-Transfer-Encoding: 8bit\n\n", template->charset);
if (!ast_strlen_zero(template->body)) {
- ast_str_substitute_variables(&str1, 0, ast, template->body);
+ ast_str_substitute_variables(&str1, 0, chan, template->body);
ast_debug(3, "Message now: %s\n-----\n", ast_str_buffer(str1));
fprintf(p, "%s\n", ast_str_buffer(str1));
} else {
@@ -1491,14 +1479,45 @@
fprintf(p, "\n\n--%s--\n.\n", bound);
}
fclose(p);
- snprintf(tmp2, sizeof(tmp2), "( %s < %s ; rm -f %s ) &", global_mailcmd, tmp, tmp);
- ast_safe_system(tmp2);
- ast_debug(1, "Sent message to %s with command '%s' - %s\n", vmu->email, global_mailcmd, template->attachment ? "(media attachment)" : "");
- ast_debug(3, "Actual command used: %s\n", tmp2);
- ast = ast_channel_unref(ast);
- ast_free(str1);
- ast_free(str2);
- return 0;
+
+ chan = ast_channel_unref(chan);
+
+ if (file_to_delete && dir_to_delete) {
+ /* We can't delete these files ourselves because the mail command will execute in
+ the background and we'll end up deleting them out from under it. */
+ res = snprintf(mail_cmd_buffer, sizeof(mail_cmd_buffer),
+ "( %s < %s ; rm -f %s %s ; rmdir %s ) &",
+ global_mailcmd, tmp, tmp, file_to_delete, dir_to_delete);
+ } else {
+ res = snprintf(mail_cmd_buffer, sizeof(mail_cmd_buffer),
+ "( %s < %s ; rm -f %s ) &",
+ global_mailcmd, tmp, tmp);
+ }
+
+ if (res < sizeof(mail_cmd_buffer)) {
+ file_to_delete = dir_to_delete = NULL;
+ } else {
+ ast_log(LOG_ERROR, "Could not send message, command line too long\n");
+ res = -1;
+ goto out;
+ }
+
+ ast_safe_system(mail_cmd_buffer);
+ ast_debug(1, "Sent message to %s with command '%s'%s\n", vmu->email, global_mailcmd, template->attachment ? " - (media attachment)" : "");
+ ast_debug(3, "Actual command used: %s\n", mail_cmd_buffer);
+
+ res = 0;
+
+out:
+ if (file_to_delete) {
+ unlink(file_to_delete);
+ }
+
+ if (dir_to_delete) {
+ rmdir(dir_to_delete);
+ }
+
+ return res;
}
/*!\internal
diff --git a/apps/app_voicemail.c b/apps/app_voicemail.c
index 1714a78..4597253 100644
--- a/apps/app_voicemail.c
+++ b/apps/app_voicemail.c
@@ -5372,55 +5372,95 @@
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)
{
- char tmpdir[256], newtmp[256];
- char fname[256];
- char tmpcmd[256];
- int tmpfd = -1;
- int soxstatus = 0;
+ char fname[PATH_MAX] = "";
+ char *file_to_delete = NULL, *dir_to_delete = NULL;
+ int res;
/* Eww. We want formats to tell us their own MIME type */
- char *ctype = (!strcasecmp(format, "ogg")) ? "application/" : "audio/x-";
+ char *mime_type = (!strcasecmp(format, "ogg")) ? "application/" : "audio/x-";
- if (vmu->volgain < -.001 || vmu->volgain > .001) {
+ /* This 'while' loop will only execute once. We use it so that we can 'break' */
+ while (vmu->volgain < -.001 || vmu->volgain > .001) {
+ char tmpdir[PATH_MAX];
+ char sox_gain_tmpdir[PATH_MAX];
+
create_dirpath(tmpdir, sizeof(tmpdir), vmu->context, vmu->mailbox, "tmp");
- snprintf(newtmp, sizeof(newtmp), "%s/XXXXXX", tmpdir);
- tmpfd = mkstemp(newtmp);
- chmod(newtmp, VOICEMAIL_FILE_MODE & ~my_umask);
- ast_debug(3, "newtmp: %s\n", newtmp);
- if (tmpfd > -1) {
- snprintf(tmpcmd, sizeof(tmpcmd), "sox -v %.4f %s.%s %s.%s", vmu->volgain, attach, format, newtmp, format);
- if ((soxstatus = ast_safe_system(tmpcmd)) == 0) {
- attach = newtmp;
- ast_debug(3, "VOLGAIN: Stored at: %s.%s - Level: %.4f - Mailbox: %s\n", attach, format, vmu->volgain, mailbox);
+
+ res = snprintf(sox_gain_tmpdir, sizeof(sox_gain_tmpdir), "%s/vm-gain-XXXXXX", tmpdir);
+ if (res >= sizeof(sox_gain_tmpdir)) {
+ ast_log(LOG_ERROR, "Failed to create temporary directory path %s: Out of buffer space\n", tmpdir);
+ break;
+ }
+
+ if (mkdtemp(sox_gain_tmpdir)) {
+ int soxstatus = 0;
+ char sox_gain_cmd[PATH_MAX];
+
+ ast_debug(3, "sox_gain_tmpdir: %s\n", sox_gain_tmpdir);
+
+ /* Save for later */
+ dir_to_delete = sox_gain_tmpdir;
+
+ res = snprintf(fname, sizeof(fname), "%s/output.%s", sox_gain_tmpdir, format);
+ if (res >= sizeof(fname)) {
+ ast_log(LOG_ERROR, "Failed to create filename buffer for %s/output.%s: Too long\n", sox_gain_tmpdir, format);
+ break;
+ }
+
+ res = snprintf(sox_gain_cmd, sizeof(sox_gain_cmd), "sox -v %.4f %s.%s %s",
+ vmu->volgain, attach, format, fname);
+ if (res >= sizeof(sox_gain_cmd)) {
+ ast_log(LOG_ERROR, "Failed to generate sox command, out of buffer space\n");
+ break;
+ }
+
+ soxstatus = ast_safe_system(sox_gain_cmd);
+ if (!soxstatus) {
+ /* Save for later */
+ file_to_delete = fname;
+ ast_debug(3, "VOLGAIN: Stored at: %s - Level: %.4f - Mailbox: %s\n", fname, vmu->volgain, mailbox);
} else {
- ast_log(LOG_WARNING, "Sox failed to re-encode %s.%s: %s (have you installed support for all sox file formats?)\n", attach, format,
- soxstatus == 1 ? "Problem with command line options" : "An error occurred during file processing");
+ ast_log(LOG_WARNING, "Sox failed to re-encode %s: %s (have you installed support for all sox file formats?)\n",
+ fname,
+ soxstatus == 1 ? "Problem with command line options" : "An error occurred during file processing");
ast_log(LOG_WARNING, "Voicemail attachment will have no volume gain.\n");
}
}
+
+ break;
}
+
+ if (!file_to_delete) {
+ res = snprintf(fname, sizeof(fname), "%s.%s", attach, format);
+ if (res >= sizeof(fname)) {
+ ast_log(LOG_ERROR, "Failed to create filename buffer for %s.%s: Too long\n", attach, format);
+ return -1;
+ }
+ }
+
fprintf(p, "--%s" ENDL, bound);
if (msgnum > -1)
- fprintf(p, "Content-Type: %s%s; name=\"%s\"" ENDL, ctype, format, filename);
+ fprintf(p, "Content-Type: %s%s; name=\"%s\"" ENDL, mime_type, format, filename);
else
- fprintf(p, "Content-Type: %s%s; name=\"%s.%s\"" ENDL, ctype, format, greeting_attachment, format);
+ fprintf(p, "Content-Type: %s%s; name=\"%s.%s\"" ENDL, mime_type, format, greeting_attachment, format);
fprintf(p, "Content-Transfer-Encoding: base64" ENDL);
fprintf(p, "Content-Description: Voicemail sound attachment." ENDL);
if (msgnum > -1)
fprintf(p, "Content-Disposition: attachment; filename=\"%s\"" ENDL ENDL, filename);
else
fprintf(p, "Content-Disposition: attachment; filename=\"%s.%s\"" ENDL ENDL, greeting_attachment, format);
- snprintf(fname, sizeof(fname), "%s.%s", attach, format);
base_encode(fname, p);
if (last)
fprintf(p, ENDL ENDL "--%s--" ENDL "." ENDL, bound);
- if (tmpfd > -1) {
- if (soxstatus == 0) {
- unlink(fname);
- }
- close(tmpfd);
- unlink(newtmp);
+
+ if (file_to_delete) {
+ unlink(file_to_delete);
}
+
+ if (dir_to_delete) {
+ rmdir(dir_to_delete);
+ }
+
return 0;
}
--
To view, visit https://gerrit.asterisk.org/6313
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-MessageType: merged
Gerrit-Change-Id: I30ad04f0e115f0b11693ff678ba5184d8b938e43
Gerrit-Change-Number: 6313
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-commits/attachments/20170829/76d23c5b/attachment-0001.html>
More information about the asterisk-commits
mailing list