setup_paths leaks memory: realpath is already set in mx_open_mailbox()
restore_paths is unneeded. mx_fastclose_mailbox() will free stuff,
and nothing is looking at the path once we are closing or aborting.
Make a copy of the hooks. Otherwise 'unhook *' will leave dangling
pointers.
Add compress_info freeing inside mx_fastclose_mailbox(). Only free
inside compress.c when we want to prevent close() from doing anything.
close_mailbox() didn't preserve ctx->path on error.
execute_command() didn't return an error if the mutt_system() command
failed.
mx_open_mailbox_append() should check mutt_comp_can_append() only for
the case that the mailbox doesn't exist. When it exists,
mx_get_magic() has already looked at the file contents before checking
for matching open_hooks.
In open_append_mailbox() if no append hook is defined, it should't
call ci->open() if the mailbox doesn't exist. It should act just like
append and create a temporary file.
check_mailbox() needs more work. For now, at least have it properly
close the mailbox on error.
# HG changeset patch
# User Kevin McCarthy <ke...@8t8.us>
# Date 1478734908 28800
# Wed Nov 09 15:41:48 2016 -0800
# Node ID 1cc878cc71bf90d5358230075b05029b7c839d77
# Parent 326fac112b39ddfde430e80a6ee58fed74c0b9e0
Compress: fix several logic and memory bugs.
setup_paths leaks memory: realpath is already set in mx_open_mailbox()
restore_paths is unneeded. mx_fastclose_mailbox() will free stuff,
and nothing is looking at the path once we are closing or aborting.
Make a copy of the hooks. Otherwise 'unhook *' will leave dangling
pointers.
Add compress_info freeing inside mx_fastclose_mailbox(). Only free
inside compress.c when we want to prevent close() from doing anything.
close_mailbox() didn't preserve ctx->path on error.
execute_command() didn't return an error if the mutt_system() command
failed.
mx_open_mailbox_append() should check mutt_comp_can_append() only for
the case that the mailbox doesn't exist. When it exists,
mx_get_magic() has already looked at the file contents before checking
for matching open_hooks.
In open_append_mailbox() if no append hook is defined, it should't
call ci->open() if the mailbox doesn't exist. It should act just like
append and create a temporary file.
check_mailbox() needs more work. For now, at least have it properly
close the mailbox on error.
diff --git a/compress.c b/compress.c
--- a/compress.c
+++ b/compress.c
@@ -112,60 +112,35 @@
/**
* setup_paths - Set the mailbox paths
* @ctx: Mailbox to modify
*
* Save the compressed filename in ctx->realpath.
* Create a temporary filename and put its name in ctx->path.
*
* Note: The temporary file is NOT created.
- * Note: ctx->path will be freed by restore_path()
*/
static void
setup_paths (CONTEXT *ctx)
{
if (!ctx)
return;
char tmppath[_POSIX_PATH_MAX];
/* Setup the right paths */
+ FREE(&ctx->realpath);
ctx->realpath = ctx->path;
/* We will uncompress to /tmp */
mutt_mktemp (tmppath, sizeof (tmppath));
ctx->path = safe_strdup (tmppath);
}
/**
- * restore_path - Put back the original mailbox name
- * @ctx: Mailbox to modify
- *
- * When we use a compressed mailbox, we change the CONTEXT to refer to the
- * uncompressed file. We store the original name in ctx->realpath.
- * ctx->path = "/tmp/mailbox"
- * ctx->realpath = "mailbox.gz"
- *
- * When we have finished with a compressed mailbox, we put back the original
- * name.
- * ctx->path = "mailbox.gz"
- * ctx->realpath = NULL
- */
-static void
-restore_path (CONTEXT *ctx)
-{
- if (!ctx)
- return;
-
- FREE(&ctx->path);
- ctx->path = ctx->realpath;
- ctx->realpath = NULL;
-}
-
-/**
* get_size - Get the size of a file
* @path: File to measure
*
* Returns:
* number: Size in bytes
* 0: On error
*/
static int
@@ -232,18 +207,16 @@
}
/**
* set_compress_info - Find the compress hooks for a mailbox
* @ctx: Mailbox to examine
*
* When a mailbox is opened, we check if there are any matching hooks.
*
- * Note: Caller must free the COMPRESS_INFO when done.
- *
* Returns:
* COMPRESS_INFO: Hook info for the mailbox's path
* NULL: On error
*/
static COMPRESS_INFO *
set_compress_info (CONTEXT *ctx)
{
if (!ctx || !ctx->path)
@@ -258,24 +231,44 @@
return NULL;
const char *c = find_hook (MUTT_CLOSEHOOK, ctx->path);
const char *a = find_hook (MUTT_APPENDHOOK, ctx->path);
COMPRESS_INFO *ci = safe_calloc (1, sizeof (COMPRESS_INFO));
ctx->compress_info = ci;
- ci->open = o;
- ci->close = c;
- ci->append = a;
+ ci->open = safe_strdup (o);
+ ci->close = safe_strdup (c);
+ ci->append = safe_strdup (a);
return ci;
}
/**
+ * mutt_free_compress_info - Frees the compress info members and structure.
+ * @ctx: Mailbox to free compress_info for.
+ */
+void
+mutt_free_compress_info (CONTEXT *ctx)
+{
+ COMPRESS_INFO *ci;
+
+ if (!ctx || !ctx->compress_info)
+ return;
+
+ ci = ctx->compress_info;
+ FREE (&ci->open);
+ FREE (&ci->close);
+ FREE (&ci->append);
+
+ FREE (&ctx->compress_info);
+}
+
+/**
* cb_format_str - Expand the filenames in the command string
* @dest: Buffer in which to save string
* @destlen: Buffer length
* @col: Starting column, UNUSED
* @cols: Number of screen columns, UNUSED
* @op: printf-like operator, e.g. 't'
* @src: printf-like format string
* @fmt: Field formatting string, UNUSED
@@ -350,63 +343,67 @@
*
* Returns:
* 1: Success
* 0: Failure
*/
static int
execute_command (CONTEXT *ctx, const char *command, int create_file, const char *progress)
{
+ FILE *fp;
+ int rc = 0;
+ int unlock = 0;
+ char sys_cmd[HUGE_STRING];
+
if (!ctx || !command || !progress)
return 0;
if (!ctx->quiet)
mutt_message (progress, ctx->realpath);
- FILE *fp;
if (create_file)
fp = fopen (ctx->realpath, "a");
else
fp = fopen (ctx->realpath, "r");
-
if (!fp)
{
mutt_perror (ctx->realpath);
return 0;
}
mutt_block_signals();
/* If we're creating the file, lock it exclusively */
if (!lock_mailbox (ctx, fp, create_file))
{
- safe_fclose (&fp);
- mutt_unblock_signals();
mutt_error (_("Unable to lock mailbox!"));
- return 0;
+ goto cleanup;
}
+ unlock = 1;
endwin();
fflush (stdout);
- char sys_cmd[HUGE_STRING];
-
expand_command_str (ctx, command, sys_cmd, sizeof (sys_cmd));
- int rc = mutt_system (sys_cmd);
- if (rc != 0)
+ if (mutt_system (sys_cmd) != 0)
{
mutt_any_key_to_continue (NULL);
mutt_error (_("Error executing: %s\n"), sys_cmd);
+ goto cleanup;
}
- unlock_mailbox (ctx, fp);
+ rc = 1;
+
+cleanup:
+ if (unlock)
+ unlock_mailbox (ctx, fp);
mutt_unblock_signals();
safe_fclose (&fp);
- return 1;
+ return rc;
}
/**
* open_read - Open a compressed mailbox for reading
* @ctx: Mailbox to open
*
* Decompress the mailbox and set up the paths and hooks needed.
*
@@ -456,17 +453,17 @@
goto or_fail;
}
return 1;
or_fail:
/* remove the partial uncompressed file */
remove (ctx->path);
- restore_path (ctx);
+ mutt_free_compress_info (ctx);
return 0;
}
/**
* open_mailbox - Open a compressed mailbox
* @ctx: Mailbox to open
*
@@ -517,69 +514,71 @@
/* If this succeeds, we know there's an open-hook */
COMPRESS_INFO *ci = set_compress_info (ctx);
if (!ci)
return -1;
/* To append we need an append-hook or a close-hook */
if (!ci->append && !ci->close)
{
- FREE(&ctx->compress_info);
mutt_error (_("Cannot append without an append-hook or close-hook : %s"), ctx->path);
- return -1;
+ goto oa_fail1;
}
ctx->magic = DefaultMagic;
/* We can only deal with mbox and mmdf mailboxes */
if ((ctx->magic != MUTT_MBOX) && (ctx->magic != MUTT_MMDF))
- return -1;
+ goto oa_fail1;
setup_paths (ctx);
ctx->mx_ops = &mx_comp_ops;
ci->child_ops = mx_get_ops (ctx->magic);
if (!ci->child_ops)
{
mutt_error (_("Can't find mailbox ops for mailbox type %d"), ctx->magic);
- return -1;
+ goto oa_fail2;
}
- if (ci->append)
+ if (ci->append || (access (ctx->realpath, F_OK) != 0))
{
/* Create an empty temporary file */
ctx->fp = safe_fopen (ctx->path, "w");
if (!ctx->fp)
{
mutt_perror (ctx->path);
- goto oa_fail;
+ goto oa_fail2;
}
}
else
{
/* Open the existing mailbox */
int rc = execute_command (ctx, ci->open, 0, _("Decompressing %s"));
if (rc == 0)
{
mutt_error (_("Compress command failed: %s"), ci->open);
- goto oa_fail;
+ goto oa_fail2;
}
ctx->fp = safe_fopen (ctx->path, "a");
if (!ctx->fp)
{
mutt_perror (ctx->path);
- goto oa_fail;
+ goto oa_fail2;
}
}
return 0;
-oa_fail:
+oa_fail2:
/* remove the partial uncompressed file */
remove (ctx->path);
- restore_path (ctx);
+oa_fail1:
+ /* Free the compress_info to prevent close from trying to recompress */
+ mutt_free_compress_info (ctx);
+
return -1;
}
/**
* close_mailbox - Close a compressed mailbox
* @ctx: Mailbox to close
*
* If the mailbox has been changed then re-compress the tmp file.
@@ -609,18 +608,16 @@
{
remove (ctx->realpath);
}
else
{
remove (ctx->path);
}
- restore_path (ctx);
- FREE(&ctx->compress_info);
return 0;
}
const char *append;
const char *msg;
/* The file exists and we can append */
if ((access (ctx->realpath, F_OK) == 0) && ci->append)
@@ -635,20 +632,18 @@
}
int rc = execute_command (ctx, append, 1, msg);
if (rc == 0)
{
mutt_any_key_to_continue (NULL);
mutt_error (_(" %s: Error compressing mailbox! Uncompressed one kept!\n"), ctx->path);
}
-
- remove (ctx->path);
- restore_path (ctx);
- FREE(&ctx->compress_info);
+ else
+ remove (ctx->path);
return 0;
}
/**
* check_mailbox - Check for changes in the compressed file
* @ctx: Mailbox
*
@@ -673,24 +668,27 @@
COMPRESS_INFO *ci = ctx->compress_info;
if (!ci)
return -1;
int size = get_size (ctx->realpath);
if (size == ci->size)
return 0;
+ /* TODO: this is a copout. We should reopen the compressed mailbox
+ * and call mutt_reopen_mailbox. */
if (ctx->changed)
{
- FREE(&ctx->compress_info);
- restore_path (ctx);
+ mutt_free_compress_info (ctx);
+ mx_fastclose_mailbox (ctx);
mutt_error (_("Mailbox was corrupted!"));
return -1;
}
+ /* TODO: this block leaks memory. this is doing it all wrong */
close_mailbox (ctx);
const char *path = ctx->path;
ctx->path = NULL;
mx_open_mailbox (path, 0, ctx);
FREE(&path);
diff --git a/compress.h b/compress.h
--- a/compress.h
+++ b/compress.h
@@ -14,16 +14,18 @@
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
#ifndef _COMPRESS_H_
#define _COMPRESS_H_
+void mutt_free_compress_info (CONTEXT *ctx);
+
int mutt_comp_can_append (CONTEXT *ctx);
int mutt_comp_can_read (const char *path);
int mutt_comp_sync (CONTEXT *ctx);
int mutt_comp_valid_command (const char *cmd);
extern struct mx_ops mx_comp_ops;
#endif /* _COMPRESS_H_ */
diff --git a/mx.c b/mx.c
--- a/mx.c
+++ b/mx.c
@@ -503,34 +503,34 @@
}
if (ctx->magic < 0)
{
if (stat (ctx->path, &sb) == -1)
{
if (errno == ENOENT)
{
- ctx->magic = DefaultMagic;
+#ifdef USE_COMPRESSED
+ if (mutt_comp_can_append (ctx))
+ ctx->magic = MUTT_COMPRESSED;
+ else
+#endif
+ ctx->magic = DefaultMagic;
flags |= MUTT_APPENDNEW;
}
else
{
mutt_perror (ctx->path);
return -1;
}
}
else
return -1;
}
-#ifdef USE_COMPRESSED
- if (mutt_comp_can_append (ctx))
- ctx->mx_ops = &mx_comp_ops;
- else
-#endif
ctx->mx_ops = mx_get_ops (ctx->magic);
if (!ctx->mx_ops || !ctx->mx_ops->open_append)
return -1;
return ctx->mx_ops->open_append (ctx, flags);
}
/*
@@ -652,16 +652,20 @@
/* never announce that a mailbox we've just left has new mail. #3290
* XXX: really belongs in mx_close_mailbox, but this is a nice hook point */
if (!ctx->peekonly)
mutt_buffy_setnotified(ctx->path);
if (ctx->mx_ops)
ctx->mx_ops->close (ctx);
+#ifdef USE_COMPRESSED
+ mutt_free_compress_info (ctx);
+#endif /* USE_COMPRESSED */
+
if (ctx->subj_hash)
hash_destroy (&ctx->subj_hash, NULL);
if (ctx->id_hash)
hash_destroy (&ctx->id_hash, NULL);
mutt_clear_threads (ctx);
for (i = 0; i < ctx->msgcount; i++)
mutt_free_header (&ctx->hdrs[i]);
FREE (&ctx->hdrs);
