Subject: bug#13098: closed (Re: bug#13098: [PATCH] cut.c: Fix memory leak)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 06 Dec 2012 16:43:04 +0000

Your bug report

#13098: [PATCH] cut.c: Fix memory leak

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
13098: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13098
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#13098: [PATCH] cut.c: Fix memory leak
From: Pádraig Brady <[email-masked]>
To: Cojocaru Alexandru <[email-masked]>
Cc: [email-masked]
Date: Thu, 06 Dec 2012 16:42:13 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

On 12/06/2012 02:11 AM, Cojocaru Alexandru wrote:
>>>From 82f2b062c0e21d9a0d64f9ceab363d2a79f5a6eb Mon Sep 17 00:00:00 2001
> From: Cojocaru Alexandru <[email-masked]>
> 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.

>From c40543afe9fd5a06fac60f47ae92775303f83b49 Mon Sep 17 00:00:00 2001
From: Cojocaru Alexandru <[email-masked]>
Date: Thu, 6 Dec 2012 03:03:41 +0100
Subject: [PATCH] cut: avoid a redundant heap allocation

* src/cut.c (set_fields): Don't allocate memory for
`printable_field' if there are no finite ranges.
The extra allocation was introduced via commit v8.10-3-g2e636af.
---
 src/cut.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/cut.c b/src/cut.c
index dc83072..de9320c 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 (max_range_endpoint)
+    printable_field = xzalloc (max_range_endpoint / CHAR_BIT + 1);
 
   qsort (rp, n_rp, sizeof (rp[0]), compare_ranges);
 
@@ -531,7 +530,8 @@ set_fields (const char *fieldstr)
 
   if (output_delimiter_specified
       && !complement
-      && eol_range_start && !is_printable_field (eol_range_start))
+      && eol_range_start
+      && max_range_endpoint && !is_printable_field (eol_range_start))
     mark_range_start (eol_range_start);
 
   free (rp);
-- 
1.7.6.4

Subject: [PATCH] cut.c: Fix memory leak
From: Cojocaru Alexandru <[email-masked]>
To: [email-masked]
Date: Thu, 6 Dec 2012 03:11:48 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

>From 82f2b062c0e21d9a0d64f9ceab363d2a79f5a6eb Mon Sep 17 00:00:00 2001
From: Cojocaru Alexandru <[email-masked]>
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 <[email-masked]>