Subject: Re: bug#25023: Bug PR utility with -S option
From: Pádraig Brady <[email-masked]>
To: Marcel Böhme <[email-masked]>, [email-masked]
Date: Fri, 25 Nov 2016 14:10:21 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

On 25/11/16 02:36, Marcel Böhme wrote:
> Dear all,
> 
> The following input to PR does not crash the program but ASAN reports a buffer overflow.
> The bug was found with AFLFast, a fork of AFL. Thanks also to Van-Thuan Pham.
> 
> $ echo a > a
> $ pr "-S$(printf "\t\t\t")" a -m a > /dev/null
> 
> =================================================================
> ==102438==ERROR: AddressSanitizer: global-buffer-overflow on address 0x00000041b622 at pc 0x00000040506b bp 0x7ffc95917160 sp 0x7ffc95917158
> READ of size 1 at 0x00000041b622 thread T0
>     #0 0x40506a in print_sep_string ../src/pr.c:2241
>     #1 0x407ec4 in read_line ../src/pr.c:2493
>     #2 0x40985c in print_page ../src/pr.c:1802
>     #3 0x40985c in print_files ../src/pr.c:1618
>     #4 0x4036e0 in main ../src/pr.c:1136
>     #5 0x7ff29fa67f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
>     #6 0x404209  (/home/ubuntu/subjects/coreutils_fixed/obj-asan/src/pr+0x404209)
> 
> 0x00000041b622 is located 62 bytes to the left of global variable '*.LC12' defined in '../src/pr.c' (0x41b660) of size 4
>   '*.LC12' is ascii string '%*d'
> 0x00000041b622 is located 0 bytes to the right of global variable '*.LC11' defined in '../src/pr.c' (0x41b620) of size 2
>   '*.LC11' is ascii string ' '
> SUMMARY: AddressSanitizer: global-buffer-overflow ../src/pr.c:2241 in print_sep_string

Fixed in that attached.

thanks!

From d91aeef0527bf8ec0f83c3c3b69f3979c0b4c4a0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Fri, 25 Nov 2016 13:46:23 +0000
Subject: [PATCH] pr: fix read from invalid memory with tabs in separator

This was detected with:
  echo a > a; pr "-S$(printf "\t\t\t")" a -m a > /dev/null
Resulting in ASAN triggering:
  ====================================================
  ERROR: AddressSanitizer: global-buffer-overflow
  READ of size 1 at 0x00000041b622 thread T0
    #0 0x40506a in print_sep_string ../src/pr.c:2241
    #1 0x407ec4 in read_line ../src/pr.c:2493
    #2 0x40985c in print_page ../src/pr.c:1802
    #3 0x40985c in print_files ../src/pr.c:1618
    #4 0x4036e0 in main ../src/pr.c:1136

* src/pr.c (init_parameters): Ensure we only override the
specified separator when it's a single tab, thus matching
the calculated separator length.
* tests/pr/pr-tests.pl: Add a test case.
* NEWS: Mention the fix.
---
 NEWS                 | 4 ++++
 src/pr.c             | 2 +-
 tests/pr/pr-tests.pl | 7 +++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index edfbdfa..ba7679f 100644
--- a/NEWS
+++ b/NEWS
@@ -35,6 +35,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   nl now resets numbering for each page section rather than just for each page.
   [This bug was present in "the beginning".]
 
+  pr now handles specified separator strings containing tabs correctly.
+  Previously it would have output random data from memory.
+  [This bug was detected with ASAN and present in "the beginning".]
+
   sort -h -k now works even in locales that use blank as thousands separator.
 
   stty --help no longer outputs extraneous gettext header lines
diff --git a/src/pr.c b/src/pr.c
index 20e8637..26f221f 100644
--- a/src/pr.c
+++ b/src/pr.c
@@ -1233,7 +1233,7 @@ init_parameters (int number_of_files)
         }
       /* It's rather pointless to define a TAB separator with column
          alignment */
-      else if (!join_lines && *col_sep_string == '\t')
+      else if (!join_lines && col_sep_length == 1 && *col_sep_string == '\t')
         col_sep_string = column_separator;
 
       truncate_lines = true;
diff --git a/tests/pr/pr-tests.pl b/tests/pr/pr-tests.pl
index 4d85dc9..ec3980a 100755
--- a/tests/pr/pr-tests.pl
+++ b/tests/pr/pr-tests.pl
@@ -467,6 +467,13 @@ push @Tests,
     {IN=>{3=>"x\ty\tz\n"}},
      {OUT=>join("\t", qw(a b c m n o x y z)) . "\n"} ];
 
+# This resulted in reading invalid memory before coreutils-8.26
+push @Tests,
+   ['asan1', "-m -S'\t\t\t' -t",
+    {IN=>{1=>"a\n"}},
+    {IN=>{2=>"a\n"}},
+     {OUT=>"a\t\t\t\t  \t\t\ta\n"} ];
+
 @Tests = triple_test \@Tests;
 
 my $save_temps = $ENV{DEBUG};
-- 
2.5.5