Bug 1499781
| Summary: | thin_repair/cache_repair shouldn't corrupt a meta device if given a non existent file to restore from | |||
|---|---|---|---|---|
| Product: | Red Hat Enterprise Linux 7 | Reporter: | Jakub Krysl <jkrysl> | |
| Component: | device-mapper-persistent-data | Assignee: | Joe Thornber <thornber> | |
| Status: | CLOSED ERRATA | QA Contact: | Lin Li <lilin> | |
| Severity: | medium | Docs Contact: | ||
| Priority: | unspecified | |||
| Version: | 7.5 | CC: | agk, heinzm, jbrassow, lvm-team, mcsontos, msnitzer, thornber | |
| Target Milestone: | rc | |||
| Target Release: | --- | |||
| Hardware: | Unspecified | |||
| OS: | Unspecified | |||
| Whiteboard: | ||||
| Fixed In Version: | device-mapper-persistent-data-0.8.5-3.el7 | Doc Type: | If docs needed, set a value | |
| Doc Text: | Story Points: | --- | ||
| Clone Of: | ||||
| : | 1774923 (view as bug list) | Environment: | ||
| Last Closed: | 2020-09-29 20:23:15 UTC | Type: | Bug | |
| Regression: | --- | Mount Type: | --- | |
| Documentation: | --- | CRM: | ||
| Verified Versions: | Category: | --- | ||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | ||
| Cloudforms Team: | --- | Target Upstream Version: | ||
| Embargoed: | ||||
| Bug Depends On: | ||||
| Bug Blocks: | 1774923 | |||
|
Description
Jakub Krysl
2017-10-09 11:46:02 UTC
Mass migration to lilin. The following patch has been pushed upstream:
https://github.com/jthornber/thin-provisioning-tools/commit/1dd7b454bbfa2b4edfd86abd7fdcc234ad9551eb
and will be in the upcoming 0.8.6 release.
Corrupting metadata should be taken seriously, asking for Exception. This is an incorrect use, and the tool should prevent incorrect use where possible. (In reply to Marian Csontos from comment #4) > Corrupting metadata should be taken seriously, asking for Exception. This is > an incorrect use, and the tool should prevent incorrect use where possible. No metadata is being corrupted. thin_restore/repair always write to a fresh metadata area. This patch just zeroes the newly written superblock if the restore fails so people wont try and use it by accident (ie. ignoring the exit code from the restore). Not severe enough to ask for an exception, going to push to 7.9. 1,specfile: # BZ 1499781: patch3: dmpd-thin_repair-cache_repair-Check-input-file-exists-earlier.patch 2,source code: cat dmpd-thin_repair-cache_repair-Check-input-file-exists-earlier.patch base/file_utils.cc | 4 ++-- base/file_utils.h | 2 +- caching/cache_repair.cc | 8 +++++++- functional-tests/cache-functional-tests.scm | 32 ++++++++++++++++++++++++++--- functional-tests/era-functional-tests.scm | 4 ++-- functional-tests/functional-tests.scm | 4 ++-- functional-tests/thin-functional-tests.scm | 27 ++++++++++++++++++++++-- thin-provisioning/thin_repair.cc | 8 +++++++- 8 files changed, 75 insertions(+), 14 deletions(-) diff --git a/base/file_utils.cc b/base/file_utils.cc index ba8957c..7883cfe 100644 --- a/base/file_utils.cc +++ b/base/file_utils.cc @@ -66,13 +66,13 @@ file_utils::file_exists(string const &path) { } void -file_utils::check_file_exists(string const &file) { +file_utils::check_file_exists(string const &file, bool must_be_regular_file) { struct stat info; int r = ::stat(file.c_str(), &info); if (r) throw runtime_error("Couldn't stat file"); - if (!S_ISREG(info.st_mode)) + if (must_be_regular_file && !S_ISREG(info.st_mode)) throw runtime_error("Not a regular file"); } diff --git a/base/file_utils.h b/base/file_utils.h index 2ee20ab..3edcc9e 100644 --- a/base/file_utils.h +++ b/base/file_utils.h @@ -10,7 +10,7 @@ namespace file_utils { int open_file(std::string const &path, int flags); bool file_exists(std::string const &path); - void check_file_exists(std::string const &file); + void check_file_exists(std::string const &file, bool must_be_regular_file = true); int create_block_file(std::string const &path, off_t file_size); int open_block_file(std::string const &path, off_t min_size, bool writeable, bool excl = true); uint64_t get_file_length(std::string const &file); diff --git a/caching/cache_repair.cc b/caching/cache_repair.cc index 9587d5f..8a837a8 100644 --- a/caching/cache_repair.cc +++ b/caching/cache_repair.cc @@ -2,6 +2,7 @@ #include <getopt.h> #include <libgen.h> +#include "base/file_utils.h" #include "base/output_file_requirements.h" #include "caching/commands.h" #include "caching/metadata.h" @@ -29,12 +30,17 @@ namespace { } int repair(string const &old_path, string const &new_path) { + bool metadata_touched = false; try { + file_utils::check_file_exists(new_path, false); + metadata_touched = true; metadata_dump(open_metadata_for_read(old_path), output_emitter(new_path), true); } catch (std::exception &e) { + if (metadata_touched) + file_utils::zero_superblock(new_path); cerr << e.what() << endl; return 1; } @@ -110,7 +116,7 @@ cache_repair_cmd::run(int argc, char **argv) check_output_file_requirements(*output_path); else { - cerr << "no output file provided" << endl; + cerr << "No output file provided." << endl; usage(cerr); return 1; } diff --git a/functional-tests/cache-functional-tests.scm b/functional-tests/cache-functional-tests.scm index 0e70b94..9b3a203 100644 --- a/functional-tests/cache-functional-tests.scm +++ b/functional-tests/cache-functional-tests.scm @@ -15,6 +15,7 @@ (define-tool cache-dump) (define-tool cache-restore) (define-tool cache-metadata-size) + (define-tool cache-repair) (define-syntax with-cache-xml (syntax-rules () @@ -35,7 +36,8 @@ (syntax-rules () ((_ (md) b1 b2 ...) (with-temp-file-sized ((md "cache.bin" (to-bytes (meg 4)))) - b1 b2 ...)))) + (system (fmt #f "dd if=/usr/bin/ls of=" md " bs=4096 > /dev/null 2>&1")) + b1 b2 ...)))) (define-syntax with-empty-metadata (syntax-rules () @@ -180,7 +182,7 @@ "the input file can't be found" (with-empty-metadata (md) (run-fail-rcv (_ stderr) (cache-restore "-i no-such-file -o" md) - (assert-superblock-untouched md) + (assert-superblock-all-zeroes md) (assert-starts-with "Couldn't stat file" stderr)))) (define-scenario (cache-restore garbage-input-file) @@ -188,7 +190,7 @@ (with-empty-metadata (md) (with-temp-file-sized ((xml "cache.xml" 4096)) (run-fail-rcv (_ stderr) (cache-restore "-i" xml "-o" md) - (assert-superblock-untouched md))))) + (assert-superblock-all-zeroes md))))) (define-scenario (cache-restore missing-output-file) "the output file can't be found" @@ -354,4 +356,28 @@ (run-ok-rcv (stdout stderr) (cache-metadata-size "--nr-blocks 67108864") (assert-equal "3678208 sectors" stdout) (assert-eof stderr))) + + ;;;----------------------------------------------------------- + ;;; cache_repair scenarios + ;;;----------------------------------------------------------- + (define-scenario (cache-repair missing-input-file) + "the input file can't be found" + (with-empty-metadata (md) + (run-fail-rcv (_ stderr) (cache-repair "-i no-such-file -o" md) + (assert-superblock-all-zeroes md) + (assert-starts-with "Couldn't stat path" stderr)))) + + (define-scenario (cache-repair garbage-input-file) + "the input file is just zeroes" + (with-empty-metadata (md1) + (with-corrupt-metadata (md2) + (run-fail-rcv (_ stderr) (cache-repair "-i" md1 "-o" md2) + (assert-superblock-all-zeroes md2))))) + + (define-scenario (cache-repair missing-output-file) + "the output file can't be found" + (with-cache-xml (xml) + (run-fail-rcv (_ stderr) (cache-repair "-i" xml) + (assert-starts-with "No output file provided." stderr)))) + ) diff --git a/functional-tests/era-functional-tests.scm b/functional-tests/era-functional-tests.scm index 373fe34..890f0ff 100644 --- a/functional-tests/era-functional-tests.scm +++ b/functional-tests/era-functional-tests.scm @@ -153,7 +153,7 @@ "the input file can't be found" (with-empty-metadata (md) (run-fail-rcv (_ stderr) (era-restore "-i no-such-file -o" md) - (assert-superblock-untouched md) + (assert-superblock-all-zeroes md) (assert-starts-with "Couldn't stat file" stderr)))) (define-scenario (era-restore garbage-input-file) @@ -161,7 +161,7 @@ (with-empty-metadata (md) (with-temp-file-sized ((xml "era.xml" 4096)) (run-fail-rcv (_ stderr) (era-restore "-i " xml "-o" md) - (assert-superblock-untouched md))))) + (assert-superblock-all-zeroes md))))) (define-scenario (era-restore output-unspecified) "Fails if no metadata dev specified" diff --git a/functional-tests/functional-tests.scm b/functional-tests/functional-tests.scm index aa5b95c..758498e 100644 --- a/functional-tests/functional-tests.scm +++ b/functional-tests/functional-tests.scm @@ -21,7 +21,7 @@ assert-eof assert-starts-with assert-matches - assert-superblock-untouched + assert-superblock-all-zeroes assert-member? assert-raises-thunk assert-raises @@ -259,7 +259,7 @@ (loop (+ i 1)) #f))))) - (define (assert-superblock-untouched md) + (define (assert-superblock-all-zeroes md) (with-bcache (cache md 1) (with-block (b cache 0 (get-flags)) (unless (all-zeroes? (block-data b) 4096) diff --git a/functional-tests/thin-functional-tests.scm b/functional-tests/thin-functional-tests.scm index d096b78..6bb7d24 100644 --- a/functional-tests/thin-functional-tests.scm +++ b/functional-tests/thin-functional-tests.scm @@ -37,10 +37,12 @@ b1 b2 ...))))) ;;; It would be nice if the metadata was at least similar to valid data. + ;;; Here I'm just using the start of the ls binary as 'random' data. (define-syntax with-corrupt-metadata (syntax-rules () ((_ (md) b1 b2 ...) (with-temp-file-sized ((md "thin.bin" (meg 4))) + (system (fmt #f "dd if=/usr/bin/ls of=" md " bs=4096 > /dev/null 2>&1")) b1 b2 ...)))) (define-syntax with-empty-metadata @@ -167,7 +169,7 @@ "the input file can't be found" (with-empty-metadata (md) (run-fail-rcv (_ stderr) (thin-restore "-i no-such-file -o" md) - (assert-superblock-untouched md) + (assert-superblock-all-zeroes md) (assert-starts-with "Couldn't stat file" stderr)))) (define-scenario (thin-restore garbage-input-file) @@ -175,7 +177,7 @@ (with-empty-metadata (md) (with-temp-file-sized ((xml "thin.xml" 4096)) (run-fail-rcv (_ stderr) (thin-restore "-i " xml "-o" md) - (assert-superblock-untouched md))))) + (assert-superblock-all-zeroes md))))) (define-scenario (thin-restore missing-output-file) "the output file can't be found" @@ -333,4 +335,25 @@ (with-empty-metadata (md) (run-fail-rcv (_ stderr) (thin-repair "-i" xml "-o" md) #t)))) + + (define-scenario (thin-repair missing-input-file) + "the input file can't be found" + (with-empty-metadata (md) + (run-fail-rcv (_ stderr) (thin-repair "-i no-such-file -o" md) + (assert-superblock-all-zeroes md) + (assert-starts-with "Couldn't stat file" stderr)))) + + (define-scenario (thin-repair garbage-input-file) + "the input file is just zeroes" + (with-empty-metadata (md1) + (with-corrupt-metadata (md2) + (run-fail-rcv (_ stderr) (thin-repair "-i " md1 "-o" md2) + (assert-superblock-all-zeroes md2))))) + + (define-scenario (thin-repair missing-output-file) + "the output file can't be found" + (with-thin-xml (xml) + (run-fail-rcv (_ stderr) (thin-repair "-i " xml) + (assert-starts-with "No output file provided." stderr)))) + ) diff --git a/thin-provisioning/thin_repair.cc b/thin-provisioning/thin_repair.cc index e22b193..0673753 100644 --- a/thin-provisioning/thin_repair.cc +++ b/thin-provisioning/thin_repair.cc @@ -2,6 +2,7 @@ #include <getopt.h> #include <libgen.h> +#include "base/file_utils.h" #include "base/output_file_requirements.h" #include "persistent-data/file_utils.h" #include "thin-provisioning/commands.h" @@ -17,15 +18,20 @@ using namespace thin_provisioning; namespace { int repair(string const &old_path, string const &new_path) { + bool metadata_touched = false; try { // block size gets updated by the restorer block_manager<>::ptr new_bm = open_bm(new_path, block_manager<>::READ_WRITE); + file_utils::check_file_exists(old_path, false); + metadata_touched = true; metadata::ptr new_md(new metadata(new_bm, metadata::CREATE, 128, 0)); emitter::ptr e = create_restore_emitter(new_md); block_manager<>::ptr old_bm = open_bm(old_path, block_manager<>::READ_ONLY); metadata_repair(old_bm, e); } catch (std::exception &e) { + if (metadata_touched) + file_utils::zero_superblock(new_path); cerr << e.what() << endl; return 1; } @@ -101,7 +107,7 @@ thin_repair_cmd::run(int argc, char **argv) check_output_file_requirements(*output_path); else { - cerr << "no output file provided" << endl; + cerr << "No output file provided." << endl; usage(cerr); return 1; } 3,beaker job: https://beaker.engineering.redhat.com/recipes/8409347#tasks Since the problem described in this bug report should be resolved in a recent advisory, it has been closed with a resolution of ERRATA. For information on the advisory (device-mapper-persistent-data bug fix and enhancement update), and where to find the updated files, follow the link below. If the solution does not work for you, open a new bug report. https://access.redhat.com/errata/RHBA-2020:3988 |