[PATCH] Release memory allocated by internet_address_list_parse_string()

[PATCH] Release memory allocated by internet_address_list_parse_string()

[Tomi Ollila]

g_object_unref() releases the memory of the InternetAddressList object
returned by internet_address_list_parse_string() -- when last (only)
reference is released, internet_address_list_finalize() will do cleanup.
---
When reviewing, see also gmime-2.4.25/gmime/internet-address.c (or older)
I tested this patch by entering:  notmuch show --format=mbox '*' | wc

Note that this fixes one potential memory leak only in case --format=mbox
as _extract_email_address is called only there.

 notmuch-show.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 603992a..3abfa07 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -255,7 +255,7 @@ _extract_email_address (const void *ctx, const char *from)
     email = talloc_strdup (ctx, email);
 
   DONE:
-    /* XXX: How to free addresses here? */
+    g_object_unref (addresses);
     return email;
    }
 
-- 
1.7.7.3

Re: [PATCH] Release memory allocated by internet_address_list_parse_string()

[Tomi Ollila]

On Wed, 07 Dec 2011 22:13:19 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> g_object_unref() releases the memory of the InternetAddressList object
> returned by internet_address_list_parse_string() -- when last (only)
> reference is released, internet_address_list_finalize() will do cleanup.
> ---
> When reviewing, see also gmime-2.4.25/gmime/internet-address.c (or older)
> I tested this patch by entering:  notmuch show --format=mbox '*' | wc
> 
> Note that this fixes one potential memory leak only in case --format=mbox
> as _extract_email_address is called only there.

There is at least one bug here -- I failed to check whether addresses
is non-NULL before attempting to g_object_unref (). So please ignore
that when reviewing whether g_object_unref() is the correct action here.

I provide a new patch when I know whether re-check or separate the
NULL test in the beginning of _extract_email_address().

>  notmuch-show.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 603992a..3abfa07 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -255,7 +255,7 @@ _extract_email_address (const void *ctx, const char *from)
>      email = talloc_strdup (ctx, email);
>  
>    DONE:
> -    /* XXX: How to free addresses here? */
> +    g_object_unref (addresses);
>      return email;
>     }
>  
> -- 
> 1.7.7.3

Tomi

[Patch 1/2] separate handling when addresses == NULL

[Tomi Ollila]

When addresses is NULL, (future) addresses object cleanup is not needed.
---
 notmuch-show.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 603992a..c27ef6a 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -239,7 +239,11 @@ _extract_email_address (const void *ctx, const char *from)
     addresses = internet_address_list_parse_string (from);
 
     /* Bail if there is no address here. */
-    if (addresses == NULL || internet_address_list_length (addresses) < 1)
+    if (addresses == NULL)
+	return email;
+
+    /* Bail if there is no address here. */
+    if (internet_address_list_length (addresses) < 1)
 	goto DONE;
 
     /* Otherwise, just use the first address. */
-- 
1.7.7.3

[PATCH 2/2] Release memory allocated by internet_address_list_parse_string()

[Tomi Ollila]

g_object_unref() releases the memory of the InternetAddressList object
returned by internet_address_list_parse_string() -- when last (only)
reference is released, internet_address_list_finalize() will do cleanup.
---
When reviewing, see also gmime-2.4.25/gmime/internet-address.c (or older)
I tested this patch by entering:  notmuch show --format=mbox '*' | wc

Note that this fixes one potential memory leak only in case --format=mbox
as _extract_email_address is called only there.

 notmuch-show.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index c27ef6a..db02891 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -259,7 +259,8 @@ _extract_email_address (const void *ctx, const char *from)
     email = talloc_strdup (ctx, email);
 
   DONE:
-    /* XXX: How to free addresses here? */
+    g_object_unref(addresses);
+
     return email;
    }
 
-- 
1.7.7.3

Re: [Patch 1/2] separate handling when addresses == NULL

[Austin Clements]

Quoth Tomi Ollila on Dec 09 at  3:52 pm:
> When addresses is NULL, (future) addresses object cleanup is not needed.
> ---
>  notmuch-show.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 603992a..c27ef6a 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -239,7 +239,11 @@ _extract_email_address (const void *ctx, const char *from)
>      addresses = internet_address_list_parse_string (from);
>  
>      /* Bail if there is no address here. */
> -    if (addresses == NULL || internet_address_list_length (addresses) < 1)
> +    if (addresses == NULL)
> +	return email;
> +
> +    /* Bail if there is no address here. */
> +    if (internet_address_list_length (addresses) < 1)
>  	goto DONE;

Personally, I would much prefer to see the code as it was---with the
sometimes unnecessary goto DONE---and an if (addresses) around the
later cleanup that the second patch adds.  It's more maintainable in
case someone ever adds more code earlier in this function and this
microoptimization isn't buying you anything.

>  
>      /* Otherwise, just use the first address. */

[PATCH v3] Release memory allocated by internet_address_list_parse_string()

[Tomi Ollila]

g_object_unref() releases the memory of the InternetAddressList object
returned by internet_address_list_parse_string() -- when last (only)
reference is released, internet_address_list_finalize() will do cleanup.
---

hmm, have to agree amdragon's "suggested alternative" is more robust 
(in maintainability point of view) than the one I took previously.

The 3 other alternatives are not common in notmuch code and therefore dropped:
1) use multiple goto labels to jump in
2) use deeply nested if blocks
3) the ultimate solution: do { .. } while (0) blocks and break;s >;)

When reviewing, see also gmime-2.4.25/gmime/internet-address.c (or older)
I tested this patch by entering:  notmuch show --format=mbox '*' | wc

 notmuch-show.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index 603992a..873a7c4 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -255,7 +255,9 @@ _extract_email_address (const void *ctx, const char *from)
     email = talloc_strdup (ctx, email);
 
   DONE:
-    /* XXX: How to free addresses here? */
+    if (addresses)
+	g_object_unref (addresses);
+
     return email;
    }
 
-- 
1.7.7.3

Re: [PATCH v3] Release memory allocated by internet_address_list_parse_string()

[David Bremner]

On Sat, 10 Dec 2011 12:18:54 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> g_object_unref() releases the memory of the InternetAddressList object
> returned by internet_address_list_parse_string() -- when last (only)
> reference is released, internet_address_list_finalize() will do cleanup.

Pushed this version.

d