if you use ptx, please let us know [Re: ptx bug (invalid read)

[Jim Meyering]

Cristian Cadar <address@hidden> wrote:
>    Hello, I found an older bug report generated by our tool for ptx,
> which I forgot to report.  The bug is still present in the current
> version of Coreutils (6.12).  I did not have time to investigate the
> root cause of the bug, but I'm including a very simple test case and the
> output reported by valgrind:
>
> $ echo -n a>A && valgrind ptx x A
> ==9357== Memcheck, a memory error detector.
> ==9357== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
> ...
> ==9357== Invalid read of size 1
> ==9357==    at 0x804B0F5: define_all_fields (ptx.c:1516)
> ==9357==    by 0x804BF64: generate_all_output (ptx.c:1846)
> ==9357==    by 0x804C9D9: main (ptx.c:2218)
> ==9357==  Address 0x4022391 is 0 bytes after a block of size 1 alloc'd
> ==9357==    at 0x40054E5: malloc (vg_replace_malloc.c:149)
> ==9357==    by 0x804F4C5: xmalloc (xmalloc.c:49)
> ==9357==    by 0x804984C: swallow_file_in_memory (ptx.c:547)
> ==9357==    by 0x804C994: main (ptx.c:2203)

Thanks for finding and reporting that!
Looking into it, I found two shallow bugs (patched below)
and added some basic tests.
With this patch, the problem you found is still reproducible,
but requires a slightly different invocation:

  echo -n a>A && valgrind ptx A A

The trouble is that global state is reused
when processing the second and subsequent
file argument.  Fixing that will require a more
invasive change, and I'm not convinced it's even
worth my time.  Does anyone even use ptx these day?
Does anyone rely on the GNU extension of accepting more
than one input file?  Since it appears that feature has never
worked properly, I seriously doubt it.  So I'm considering
whether to add ptx to the list of programs that are not
installed by default.

Jim

>From 773be9eca85da9a9a33d42d29ecfd04c9aec5c3f Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 15 Jul 2008 08:30:38 +0200
Subject: [PATCH] fix two bugs in ptx

* src/ptx.c (fix_output_parameters): Don't let before_max_width
go negative -- that would cause an infloop in define_all_fields.
(main): Don't clobber name[0] with lists of two or more input files.
* tests/misc/ptx: New file.  Test for the above.
* tests/Makefile.am (TESTS): Add misc/ptx.
---
 src/ptx.c         |    7 ++++---
 tests/Makefile.am |    1 +
 tests/misc/ptx    |   44 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 3 deletions(-)
 create mode 100755 tests/misc/ptx

diff --git a/src/ptx.c b/src/ptx.c
index 827d22e..c04c90c 100644
--- a/src/ptx.c
+++ b/src/ptx.c
@@ -1353,6 +1353,8 @@ fix_output_parameters (void)
         right side, or one on either side.  */

       before_max_width -= 2 * truncation_string_length;
+      if (before_max_width < 0)
+       before_max_width = 0;
       keyafter_max_width -= 2 * truncation_string_length;
     }
   else
@@ -2112,11 +2114,10 @@ main (int argc, char **argv)

       for (file_index = 0; file_index < number_input_files; file_index++)
        {
-         input_file_name[file_index] = argv[optind];
          if (!*argv[optind] || STREQ (argv[optind], "-"))
-           input_file_name[0] = NULL;
+           input_file_name[file_index] = NULL;
          else
-           input_file_name[0] = argv[optind];
+           input_file_name[file_index] = argv[optind];
          optind++;
        }
     }
diff --git a/tests/Makefile.am b/tests/Makefile.am
index f7275f8..c2da630 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -120,6 +120,7 @@ TESTS =                                             \
   chgrp/no-x                                   \
   chgrp/posix-H                                        \
   chgrp/recurse                                        \
+  misc/ptx                                     \
   misc/test                                    \
   misc/seq                                     \
   misc/head                                    \
diff --git a/tests/misc/ptx b/tests/misc/ptx
new file mode 100755
index 0000000..cfc1544
--- /dev/null
+++ b/tests/misc/ptx
@@ -0,0 +1,44 @@
+#!/usr/bin/perl
+
+# Copyright (C) 2008 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+use strict;
+
+my $prog = 'ptx';
+
+# Turn off localization of executable's output.
address@hidden(LANGUAGE LANG LC_ALL)} = ('C') x 3;
+
+my @Tests =
+(
+["1tok", '-w10', {IN=>"bar\n"},     {OUT=>"        bar\n"}],
+["2tok", '-w10', {IN=>"foo bar\n"}, {OUT=>"     /   bar\n        foo/\n"}],
+
+# with coreutils-6.12 and earlier, this would infloop with -wN, N < 10
+["narrow", '-w2', {IN=>"qux\n"},    {OUT=>"      qux\n"}],
+["narrow-g", '-g1 -w2', {IN=>"ta\n"}, {OUT=>"  ta\n"}],
+
+# with coreutils-6.12 and earlier, this would act like "ptx F1 F1"
+["2files", '-g1 -w1', {IN=>{F1=>"a"}}, {IN=>{F2=>"b"}}, {OUT=>"  a\n  b\n"}],
+);
+
address@hidden = triple_test address@hidden;
+
+my $save_temps = $ENV{DEBUG};
+my $verbose = $ENV{VERBOSE};
+
+my $fail = run_tests ($prog, $prog, address@hidden, $save_temps, $verbose);
+exit $fail;
--
1.5.6.3.386.gc8666