* [PATCH 1/3] notmuch-restore: fix return value propagation
2012-12-17 3:59 v2 restore leak fix david
@ 2012-12-17 3:59 ` david
2012-12-17 3:59 ` [PATCH 2/3] tag-utils: use the tag_opt_list_t as talloc context, if possible david
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: david @ 2012-12-17 3:59 UTC (permalink / raw)
To: notmuch; +Cc: David Bremner
From: David Bremner <bremner@debian.org>
Previously notmuch_restore_command returned 0 if tag_message returned
a non-zero (failure) value. This is wrong, since non-zero status
indicates something mysterious went wrong with retrieving the message,
or applying it.
There was also a failure to check or propagate the return value from
tag_op_list_apply in tag_message.
---
notmuch-restore.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/notmuch-restore.c b/notmuch-restore.c
index 40596a8..5a02328 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -25,6 +25,9 @@
static regex_t regex;
+/* Non-zero return indicates an error in retrieving the message,
+ * or in applying the tags.
+ */
static int
tag_message (unused (void *ctx),
notmuch_database_t *notmuch,
@@ -48,7 +51,7 @@ tag_message (unused (void *ctx),
/* In order to detect missing messages, this check/optimization is
* intentionally done *after* first finding the message. */
if ((flags & TAG_FLAG_REMOVE_ALL) || tag_op_list_size (tag_ops))
- tag_op_list_apply (message, tag_ops, flags);
+ ret = tag_op_list_apply (message, tag_ops, flags);
notmuch_message_destroy (message);
@@ -231,8 +234,12 @@ 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))
+ if (ret < 0)
+ break;
+
+ ret = tag_message (line_ctx, notmuch, query_string,
+ tag_ops, flags);
+ if (ret)
break;
} while ((line_len = getline (&line, &line_size, input)) != -1);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/3] tag-utils: use the tag_opt_list_t as talloc context, if possible.
2012-12-17 3:59 v2 restore leak fix david
2012-12-17 3:59 ` [PATCH 1/3] notmuch-restore: fix return value propagation david
@ 2012-12-17 3:59 ` david
2012-12-17 16:24 ` Tomi Ollila
2012-12-17 3:59 ` [PATCH 3/3] notmuch-restore: allocate a temporary talloc context for each line parsed david
2012-12-22 21:38 ` v2 restore leak fix Jani Nikula
3 siblings, 1 reply; 7+ messages in thread
From: david @ 2012-12-17 3:59 UTC (permalink / raw)
To: notmuch; +Cc: David Bremner
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 5a02328..665373f 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -105,7 +105,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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/3] tag-utils: use the tag_opt_list_t as talloc context, if possible.
2012-12-17 3:59 ` [PATCH 2/3] tag-utils: use the tag_opt_list_t as talloc context, if possible david
@ 2012-12-17 16:24 ` Tomi Ollila
0 siblings, 0 replies; 7+ messages in thread
From: Tomi Ollila @ 2012-12-17 16:24 UTC (permalink / raw)
To: david, notmuch; +Cc: David Bremner
On Mon, Dec 17 2012, david@tethera.net wrote:
> 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.
> ---
The whole patch series looks good to me -- just that this comment
gets a bit over my head...
Tomi
> 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 5a02328..665373f 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -105,7 +105,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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] notmuch-restore: allocate a temporary talloc context for each line parsed.
2012-12-17 3:59 v2 restore leak fix david
2012-12-17 3:59 ` [PATCH 1/3] notmuch-restore: fix return value propagation david
2012-12-17 3:59 ` [PATCH 2/3] tag-utils: use the tag_opt_list_t as talloc context, if possible david
@ 2012-12-17 3:59 ` david
2012-12-22 21:38 ` v2 restore leak fix Jani Nikula
3 siblings, 0 replies; 7+ messages in thread
From: david @ 2012-12-17 3:59 UTC (permalink / raw)
To: notmuch; +Cc: David Bremner
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 665373f..9ed9b51 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -125,6 +125,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;
@@ -208,10 +209,14 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
do {
char *query_string;
+ if (line_ctx != NULL)
+ talloc_free (line_ctx);
+
+ 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) {
@@ -244,6 +249,9 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
} while ((line_len = getline (&line, &line_size, input)) != -1);
+ if (line_ctx != NULL)
+ talloc_free (line_ctx);
+
if (input_format == DUMP_FORMAT_SUP)
regfree (®ex);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: v2 restore leak fix
2012-12-17 3:59 v2 restore leak fix david
` (2 preceding siblings ...)
2012-12-17 3:59 ` [PATCH 3/3] notmuch-restore: allocate a temporary talloc context for each line parsed david
@ 2012-12-22 21:38 ` Jani Nikula
2012-12-23 3:34 ` David Bremner
3 siblings, 1 reply; 7+ messages in thread
From: Jani Nikula @ 2012-12-22 21:38 UTC (permalink / raw)
To: david, notmuch
On Mon, 17 Dec 2012, david@tethera.net wrote:
> This obsoletes
>
> id:1355688997-19164-1-git-send-email-david@tethera.net
>
> Actually the first patch in the series is now an unrelated bug fix,
> since it isn't needed anymore for 2 and 3.
The series LGTM.
Jani.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: v2 restore leak fix
2012-12-22 21:38 ` v2 restore leak fix Jani Nikula
@ 2012-12-23 3:34 ` David Bremner
0 siblings, 0 replies; 7+ messages in thread
From: David Bremner @ 2012-12-23 3:34 UTC (permalink / raw)
To: Jani Nikula, notmuch
Jani Nikula <jani@nikula.org> writes:
> On Mon, 17 Dec 2012, david@tethera.net wrote:
>> This obsoletes
>>
>> id:1355688997-19164-1-git-send-email-david@tethera.net
>>
>> Actually the first patch in the series is now an unrelated bug fix,
>> since it isn't needed anymore for 2 and 3.
>
> The series LGTM.
>
> Jani.
Pushed, with a Tomi approved commit message for patch 2, and one small
fix to patch 1, which used line_ctx before it was defined in a later
patch.
d
^ permalink raw reply [flat|nested] 7+ messages in thread