GNU bug report logs - #6936
Bug#594666: /usr/bin/tac: tac aborts

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 6936 in the body.
You can then email your comments to 6936 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Salvo Tomaselli <tiposchi <at> tiscali.it>
Cc: 594666 <at> bugs.debian.org, bug-coreutils <at> gnu.org
Subject: Re: Bug#594666: /usr/bin/tac: tac aborts
Date: Sat, 28 Aug 2010 18:26:06 +0200

Salvo Tomaselli wrote:
> Package: coreutils
> Version: 8.5-1
> Severity: normal
> File: /usr/bin/tac
>
> Tac aborts when using it on a particular file.
>
> *** glibc detected *** tac: double free or corruption (top): 0x00000000025c5030 ***
...

Thank you for the report!
That is indeed a bug in the very latest.

For a stand-alone, minimal demonstrator, run this:

    valgrind tac <(printf %0$((2**14 + 1))d 0) > /dev/null

It prints this:

     Invalid free() / delete / delete[]
        at 0x4A04D72: free (vg_replace_malloc.c:325)
        by 0x402294: main (tac.c:669)
      Address 0x4c30040 is 0 bytes inside a block of size 16,388 free'd
        at 0x4A05255: realloc (vg_replace_malloc.c:476)
        by 0x4117B8: xrealloc (xmalloc.c:57)
        by 0x401A68: tac_seekable (tac.c:319)
        by 0x402379: main (tac.c:515)

Here is a fix:

From b3959fc691e606857a3c6e9b316ec34819972245 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Sat, 28 Aug 2010 17:45:29 +0200
Subject: [PATCH] tac: avoid double free

* src/tac.c (main): Reading a line longer than 16KiB would cause
tac to realloc its primary buffer.  Then, just before exit, tac
would mistakenly free the original (now free'd) buffer.
This bug was introduced by commit be6c13e7, "maint: always free a
buffer, to avoid even semblance of a leak".
* NEWS (Bug fixes): Mention it.
* tests/misc/tac (double-free): New test, to exercise this.
Reported by Salvo Tomaselli in <http://bugs.debian.org/594666>.
---
 NEWS           |    3 +++
 src/tac.c      |    6 ++++--
 tests/misc/tac |    6 ++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 85f55a2..f29d311 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   du -H and -L now consistently count pointed-to files instead of
   symbolic links, and correctly diagnose dangling symlinks.

+  tac would perform a double-free when given an input line longer than 16KiB.
+  [bug introduced in coreutils-8.3]
+
 ** New features

   cp now accepts the --attributes-only option to not copy file data,
diff --git a/src/tac.c b/src/tac.c
index cec9736..859e006 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -633,7 +633,6 @@ main (int argc, char **argv)
   if (! (read_size < half_buffer_size && half_buffer_size < G_buffer_size))
     xalloc_die ();
   G_buffer = xmalloc (G_buffer_size);
-  void *buf = G_buffer;
   if (sentinel_length)
     {
       strcpy (G_buffer, separator);
@@ -666,6 +665,9 @@ main (int argc, char **argv)
       error (0, errno, "-");
       ok = false;
     }
-  free (buf);
+
+  size_t offset = sentinel_length ? sentinel_length : 1;
+  free (G_buffer - offset);
+
   exit (ok ? EXIT_SUCCESS : EXIT_FAILURE);
 }
diff --git a/tests/misc/tac b/tests/misc/tac
index 7631049..4130c00 100755
--- a/tests/misc/tac
+++ b/tests/misc/tac
@@ -24,6 +24,9 @@ my $prog = 'tac';

 my $bad_dir = 'no/such/dir';

+# This must be longer than 16KiB to trigger the double free in coreutils-8.5.
+my $long_line = 'o' x (16 * 1024 + 1);
+
 my @Tests =
 (
   ['segfault', '-r', {IN=>"a\n"}, {IN=>"b\n"}, {OUT=>"a\nb\n"}],
@@ -67,6 +70,9 @@ my @Tests =
    {ERR_SUBST => "s,`$bad_dir': .*,...,"},
    {ERR => "$prog: cannot create temporary file in ...\n"},
    {EXIT => 1}],
+
+  # coreutils-8.5's tac would double-free its primary buffer.
+  ['double-free', {IN=>$long_line}, {OUT=>$long_line}],
 );

 @Tests = triple_test \@Tests;
--
1.7.2.2.510.g7180a

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-coreutils <at> gnu.org
Subject: Re: bug#6936: Bug#594666: /usr/bin/tac: tac aborts
Date: Sat, 28 Aug 2010 12:18:50 -0700

On 08/28/2010 09:26 AM, Jim Meyering wrote:

> This bug was introduced by commit be6c13e7, "maint: always free a
> buffer, to avoid even semblance of a leak".

Hah!  I've always been suspicious of these unnecessary free()s,
i.e., free()s that are put in only to make 'valgrind' happy.

How about if we do these unnecessary free()s only if 'lint'
is defined?  That would make the production software more
reliable.

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-coreutils <at> gnu.org
Subject: Re: bug#6936: Bug#594666: /usr/bin/tac: tac aborts
Date: Sat, 28 Aug 2010 21:59:52 +0200

Paul Eggert wrote:
> On 08/28/2010 09:26 AM, Jim Meyering wrote:
>
>> This bug was introduced by commit be6c13e7, "maint: always free a
>> buffer, to avoid even semblance of a leak".
>
> Hah!  I've always been suspicious of these unnecessary free()s,
> i.e., free()s that are put in only to make 'valgrind' happy.
>
> How about if we do these unnecessary free()s only if 'lint'
> is defined?  That would make the production software more
> reliable.

Hi Paul,

It's a close call, especially given this actual bug.
However, for me at least, this is similar to compiler warnings in that
freeing unconditionally makes for cleaner output from leak-checking
tools like valgrind, and thus, desirable.  In addition, guarding the
free inside #ifdef lint...#endif would expose the "possibly leaked"
condition to any unsuspecting developer who builds without -Dlint.
We've had a few reports like that: the solution is simply to tell such
folks to use -Dlint, but preventing reports altogether is even better.
Another point against such a change is that it'd add an in-function #ifdef.

On the other hand, there is precedent for this sort of a guard
in e.g., shuf.c and mktemp.c.  In addition, when configuring
with --enable-gcc-warnings, you automatically get -Dlint, so
maybe it makes sense.  I'm tempted to push this, but could
go either way:


From 9e837201ba6d8974de8e09c1d359c16ecab12584 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Sat, 28 Aug 2010 21:54:17 +0200
Subject: [PATCH] tac: suppress technically unneeded "free"

* src/tac.c (main): Guard final free with #ifdef lint.
Suggested by Paul Eggert.
---
 src/tac.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/tac.c b/src/tac.c
index 859e006..dbfe2b0 100644
--- a/src/tac.c
+++ b/src/tac.c
@@ -666,8 +666,10 @@ main (int argc, char **argv)
       ok = false;
     }

+#ifdef lint
   size_t offset = sentinel_length ? sentinel_length : 1;
   free (G_buffer - offset);
+#endif

   exit (ok ? EXIT_SUCCESS : EXIT_FAILURE);
 }
--
1.7.2.2.510.g7180a

Message #16 received at 6936-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: 6936-done <at> debbugs.gnu.org
Subject: Re: Bug#594666: /usr/bin/tac: tac aborts
Date: Sun, 07 Aug 2011 18:22:38 +0200

Jim Meyering wrote:
> Salvo Tomaselli wrote:
>> Package: coreutils
>> Version: 8.5-1
>> Severity: normal
>> File: /usr/bin/tac
>>
>> Tac aborts when using it on a particular file.
>>
>> *** glibc detected *** tac: double free or corruption (top):
>> 0x00000000025c5030 ***
> ...
>
> Thank you for the report!
> That is indeed a bug in the very latest.
>
> For a stand-alone, minimal demonstrator, run this:
>
>     valgrind tac <(printf %0$((2**14 + 1))d 0) > /dev/null
>
> It prints this:
>
>      Invalid free() / delete / delete[]
>         at 0x4A04D72: free (vg_replace_malloc.c:325)
>         by 0x402294: main (tac.c:669)
>       Address 0x4c30040 is 0 bytes inside a block of size 16,388 free'd
>         at 0x4A05255: realloc (vg_replace_malloc.c:476)
>         by 0x4117B8: xrealloc (xmalloc.c:57)
>         by 0x401A68: tac_seekable (tac.c:319)
>         by 0x402379: main (tac.c:515)
>
> Here is a fix:
>
>>From b3959fc691e606857a3c6e9b316ec34819972245 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering <at> redhat.com>
> Date: Sat, 28 Aug 2010 17:45:29 +0200
> Subject: [PATCH] tac: avoid double free
>
> * src/tac.c (main): Reading a line longer than 16KiB would cause
> tac to realloc its primary buffer.  Then, just before exit, tac
> would mistakenly free the original (now free'd) buffer.
> This bug was introduced by commit be6c13e7, "maint: always free a
> buffer, to avoid even semblance of a leak".
> * NEWS (Bug fixes): Mention it.
> * tests/misc/tac (double-free): New test, to exercise this.
> Reported by Salvo Tomaselli in <http://bugs.debian.org/594666>.

This was fixed long ago.
Marking as "done".

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.