[Asterisk-code-review] voicemail: Fix various abuses of mkstemp (asterisk[15])
Sean Bright
asteriskteam at digium.com
Fri Aug 25 16:08:48 CDT 2017
Sean Bright has uploaded this change for review. ( https://gerrit.asterisk.org/6314
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(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/14/6314/1
diff --git a/apps/app_minivm.c b/apps/app_minivm.c
index 9359f82..15449ad 100644
--- a/apps/app_minivm.c
+++ b/apps/app_minivm.c
@@ -1232,6 +1232,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] = "";
@@ -1241,20 +1243,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;
}
@@ -1269,9 +1269,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);
@@ -1279,35 +1277,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 */
@@ -1322,16 +1315,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);
@@ -1359,9 +1348,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 */
@@ -1386,7 +1374,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;
@@ -1429,7 +1417,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;
@@ -1462,7 +1450,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 {
@@ -1489,14 +1477,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 f1b8bd1..a9b8fe3 100644
--- a/apps/app_voicemail.c
+++ b/apps/app_voicemail.c
@@ -5370,55 +5370,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/6314
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: newchange
Gerrit-Change-Id: I30ad04f0e115f0b11693ff678ba5184d8b938e43
Gerrit-Change-Number: 6314
Gerrit-PatchSet: 1
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170825/0b402895/attachment-0001.html>
More information about the asterisk-code-review
mailing list