From 9c4befbc229ea1c0517205664b34477f9a088a89 Mon Sep 17 00:00:00 2001 From: William Douglas Date: Thu, 4 Apr 2024 14:59:51 -0700 Subject: [PATCH] Rework content deletion handling Currently sys_rm_recursive was used in any instance of deleting swupd content from the system (update, repair and bundle-remove). This can cause user data loss when unkown files are in directories that swupd is deleting. To prevent this, this patch changes how deleting content in swupd operates. Swupd content removal is now done with sys_rm and the return value is checked in case the removal failed due to a directory that still had files in it. When this specific failure occurs, the directory is added to a new list for reprocessing removals as it is expected once the rest of the deletes on the system occur the failures will go away as the directories will be empty (these deletes are processed in alphabetical reverse order so leaf directories are processed first). If the removal fails again it is presumed the contents of the directory are not files swupd knows about and as such should be kept somewhere else. For handling the retention of user data, directories (with only the content unknown to swupd) are renamed (currently using a .deleted.$timestamp. prefix of the old name) and stored at the same directory level they were previously found with one exception. The exception is for nested deleted content best illustrated with an example: /swupd-dir1/user-file1 /swupd-dir1/swupd-dir2/user-file2 When swupd tries to remove the /swupd-dir1 content, it will store the user files as follows: /.deleted.$timestamp1.swupd-dir1/user-file1 /.deleted.$timestamp1.swupd-dir1/.deleted.$timestamp1.swupd-dir2/user-file2 To demarcate what was part of swupd content vs user content. Signed-off-by: William Douglas --- src/swupd_lib/target_root.c | 173 +++++++++++++----- .../bundleremove/remove-basics.bats | 4 +- test/functional/repair/repair-error.bats | 12 +- .../repair/repair-type-changed-system.bats | 2 + test/functional/testlib.bash | 4 +- .../update/update-newest-deleted.bats | 16 +- .../update-type-changes-dir-to-file.bats | 3 + 7 files changed, 155 insertions(+), 59 deletions(-) diff --git a/src/swupd_lib/target_root.c b/src/swupd_lib/target_root.c index 14d8fca17..5cd5eb668 100644 --- a/src/swupd_lib/target_root.c +++ b/src/swupd_lib/target_root.c @@ -21,6 +21,7 @@ * */ +#include #include #include #include @@ -29,6 +30,7 @@ #include #include #include +#include #include #include "swupd.h" @@ -36,6 +38,7 @@ #include "xattrs.h" #define STAGE_FILE_PREFIX ".update." +#define BACKUP_FILE_PREFIX ".deleted." static bool verify_fix_path(char *dir, struct manifest *mom); @@ -330,6 +333,61 @@ static enum swupd_code install_dir(const char *fullfile_path, const char *target return install_dir_using_tar(fullfile_path, target_file); } +/* Backup target directory to a backup and timestamp prefixed name. */ +/* This function should only be used after the deletes are processed. */ +static enum swupd_code move_to_timestamp(const char *filename) +{ + struct tm *timeinfo; + time_t now; + char time_str[50] = { 0 }; + char *backup_dir = NULL; + int ret = SWUPD_OK; + char *target_basename; + char *dir = NULL; + char *target_file = NULL; + char *target_path = NULL; + + dir = sys_dirname(filename); + target_basename = sys_basename(filename); + target_path = sys_path_join("%s/%s", globals.path_prefix, dir); + target_file = sys_path_join("%s/%s", globals.path_prefix, filename); + + time(&now); + timeinfo = localtime(&now); + if (!timeinfo) { + ret = SWUPD_TIME_UNKNOWN; + goto out; + } + + strftime(time_str, sizeof(time_str), "%s", timeinfo); + backup_dir = sys_path_join("%s/%s%s.%s", target_path, BACKUP_FILE_PREFIX, time_str, target_basename); + + /* It shouldn't be possible to have an already created backup_dir + but just in case, try and remove it and error out if unable to. */ + ret = sys_rm_recursive(backup_dir); + if (ret != 0 && ret != -ENOENT) { + ret = SWUPD_COULDNT_REMOVE_FILE; + error("Failed to remove backup dir %s\n", backup_dir); + goto out; + } + + ret = rename(target_file, backup_dir); + if (ret != 0) { + ret = SWUPD_COULDNT_RENAME_DIR; + goto out; + } + +out: + FREE(dir); + FREE(target_file); + FREE(target_path); + + if (backup_dir) { + FREE(backup_dir); + } + return ret; +} + /* Do the staging of new files into the filesystem */ // TODO: "stage_single_file is currently not able to be run in parallel" /* Consider adding a remove_leftovers() that runs in verify/fix in order to @@ -389,11 +447,13 @@ static enum swupd_code stage_single_file(struct file *file, struct manifest *mom if (file_type_changed(target_file, file)) { // file type changed, move old out of the way for new debug("The file type changed for file %s, removing old file\n", target_file); - ret = sys_rm_recursive(target_file); + ret = sys_rm(target_file); if (ret != 0 && ret != -ENOENT) { - error("Target has different file type but could not be removed: %s\n", target_file); - ret = SWUPD_COULDNT_REMOVE_FILE; - goto out; + ret = move_to_timestamp(file->filename); + if (ret != 0) { + error("Target has different file type but could not be moved to backup location: %s\n", target_file); + goto out; + } } } @@ -554,11 +614,11 @@ int rename_staged_file_to_final(struct file *file) * or we might end up deleting something else */ target_path = sys_dirname(target); if (sys_path_is_absolute(target_path)) { - ret = sys_rm_recursive(target); + ret = sys_rm(target); /* don't count missing ones as errors... * if somebody already deleted them for us then all is well */ - if ((ret == -ENOENT) || (ret == -ENOTDIR)) { + if (ret == -ENOENT) { ret = 0; } } @@ -566,42 +626,12 @@ int rename_staged_file_to_final(struct file *file) } else if (file->is_dir || file->is_ghosted) { ret = 0; } else { - /* If the file was previously a directory but no longer, then - * we need to move it out of the way. - * This should not happen because the server side complains - * when creating update content that includes such a state - * change. But...you never know. */ - - if (sys_is_dir(target)) { - char *lostnfound; - char *base; - - lostnfound = sys_path_join("%s/lost+found", globals.path_prefix); - ret = mkdir(lostnfound, S_IRWXU); - if ((ret != 0) && (errno != EEXIST)) { - FREE(lostnfound); - FREE(target); - return ret; - } - FREE(lostnfound); - - base = sys_basename(file->filename); - lostnfound = sys_path_join("%s/lost+found/%s", globals.path_prefix, base); - /* this will fail if the directory was not already emptied */ - ret = rename(target, lostnfound); - if (ret < 0 && errno != ENOTEMPTY && errno != EEXIST) { - error("failed to move %s to lost+found: %s\n", - base, strerror(errno)); - } - FREE(lostnfound); - } else { - ret = rename(file->staging, target); - if (ret < 0) { - error("failed to rename staged %s to final: %s\n", - file->hash, strerror(errno)); - } - unlink(file->staging); + ret = rename(file->staging, target); + if (ret < 0) { + error("failed to rename staged %s to final: %s\n", + file->hash, strerror(errno)); } + unlink(file->staging); } FREE(target); @@ -611,7 +641,8 @@ int rename_staged_file_to_final(struct file *file) static int rename_all_files_to_final(struct list *updates) { int ret, update_errs = 0; - struct list *list; + struct list *dirs_to_remove = NULL; + struct list *list, *dirs_list; int complete = 0; int list_length = list_len(updates); @@ -623,7 +654,9 @@ static int rename_all_files_to_final(struct list *updates) if (!file->do_not_update) { ret = rename_staged_file_to_final(file); - if (ret != 0) { + if (ret == -ENOTEMPTY) { + dirs_to_remove = list_prepend_data(dirs_to_remove, file); + } else if (ret != 0) { update_errs++; } } @@ -632,6 +665,25 @@ static int rename_all_files_to_final(struct list *updates) progress_report(list_length + complete, list_length * 2); } + dirs_list = list_head(dirs_to_remove); + while (dirs_list) { + struct file *file; + file = dirs_list->data; + dirs_list = dirs_list->next; + + ret = rename_staged_file_to_final(file); + if (ret != 0) { + ret = move_to_timestamp(file->filename); + if (ret != 0) { + error("Could not move file to backup location: %s\n", file->filename); + } + } + if (ret != 0) { + update_errs++; + } + } + list_free_list(dirs_to_remove); + return -update_errs; } @@ -732,21 +784,27 @@ enum swupd_code target_root_install_files(struct list *files, struct manifest *m int target_root_remove_files(struct list *files) { struct list *iter = NULL; + struct list *dirs_to_remove = NULL; + struct list *dirs_iter = NULL; struct file *file = NULL; char *fullfile = NULL; int total = list_len(files); int deleted = total; int count = 0; + int ret; iter = list_head(files); while (iter) { file = iter->data; iter = iter->next; fullfile = sys_path_join("%s/%s", globals.path_prefix, file->filename); - if (sys_rm_recursive(fullfile) == -1) { - /* if a -1 is returned it means there was an issue deleting the - * file or directory, in that case decrease the counter of deleted - * files. + ret = sys_rm(fullfile); + if (ret == -ENOTEMPTY) { + count--; + dirs_to_remove = list_prepend_data(dirs_to_remove, file); + } else if (ret != 0) { + /* There was an issue deleting the file or directory, in that + * case decrease the counter of deleted files. * Note: If a file didn't exist it will still be counted as deleted, * this is a limitation */ deleted--; @@ -756,5 +814,26 @@ int target_root_remove_files(struct list *files) progress_report(count, total); } + dirs_iter = list_head(dirs_to_remove); + while (dirs_iter) { + file = dirs_iter->data; + dirs_iter = dirs_iter->next; + fullfile = sys_path_join("%s/%s", globals.path_prefix, file->filename); + ret = sys_rm(fullfile); + if (ret != 0) { + ret = move_to_timestamp(file->filename); + if (ret != 0) { + error("Could not move file to backup location: %s\n", file->filename); + } + } + if (ret != 0) { + deleted--; + } + FREE(fullfile); + count++; + progress_report(count, total); + } + list_free_list(dirs_to_remove); + return deleted; } diff --git a/test/functional/bundleremove/remove-basics.bats b/test/functional/bundleremove/remove-basics.bats index ad7f34468..38ffb9f54 100755 --- a/test/functional/bundleremove/remove-basics.bats +++ b/test/functional/bundleremove/remove-basics.bats @@ -31,6 +31,7 @@ test_teardown() { @test "REM001: Removing one bundle" { + sudo mkdir "$TARGET_DIR"/bar/keep1 run sudo sh -c "$SWUPD bundle-remove $SWUPD_OPTS test-bundle1" assert_status_is 0 @@ -42,6 +43,8 @@ test_teardown() { assert_dir_not_exists "$TARGET_DIR"/foo assert_dir_not_exists "$TARGET_DIR"/bar assert_file_not_exists "$STATE_DIR"/bundles/test-bundle1 + # keep file isn't removed + assert_file_exists "$TARGET_DIR"/.deleted.*.bar/keep1 # bundle2 was not removed assert_file_exists "$TARGET_DIR"/usr/share/clear/bundles/test-bundle2 assert_file_exists "$TARGET_DIR"/bat/test-file4 @@ -55,7 +58,6 @@ test_teardown() { Successfully removed 1 bundle EOM ) - assert_is_output "$expected_output" } diff --git a/test/functional/repair/repair-error.bats b/test/functional/repair/repair-error.bats index 13b084c02..c258a013a 100755 --- a/test/functional/repair/repair-error.bats +++ b/test/functional/repair/repair-error.bats @@ -47,12 +47,12 @@ test_teardown() { Validate downloaded files Starting download of remaining update content. This may take a while... Adding any missing files - Error: Target has different file type but could not be removed: $ABS_TARGET_DIR/baz + Error: Target has different file type but could not be moved to backup location: $ABS_TARGET_DIR/baz -> Missing file: $ABS_TARGET_DIR/baz/bat -> not fixed - Error: Target has different file type but could not be removed: $ABS_TARGET_DIR/baz + Error: Target has different file type but could not be moved to backup location: $ABS_TARGET_DIR/baz -> Missing file: $ABS_TARGET_DIR/baz/bat/file_3 -> not fixed Repairing corrupt files - Error: Target has different file type but could not be removed: $ABS_TARGET_DIR/baz + Error: Target has different file type but could not be moved to backup location: $ABS_TARGET_DIR/baz -> Hash mismatch for file: $ABS_TARGET_DIR/baz -> not fixed -> Hash mismatch for file: $ABS_TARGET_DIR/foo/file_1 -> fixed -> Hash mismatch for file: $ABS_TARGET_DIR/usr/lib/os-release -> fixed @@ -83,11 +83,11 @@ test_teardown() { assert_status_is_not "$SWUPD_OK" expected_output=$(cat <<-EOM - Error: Target has different file type but could not be removed: $ABS_TARGET_DIR/baz + Error: Target has different file type but could not be moved to backup location: $ABS_TARGET_DIR/baz $ABS_TARGET_DIR/baz/bat -> not fixed - Error: Target has different file type but could not be removed: $ABS_TARGET_DIR/baz + Error: Target has different file type but could not be moved to backup location: $ABS_TARGET_DIR/baz $ABS_TARGET_DIR/baz/bat/file_3 -> not fixed - Error: Target has different file type but could not be removed: $ABS_TARGET_DIR/baz + Error: Target has different file type but could not be moved to backup location: $ABS_TARGET_DIR/baz $ABS_TARGET_DIR/baz -> not fixed $ABS_TARGET_DIR/foo/file_1 -> fixed $ABS_TARGET_DIR/usr/lib/os-release -> fixed diff --git a/test/functional/repair/repair-type-changed-system.bats b/test/functional/repair/repair-type-changed-system.bats index a38904833..fe9289e26 100755 --- a/test/functional/repair/repair-type-changed-system.bats +++ b/test/functional/repair/repair-type-changed-system.bats @@ -94,6 +94,7 @@ test_setup() { assert_regular_file_exists "$TARGET_DIR"/file_removed assert_regular_file_exists "$TARGET_DIR"/file_to_dir + assert_file_exists "$TARGET_DIR"/.deleted.*.symlink_to_dir/dir/file assert_regular_file_exists "$TARGET_DIR"/file_to_symlink assert_regular_file_exists "$TARGET_DIR"/file_to_symlink_dir assert_regular_file_exists "$TARGET_DIR"/file_to_symlink_broken @@ -105,6 +106,7 @@ test_setup() { assert_symlink_exists "$TARGET_DIR"/symlink_to_dir run stat --printf "%N" "$TARGET_DIR"/symlink_to_dir assert_is_output "'$TARGET_DIR/symlink_to_dir' -> 'broken'" + assert_file_exists "$TARGET_DIR"/.deleted.*.symlink_to_dir/dir/file assert_symlink_exists "$TARGET_DIR"/symlink_to_symlink run stat --printf "%N" "$TARGET_DIR"/symlink_to_symlink diff --git a/test/functional/testlib.bash b/test/functional/testlib.bash index d77315de3..ffe92e6dd 100644 --- a/test/functional/testlib.bash +++ b/test/functional/testlib.bash @@ -304,7 +304,7 @@ show_target() { # swupd_function EOM )" "$@" - print "\n$(tree "$TARGET_DIR")\n" + print "\n$(tree -a "$TARGET_DIR")\n" } @@ -4050,7 +4050,7 @@ update_bundle() { # swupd_function sudo sed -i "/\\t$filename$/s/./d/2" "$bundle_manifest" sudo sed -i "/\\t$filename\\t/s/./d/2" "$bundle_manifest" # remove the related file(s) from the version dir (if there) - sudo rm -f "$version_path"/files/"$fhash" + sudo rm -f "$version_path"/files/"$fhash" || sudo rmdir "$version_path"/files/"$fhash" sudo rm -f "$version_path"/files/"$fhash".tar else # replace the second character of the line that matches with "g" diff --git a/test/functional/update/update-newest-deleted.bats b/test/functional/update/update-newest-deleted.bats index d3cd1289e..1b8943835 100755 --- a/test/functional/update/update-newest-deleted.bats +++ b/test/functional/update/update-newest-deleted.bats @@ -6,11 +6,14 @@ test_setup() { create_test_environment "$TEST_NAME" create_bundle -L -n test-bundle1 -f /testfile "$TEST_NAME" - create_bundle -L -n test-bundle2 -d /foo "$TEST_NAME" + create_bundle -L -n test-bundle2 -d /foo -d /foo/bar -f /foo/bar/baz "$TEST_NAME" create_version -p "$TEST_NAME" 20 10 update_bundle "$TEST_NAME" os-core --update /core update_bundle "$TEST_NAME" test-bundle1 --delete /testfile update_bundle "$TEST_NAME" test-bundle2 --add /testfile + update_bundle "$TEST_NAME" test-bundle2 --delete /foo/bar/baz + update_bundle "$TEST_NAME" test-bundle2 --delete /foo/bar + update_bundle "$TEST_NAME" test-bundle2 --delete /foo create_version -p "$TEST_NAME" 30 20 update_bundle "$TEST_NAME" os-core --update /core update_bundle "$TEST_NAME" test-bundle2 --delete /testfile @@ -20,6 +23,8 @@ test_setup() { @test "UPD003: Updating a system where a file was deleted in the newer version" { # NOTE: we don't create delta packs when a file is deleted in an update with the test library + sudo mkdir "$TARGET_DIR"/foo/bar/keep1 + sudo mkdir "$TARGET_DIR"/foo/keep2 run sudo sh -c "$SWUPD update $SWUPD_OPTS" assert_status_is 0 @@ -37,7 +42,7 @@ test_setup() { deleted bundles : 0 changed files : 1 new files : 0 - deleted files : 1 + deleted files : 4 Validate downloaded files No extra files need to be downloaded Installing files... @@ -48,7 +53,12 @@ test_setup() { ) assert_is_output "$expected_output" assert_file_not_exists "$TARGET_DIR"/testfile - + assert_dir_not_exists "$TARGET_DIR"/foo + assert_dir_not_exists "$TARGET_DIR"/foo/bar + assert_file_not_exists "$TARGET_DIR"/foo/bar/baz + assert_dir_exists "$TARGET_DIR"/.deleted.*.foo/keep2 + assert_dir_exists "$TARGET_DIR"/.deleted.*.foo/.deleted.*.bar/keep1 + show_target } #WEIGHT=7 diff --git a/test/functional/update/update-type-changes-dir-to-file.bats b/test/functional/update/update-type-changes-dir-to-file.bats index 2d1667b33..25d7c0fb7 100755 --- a/test/functional/update/update-type-changes-dir-to-file.bats +++ b/test/functional/update/update-type-changes-dir-to-file.bats @@ -44,6 +44,8 @@ test_setup() { assert_file_exists "$TARGET_DIR"/file1 assert_file_exists "$TARGET_DIR"/common_file assert_dir_exists "$TARGET_DIR"/dir1 + sudo mkdir "$TARGET_DIR"/dir1/dir2 + show_target run sudo sh -c "$SWUPD update $SWUPD_OPTS" @@ -75,6 +77,7 @@ test_setup() { assert_dir_exists "$TARGET_DIR"/file1 assert_dir_not_exists "$TARGET_DIR"/dir1 assert_file_exists "$TARGET_DIR"/dir1 + assert_dir_exists "$TARGET_DIR"/.deleted.*.dir1/dir2 assert_file_exists "$TARGET_DIR"/common_file show_target