m.ge...@avm.de 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 <j...@meyering.net> 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
