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 <[email protected]>
clearlinux · Apr 4, 2024 · 1547203 · 1547203
1 parent be0dc71
commit 1547203
Show file tree

Hide file tree

Showing 6 changed files with 149 additions and 53 deletions.
diff --git a/src/swupd_lib/target_root.c b/src/swupd_lib/target_root.c
@@ -21,6 +21,7 @@
  *
  */
 
+#include <asm-generic/errno.h>
 #include <assert.h>
 #include <errno.h>
 #include <libgen.h>
@@ -29,13 +30,15 @@
 #include <string.h>
 #include <sys/stat.h>
 #include <sys/types.h>
+#include <time.h>
 #include <unistd.h>
 
 #include "swupd.h"
 #include "swupd_build_variant.h"
 #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,54 +614,24 @@ 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;
 			}
 		}
 		FREE(target_path);
 	} 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
@@ -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-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
@@ -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
@@ -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
@@ -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