proposed fix for memory leak in notmuch restore

proposed fix for memory leak in notmuch restore

[david]

In id:87vcc2q5n2.fsf@nikula.org, Jani points out a memory leak in the
new restore code.  This series is a proposed fix, namely do any
allocation on a temporary talloc context that is destroyed after each
line is processed.

[PATCH 1/3] tag-utils: use the tag_opt_list_t as talloc context, if possible.

[david]

From: David Bremner <bremner@debian.org>

This code is no less correct than the previous version, since it does
not make sense for the array to live longer than the wrapping struct.

By not relying on the context passed into tag_parse_line, we can allow
tag_op_list_t structures to live longer than that context.
---
 notmuch-restore.c |    2 +-
 tag-util.c        |    9 ++++-----
 tag-util.h        |    3 +--
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 40596a8..ae0ef45 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -102,7 +102,7 @@ parse_sup_line (void *ctx, char *line,
 	    tok_len++;
 	}
 
-	if (tag_op_list_append (ctx, tag_ops, tok, FALSE))
+	if (tag_op_list_append (tag_ops, tok, FALSE))
 	    return -1;
     }
 
diff --git a/tag-util.c b/tag-util.c
index eab482f..705b7ba 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -109,7 +109,7 @@ parse_tag_line (void *ctx, char *line,
 	    goto DONE;
 	}
 
-	if (tag_op_list_append (ctx, tag_ops, tag, remove)) {
+	if (tag_op_list_append (tag_ops, tag, remove)) {
 	    ret = line_error (TAG_PARSE_OUT_OF_MEMORY, line_for_error,
 			      "aborting");
 	    goto DONE;
@@ -294,7 +294,7 @@ tag_op_list_create (void *ctx)
     list->size = TAG_OP_LIST_INITIAL_SIZE;
     list->count = 0;
 
-    list->ops = talloc_array (ctx, tag_operation_t, list->size);
+    list->ops = talloc_array (list, tag_operation_t, list->size);
     if (list->ops == NULL)
 	return NULL;
 
@@ -303,8 +303,7 @@ tag_op_list_create (void *ctx)
 
 
 int
-tag_op_list_append (void *ctx,
-		    tag_op_list_t *list,
+tag_op_list_append (tag_op_list_t *list,
 		    const char *tag,
 		    notmuch_bool_t remove)
 {
@@ -314,7 +313,7 @@ tag_op_list_append (void *ctx,
 
     if (list->count == list->size) {
 	list->size *= 2;
-	list->ops = talloc_realloc (ctx, list->ops, tag_operation_t,
+	list->ops = talloc_realloc (list, list->ops, tag_operation_t,
 				    list->size);
 	if (list->ops == NULL) {
 	    fprintf (stderr, "Out of memory.\n");
diff --git a/tag-util.h b/tag-util.h
index 99b0fa0..c07bfde 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -87,8 +87,7 @@ tag_op_list_create (void *ctx);
  */
 
 int
-tag_op_list_append (void *ctx,
-		    tag_op_list_t *list,
+tag_op_list_append (tag_op_list_t *list,
 		    const char *tag,
 		    notmuch_bool_t remove);
 
-- 
1.7.10.4

[PATCH 2/3] notmuch-restore: replace continue with if

[david]

From: David Bremner <bremner@debian.org>

It is maybe a bit counter-intuitive with the condition reversed like
this, but it makes the memory handling fix in the next patch easier.
---
 notmuch-restore.c |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index ae0ef45..4b76d83 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -228,12 +228,15 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	    }
 	}
 
-	if (ret > 0)
-	    continue;
-
-	if (ret < 0 || tag_message (ctx, notmuch, query_string,
-				    tag_ops, flags))
-	    break;
+	if (ret <= 0) {
+	    if (ret < 0)
+		break;
+
+	    ret = tag_message (line_ctx, notmuch, query_string,
+			       tag_ops, flags);
+	    if (ret < 0)
+		break;
+	}
 
     }  while ((line_len = getline (&line, &line_size, input)) != -1);
 
-- 
1.7.10.4

[PATCH 3/3] notmuch-restore: allocate a temporary talloc context for each line parsed.

[david]

From: David Bremner <bremner@debian.org>

This lets the high level code in notmuch restore be ignorant about
what the lower level code is doing as far as allocating memory.
---
 notmuch-restore.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 4b76d83..52e7ecb 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -122,6 +122,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
     char *input_file_name = NULL;
     FILE *input = stdin;
     char *line = NULL;
+    void *line_ctx = NULL;
     size_t line_size;
     ssize_t line_len;
 
@@ -205,10 +206,11 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
     do {
 	char *query_string;
 
+	line_ctx = talloc_new (ctx);
 	if (input_format == DUMP_FORMAT_SUP) {
-	    ret = parse_sup_line (ctx, line, &query_string, tag_ops);
+	    ret = parse_sup_line (line_ctx, line, &query_string, tag_ops);
 	} else {
-	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS,
+	    ret = parse_tag_line (line_ctx, line, TAG_FLAG_BE_GENEROUS,
 				  &query_string, tag_ops);
 
 	    if (ret == 0) {
@@ -238,8 +240,14 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 		break;
 	}
 
+	talloc_free (line_ctx);
+	/* setting to NULL is important to avoid a double free */
+	line_ctx = NULL;
     }  while ((line_len = getline (&line, &line_size, input)) != -1);
 
+    if (line_ctx != NULL)
+	talloc_free (line_ctx);
+
     if (input_format == DUMP_FORMAT_SUP)
 	regfree (&regex);
 
-- 
1.7.10.4

Re: [PATCH 3/3] notmuch-restore: allocate a temporary talloc context for each line parsed.

[Jani Nikula]

On Sun, 16 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> This lets the high level code in notmuch restore be ignorant about
> what the lower level code is doing as far as allocating memory.
> ---
>  notmuch-restore.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index 4b76d83..52e7ecb 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -122,6 +122,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>      char *input_file_name = NULL;
>      FILE *input = stdin;
>      char *line = NULL;
> +    void *line_ctx = NULL;
>      size_t line_size;
>      ssize_t line_len;
>  
> @@ -205,10 +206,11 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>      do {
>  	char *query_string;

This patch only works on top of the batch tagging series, because
there's still one continue statement in the "id:" prefix check. But you
could make it work for both, *and* keep the slightly more intuitive ret
checking order if you did (yes, the slightly counter-intuitive):

	if (line_ctxt != NULL)
	  talloc_free (line_ctx);

right here, and...

> +	line_ctx = talloc_new (ctx);
>  	if (input_format == DUMP_FORMAT_SUP) {
> -	    ret = parse_sup_line (ctx, line, &query_string, tag_ops);
> +	    ret = parse_sup_line (line_ctx, line, &query_string, tag_ops);
>  	} else {
> -	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS,
> +	    ret = parse_tag_line (line_ctx, line, TAG_FLAG_BE_GENEROUS,
>  				  &query_string, tag_ops);
>  
>  	    if (ret == 0) {
> @@ -238,8 +240,14 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  		break;
>  	}
>  
> +	talloc_free (line_ctx);
> +	/* setting to NULL is important to avoid a double free */
> +	line_ctx = NULL;

...removed the above lines here.

Otherwise I think you'll need a temporary goto until the batch tagging
series. (I'm fine with that too.)

Overall the series LGTM, and I like how this dodges the query_string
alloc/free problem altogether.

BR,
Jani.

>      }  while ((line_len = getline (&line, &line_size, input)) != -1);
>  
> +    if (line_ctx != NULL)
> +	talloc_free (line_ctx);
> +
>      if (input_format == DUMP_FORMAT_SUP)
>  	regfree (&regex);
>  
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch