bug#12656: WG: Re[4]: bug#12656: cp since 8.11 corrupts files

[Jim Meyering]

address@hidden wrote:
> different cp version produce different corruptions:
>
>         8.11: unpatched: 14 corrupted files;  patched == OK
>         8.19: unpatched:  3 corrupted files;  patched == OK

Thanks for the thorough testing.
Here's a complete patch:

Changes:
  - added a comment to the code
  - added NEWS
  - added a test case

[testing note:
 I would have preferred to create a test case that made the pre-patch
 cp produce a corrupted result rather than just a valgrind-detected FMR,
 but that would have required being able to create an input file with
 multiple adjacent extents, and at least with ext4, I don't know off hand
 how to do that (without resorting to creating/using a nearly full
 partition?), and prefer to keep this a user-runnable (not root-only) test.
 Suggestions welcome.  ]


>From a5365003c88f4fce6293827c13f90acd0b5bd0cc Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 16 Oct 2012 17:43:49 +0200
Subject: [PATCH] cp: avoid data-corrupting free-memory-read

* src/extent-scan.c (extent_scan_read): Reset our last_ei
pointer whenever the parent buffer might have just been freed.
* tests/cp/fiemap-extent-FMR.sh: New test.
* tests/local.mk (all_tests): Add it.
* NEWS (Bug fixes): Mention it.
Reported by Mike Gerth in http://bugs.gnu.org/12656, and with
help from Alan Curry.  Bug introduced in commit v8.10-60-g18f5a85.
---
 NEWS                   |  4 ++++
 src/extent-scan.c      | 12 +++++++++---
 tests/cp/fiemap-FMR.sh | 31 +++++++++++++++++++++++++++++++
 tests/local.mk         |  1 +
 4 files changed, 45 insertions(+), 3 deletions(-)
 create mode 100755 tests/cp/fiemap-FMR.sh

diff --git a/NEWS b/NEWS
index aff5bf1..46ce698 100644
--- a/NEWS
+++ b/NEWS
@@ -12,6 +12,10 @@ GNU coreutils NEWS                                    -*- 
outline -*-

 ** Bug fixes

+  cp could read from freed memory and could even make corrupt copies.
+  This could happen only with a very fragmented input file and when using
+  its FIEMAP/extent-based copying code.  [bug introduced in coreutils-8.11]
+
   cp --no-preserve=mode now no longer preserves the original file's
   permissions but correctly sets mode specified by 0666 & ~umask

diff --git a/src/extent-scan.c b/src/extent-scan.c
index 0c25c57..f962298 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -89,7 +89,7 @@ extern bool
 extent_scan_read (struct extent_scan *scan)
 {
   unsigned int si = 0;
-  struct extent_info *last_ei IF_LINT ( = scan->ext_info);
+  struct extent_info *last_ei = scan->ext_info;

   while (true)
     {
@@ -127,8 +127,14 @@ extent_scan_read (struct extent_scan *scan)

       assert (scan->ei_count <= SIZE_MAX - fiemap->fm_mapped_extents);
       scan->ei_count += fiemap->fm_mapped_extents;
-      scan->ext_info = xnrealloc (scan->ext_info, scan->ei_count,
-                                  sizeof (struct extent_info));
+      {
+        /* last_ei points into a buffer that may be freed via xnrealloc.
+           Record its offset and adjust after allocation.  */
+        size_t prev_idx = last_ei - scan->ext_info;
+        scan->ext_info = xnrealloc (scan->ext_info, scan->ei_count,
+                                    sizeof (struct extent_info));
+        last_ei = scan->ext_info + prev_idx;
+      }

       unsigned int i = 0;
       for (i = 0; i < fiemap->fm_mapped_extents; i++)
diff --git a/tests/cp/fiemap-FMR.sh b/tests/cp/fiemap-FMR.sh
new file mode 100755
index 0000000..10c9e05
--- /dev/null
+++ b/tests/cp/fiemap-FMR.sh
@@ -0,0 +1,31 @@
+#!/bin/sh
+# Trigger a free-memory read bug in cp from coreutils-[8.11..8.19]
+
+# Copyright (C) 2012 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/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ cp
+
+require_valgrind_
+require_perl_
+: ${PERL=perl}
+
+$PERL -e 'for (1..600) { sysseek (*STDOUT, 4096, 1)' \
+  -e '&& syswrite (*STDOUT, "a" x 1024) or die "$!"}' > j || fail=1
+valgrind --quiet --error-exitcode=3 cp j j2 || fail=1
+cmp j j2 || fail=1
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index 486bf31..5a237fa 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -418,6 +418,7 @@ all_tests =                                 \
   tests/cp/existing-perm-race.sh               \
   tests/cp/fail-perm.sh                                \
   tests/cp/fiemap-empty.sh                     \
+  tests/cp/fiemap-FMR.sh                       \
   tests/cp/fiemap-perf.sh                      \
   tests/cp/fiemap-2.sh                         \
   tests/cp/file-perm-race.sh                   \
--
1.8.0.rc2.11.gd25c58c