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

[Cristian Cadar]

  Hi Jim, thanks for confirming the bug.  I don't use ptx myself, but
more importantly, I wanted to add that in our experience with testing
Coreutils, we found ptx to be one of the most complex utilities (for
example, it is the fourth in terms of lines of code in the front-end, so
more opportunities for bugs), and at the same time one of the most
poorly tested programs in the suite (at only 19.9% statement coverage as
of v6.10, in strong contrast with the other utilities -- the average
being at 67.5% in v6.10, see
http://keeda.stanford.edu/~cristic/coreutils-dev-tests/src/ for
details).
 
  Best,
  Cristian

On Wed, 2008-07-16 at 12:47 +0200, Jim Meyering wrote:
> 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