[debbugs-tracker] bug#13098: closed ([PATCH] cut.c: Fix memory leak)

[GNU bug Tracking System]

Your message dated Thu, 06 Dec 2012 16:42:13 +0000
with message-id <address@hidden>
and subject line Re: bug#13098: [PATCH] cut.c: Fix memory leak
has caused the debbugs.gnu.org bug report #13098,
regarding [PATCH] cut.c: Fix memory leak
to be marked as done.

(If you believe you have received this mail in error, please contact
address@hidden)


-- 
13098: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13098
GNU Bug Tracking System
Contact address@hidden with problems
> --- Begin Message ---
>
>
>
> Subject: 
>
> [PATCH] cut.c: Fix memory leak
>
>
>
>
> Date: 
>
> Thu, 6 Dec 2012 03:11:48 +0100
>
>
>
>
> >From 82f2b062c0e21d9a0d64f9ceab363d2a79f5a6eb Mon Sep 17 00:00:00 2001
> From: Cojocaru Alexandru <address@hidden>
> Date: Thu, 6 Dec 2012 03:03:41 +0100
> Subject: [PATCH] cut: fix memory leak
>
> * src/cut.c (set_fields): don't allocate memory for
> `printable_field' if there are no finite ranges.
> The bug was introduced on 2012-2-7 via commit 2e636af.
> ---
>  src/cut.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/src/cut.c b/src/cut.c
> index 4219d24..87abd15 100644
> --- a/src/cut.c
> +++ b/src/cut.c
> @@ -500,14 +500,13 @@ set_fields (const char *fieldstr)
>        if (rp[i].hi > max_range_endpoint)
>          max_range_endpoint = rp[i].hi;
>      }
> -  if (max_range_endpoint < eol_range_start)
> -    max_range_endpoint = eol_range_start;
>  
>    /* Allocate an array large enough so that it may be indexed by
>       the field numbers corresponding to all finite ranges
>       (i.e. '2-6' or '-4', but not '5-') in FIELDSTR.  */
>  
> -  printable_field = xzalloc (max_range_endpoint / CHAR_BIT + 1);
> +  if (n_rp)
> +    printable_field = xzalloc (max_range_endpoint / CHAR_BIT + 1);
>  
>    qsort (rp, n_rp, sizeof (rp[0]), compare_ranges);
>  
> @@ -531,8 +530,11 @@ set_fields (const char *fieldstr)
>  
>    if (output_delimiter_specified
>        && !complement
> -      && eol_range_start && !is_printable_field (eol_range_start))
> -    mark_range_start (eol_range_start);
> +      && eol_range_start
> +      && printable_field && !is_printable_field (eol_range_start))
> +    {
> +      mark_range_start (eol_range_start);
> +    }
>  
>    free (rp);
>  
> -- 
> 1.8.0.1
>
>
>
> -- 
> Cojocaru Alexandru <address@hidden>
>
>
>
> --- End Message ---

--- Begin Message --- Subject: Re: bug#13098: [PATCH] cut.c: Fix memory leak Date: Thu, 06 Dec 2012 16:42:13 +0000 User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120615 Thunderbird/13.0.1

On 12/06/2012 02:11 AM, Cojocaru Alexandru wrote:

From 82f2b062c0e21d9a0d64f9ceab363d2a79f5a6eb Mon Sep 17 00:00:00 2001

From: Cojocaru Alexandru <address@hidden>
Date: Thu, 6 Dec 2012 03:03:41 +0100
Subject: [PATCH] cut: fix memory leak

* src/cut.c (set_fields): don't allocate memory for
`printable_field' if there are no finite ranges.
The bug was introduced on 2012-2-7 via commit 2e636af.
---
  src/cut.c | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/src/cut.c b/src/cut.c
index 4219d24..87abd15 100644
--- a/src/cut.c
+++ b/src/cut.c
@@ -500,14 +500,13 @@ set_fields (const char *fieldstr)
        if (rp[i].hi > max_range_endpoint)
          max_range_endpoint = rp[i].hi;
      }
-  if (max_range_endpoint < eol_range_start)
-    max_range_endpoint = eol_range_start;

    /* Allocate an array large enough so that it may be indexed by
       the field numbers corresponding to all finite ranges
       (i.e. '2-6' or '-4', but not '5-') in FIELDSTR.  */

-  printable_field = xzalloc (max_range_endpoint / CHAR_BIT + 1);
+  if (n_rp)
+    printable_field = xzalloc (max_range_endpoint / CHAR_BIT + 1);

    qsort (rp, n_rp, sizeof (rp[0]), compare_ranges);

@@ -531,8 +530,11 @@ set_fields (const char *fieldstr)

    if (output_delimiter_specified
        && !complement
-      && eol_range_start && !is_printable_field (eol_range_start))
-    mark_range_start (eol_range_start);
+      && eol_range_start
+      && printable_field && !is_printable_field (eol_range_start))
+    {
+      mark_range_start (eol_range_start);
+    }

    free (rp);

This looks right. subsequent accesses to the now not alloced bit array,
are avoided when max_range_endpoint = 0.

$ valgrind cut-before --output=" " -f1234567- /dev/null 2>&1 | grep reachable
==20949==    still reachable: 154,323 bytes in 2 blocks

$ valgrind cut-after --output=" " -f1234567- /dev/null 2>&1 | grep reachable
==20816==    still reachable: 2 bytes in 1 blocks

I wouldn't describe it as a leak though, rather a redundant allocation.
Since this is an edge case minor performance issue, I don't think
it needs a NEWS entry and will just adjust the commit summary a little
to say "advoid a redundant memory allocation".

Hmm, it might be a bit more consistent to guard all
references to the bit vector array with max_range_endpoint?
How about the attached?

thanks!
Pádraig.

cut-avoid-alloc.diff
Description: Text Data

--- End Message ---