[PATCH] erofs-utils: fsck: fix issues related to --extract=X
Gao Xiang
hsiangkao at linux.alibaba.com
Fri Jan 21 12:47:16 AEDT 2022
Hi Igor,
On Fri, Jan 21, 2022 at 02:31:16AM +0200, Igor Ostapenko wrote:
> From: Igor Eisberg <igoreisberg at gmail.com>
>
> Have to disagree with some changes made to my original patch.
Thanks for the patch. If you had opinion with the original patch,
I'm very happy that if you could comment on the original patch
at that time in public. erofs-utils works for long-term plans,
we need to consider more scenarios in advance (I admit I cannot
think carefully very well as well). Sorry about slight modification
your original patch.
> 1) Using tar options makes no sense here, since tar has default
> behaviors set for normal user (uses user's owner ID + perms)
> vs. root user (preserve original owner IDs + perms by default),
So we can have such behavior as well (it seems many extractors
have such behavior, such as tar and unsquashfs.)
> and the options were there to override that preset behavior.
> fsck doesn't have any default behavior set, and the default
> values for preserve_owner and preserve_perms were left for
> the compiler to decide. This change sets the default values
> to false explicitly.
> "--no-same-owner" and "--no-same-permissions" are completely
> redundant in fsck's case, unlike tar.
No matter what default behaviors we use now, it should have both
options with opposite behaviors. Otherwise, if some user write
a script and the default behavior then changes, it will break
completely (because such users don't need to rely on the default
behavior actually.)
> * "--same-owner" and "--same-permissions" were renamed to
> "--preserve-owner" and "--preserve-perms" to better represent
> what these options do, the word "same" is ambiguous and tells
> nothing to the user ("same" to what?).
I'm either ok with "--preserve-owner" and "--preserve-perms"
words.
> * Added "--preserve" as a shortcut for both options in one.
Ok, if you like, I'm fine with this.
> * Fixed option descriptions as they had typos and were too
> ambiguous ("extract information" to where? separate file?).
> 2) Errors for chmod 0700 were not logged, failures were silent.
Yeah, thanks! (but to make sure that each line (except for the print
message) is no more than 80 chars.)
> 3) --extract=/ should fail with a proper error due to it being
> dangerous if used with sudo.
I tend to disagree such idea, since add (or overwrite) files
to / may be useful. Also, tar can do like this, why not
fsck.erofs? If you think that is great dangerous, we could add
another '--force' option together with this.
Thanks,
Gao Xiang
>
> Signed-off-by: Igor Ostapenko <igoreisberg at gmail.com>
> ---
> fsck/main.c | 62 ++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/fsck/main.c b/fsck/main.c
> index 14534b9..e2f4157 100644
> --- a/fsck/main.c
> +++ b/fsck/main.c
> @@ -18,15 +18,17 @@
> static int erofsfsck_check_inode(erofs_nid_t pnid, erofs_nid_t nid);
>
> struct erofsfsck_cfg {
> - bool corrupted;
> bool print_comp_ratio;
> bool check_decomp;
> char *extract_path;
> size_t extract_pos;
> - bool overwrite, preserve_owner, preserve_perms;
> + bool overwrite;
> + bool preserve_owner;
> + bool preserve_perms;
> mode_t umask;
> u64 physical_blocks;
> u64 logical_blocks;
> + bool corrupted;
> };
> static struct erofsfsck_cfg fsckcfg;
>
> @@ -34,11 +36,10 @@ static struct option long_options[] = {
> {"help", no_argument, 0, 1},
> {"extract", optional_argument, 0, 2},
> {"device", required_argument, 0, 3},
> - {"no-same-owner", no_argument, 0, 4},
> - {"no-same-permissions", no_argument, 0, 5},
> - {"same-owner", no_argument, 0, 6},
> - {"same-permissions", no_argument, 0, 7},
> - {"overwrite", no_argument, 0, 8},
> + {"overwrite", no_argument, 0, 4},
> + {"preserve", no_argument, 0, 5},
> + {"preserve-owner", no_argument, 0, 6},
> + {"preserve-perms", no_argument, 0, 7},
> {0, 0, 0, 0},
> };
>
> @@ -66,11 +67,10 @@ static void usage(void)
> " --extract[=X] check if all files are well encoded, optionally extract to X\n"
> " --help display this help and exit\n"
> "\nExtraction options (--extract=X is required):\n"
> - " --no-same-owner extract files as yourself\n"
> - " --no-same-permissions apply the user's umask when extracting permission\n"
> - " --same-permissions extract information about file permissions\n"
> - " --same-owner extract files about the file ownerships\n"
> - " --overwrite if file already exists then overwrite\n"
> + " --overwrite overwrite files that already exist\n"
> + " --preserve extract with original ownerships and permissions\n"
> + " --preserve-owner extract with original ownerships only\n"
> + " --preserve-perms extract with original permissions only\n"
> "\nSupported algorithms are: ", stderr);
> print_available_decompressors(stderr, ", ");
> }
> @@ -112,10 +112,16 @@ static int erofsfsck_parse_options_cfg(int argc, char **argv)
> if (len == 0)
> return -EINVAL;
>
> - /* remove trailing slashes except root */
> - while (len > 1 && optarg[len - 1] == '/')
> + /* remove trailing slashes */
> + while (len > 0 && optarg[len - 1] == '/')
> len--;
>
> + /* extracting directly to root is dangerous */
> + if (len == 0) {
> + erofs_err("invalid extract path: /");
> + return -EINVAL;
> + }
> +
> fsckcfg.extract_path = malloc(PATH_MAX);
> if (!fsckcfg.extract_path)
> return -ENOMEM;
> @@ -131,10 +137,11 @@ static int erofsfsck_parse_options_cfg(int argc, char **argv)
> ++sbi.extra_devices;
> break;
> case 4:
> - fsckcfg.preserve_owner = false;
> + fsckcfg.overwrite = true;
> break;
> case 5:
> - fsckcfg.preserve_perms = false;
> + fsckcfg.preserve_owner = true;
> + fsckcfg.preserve_perms = true;
> break;
> case 6:
> fsckcfg.preserve_owner = true;
> @@ -142,9 +149,6 @@ static int erofsfsck_parse_options_cfg(int argc, char **argv)
> case 7:
> fsckcfg.preserve_perms = true;
> break;
> - case 8:
> - fsckcfg.overwrite = true;
> - break;
> default:
> return -EINVAL;
> }
> @@ -465,7 +469,7 @@ static inline int erofs_extract_dir(struct erofs_inode *inode)
> struct stat st;
>
> if (errno != EEXIST) {
> - erofs_err("failed to create directory %s: %s",
> + erofs_err("failed to create directory: %s (%s)",
> fsckcfg.extract_path, strerror(errno));
> return -errno;
> }
> @@ -481,8 +485,11 @@ static inline int erofs_extract_dir(struct erofs_inode *inode)
> * Try to change permissions of existing directory so
> * that we can write to it
> */
> - if (chmod(fsckcfg.extract_path, 0700) < 0)
> + if (chmod(fsckcfg.extract_path, 0700) < 0) {
> + erofs_err("failed to set permissions: %s (%s)",
> + fsckcfg.extract_path, strerror(errno));
> return -errno;
> + }
> }
> return 0;
> }
> @@ -510,6 +517,8 @@ again:
> }
> } else if (errno == EACCES &&
> chmod(fsckcfg.extract_path, 0700) < 0) {
> + erofs_err("failed to set permissions: %s (%s)",
> + fsckcfg.extract_path, strerror(errno));
> return -errno;
> }
> tryagain = false;
> @@ -680,13 +689,13 @@ int main(int argc, char **argv)
>
> erofs_init_configure();
>
> - fsckcfg.corrupted = false;
> fsckcfg.print_comp_ratio = false;
> fsckcfg.check_decomp = false;
> fsckcfg.extract_path = NULL;
> fsckcfg.extract_pos = 0;
> - fsckcfg.logical_blocks = 0;
> - fsckcfg.physical_blocks = 0;
> + fsckcfg.overwrite = false;
> + fsckcfg.preserve_owner = false;
> + fsckcfg.preserve_perms = false;
>
> err = erofsfsck_parse_options_cfg(argc, argv);
> if (err) {
> @@ -696,6 +705,9 @@ int main(int argc, char **argv)
> }
>
> fsckcfg.umask = umask(0);
> + fsckcfg.logical_blocks = 0;
> + fsckcfg.physical_blocks = 0;
> + fsckcfg.corrupted = false;
>
> err = dev_open_ro(cfg.c_img_path);
> if (err) {
> @@ -725,7 +737,7 @@ int main(int argc, char **argv)
> if (!fsckcfg.extract_path)
> erofs_info("No error found");
> else
> - erofs_info("Extract data successfully");
> + erofs_info("Filesystem extracted successfully");
>
> if (fsckcfg.print_comp_ratio) {
> double comp_ratio =
> --
> 2.30.2
More information about the Linux-erofs
mailing list