bug#6936: Bug#594666: /usr/bin/tac: tac aborts

[Jim Meyering]

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 <address@hidden>
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