Message ID | 20220805094703.155967-1-lczerner@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | e2fsprogs: fix device name parsing to resolve names containing '=' | expand |
Thanks Lukas. Thanks for elaborating on your reasoning too - the ordering makes sense to me. Generally lgtm (though I'm not an owner/maintainer). On Fri, Aug 5, 2022 at 7:47 PM Lukas Czerner <lczerner@redhat.com> wrote: > > Currently in varisous e2fsprogs tools, most notably tune2fs and e2fsck > we will get the device name by passing the user provided string into > blkid_get_devname(). This library function however is primarily intended > for parsing "NAME=value" tokens. It will return the device matching the > specified token, NULL if nothing is found, or copy of the string if it's > not in "NAME=value" format. > > However in case where we're passing in a file name that contains an > equal sign blkid_get_devname() will treat it as a token and will attempt > to find the device with the match. Likely finding nothing. > > Fix it by checking existence of the file first and then attempt to call > blkid_get_devname(). In case of a collision, notify the user and > automatically prefer the one returned by blkid_get_devname(). Otherwise > return either the existing file, or NULL. > > We do it this way to avoid some existing file in working directory (for > example LABEL=volume-name) masking an actual device containing the > matchin LABEL. User can specify full, or relative path (e.g. > ./LABEL=volume-name) to make sure the file is used instead. > > Signed-off-by: Lukas Czerner <lczerner@redhat.com> > Reported-by: Daniel Ng <danielng@google.com> > --- > e2fsck/unix.c | 6 +++--- > lib/support/plausible.c | 35 ++++++++++++++++++++++++++++++++++- > lib/support/plausible.h | 3 +++ > misc/Makefile.in | 9 +++++---- > misc/e2initrd_helper.c | 5 +++-- > misc/fsck.c | 5 +++-- > misc/tune2fs.c | 4 ++-- > misc/util.c | 3 ++- > 8 files changed, 55 insertions(+), 15 deletions(-) > > diff --git a/e2fsck/unix.c b/e2fsck/unix.c > index ae231f93..edd7b9b2 100644 > --- a/e2fsck/unix.c > +++ b/e2fsck/unix.c > @@ -939,8 +939,8 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx) > goto sscanf_err; > break; > case 'j': > - ctx->journal_name = blkid_get_devname(ctx->blkid, > - optarg, NULL); > + ctx->journal_name = get_devname(ctx->blkid, > + optarg, NULL); > if (!ctx->journal_name) { > com_err(ctx->program_name, 0, > _("Unable to resolve '%s'"), > @@ -1019,7 +1019,7 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx) > ctx->io_options = strchr(argv[optind], '?'); > if (ctx->io_options) > *ctx->io_options++ = 0; > - ctx->filesystem_name = blkid_get_devname(ctx->blkid, argv[optind], 0); > + ctx->filesystem_name = get_devname(ctx->blkid, argv[optind], 0); > if (!ctx->filesystem_name) { > com_err(ctx->program_name, 0, _("Unable to resolve '%s'"), > argv[optind]); > diff --git a/lib/support/plausible.c b/lib/support/plausible.c > index bbed2a70..864a7a5e 100644 > --- a/lib/support/plausible.c > +++ b/lib/support/plausible.c > @@ -35,7 +35,6 @@ > #include "plausible.h" > #include "ext2fs/ext2fs.h" > #include "nls-enable.h" > -#include "blkid/blkid.h" > > #ifdef HAVE_MAGIC_H > static magic_t (*dl_magic_open)(int); > @@ -290,3 +289,37 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev) > return 1; > } > > + > +char *get_devname(blkid_cache cache, const char *token, const char *value) > +{ > + int is_file = 0; > + char *ret = NULL; > + > + if (!token) > + goto out; > + > + if (value) { > + ret = blkid_get_devname(cache, token, value); > + goto out; > + } > + > + if (access(token, F_OK) == 0) > + is_file = 1; > + > + ret = blkid_get_devname(cache, token, NULL); > + if (ret) { > + if (is_file && (strcmp(ret, token) != 0)) { > + fprintf(stderr, > + _("Collision found: '%s' refers to both '%s' " > + "and a file '%s'. Using '%s'!\n"), > + token, ret, token, ret); > + } > + goto out; > + } > + > +out_strdup: > + if (is_file) > + ret = strdup(token); > +out: > + return ret; > +} > diff --git a/lib/support/plausible.h b/lib/support/plausible.h > index b85150c7..8eb6817f 100644 > --- a/lib/support/plausible.h > +++ b/lib/support/plausible.h > @@ -13,6 +13,8 @@ > #ifndef PLAUSIBLE_H_ > #define PLAUSIBLE_H_ > > +#include "blkid/blkid.h" > + > /* > * Flags for check_plausibility() > */ > @@ -25,5 +27,6 @@ > > extern int check_plausibility(const char *device, int flags, > int *ret_is_dev); > +char *get_devname(blkid_cache cache, const char *token, const char *value); > > #endif /* PLAUSIBLE_H_ */ > diff --git a/misc/Makefile.in b/misc/Makefile.in > index 4db59cdf..5187883f 100644 > --- a/misc/Makefile.in > +++ b/misc/Makefile.in > @@ -360,12 +360,12 @@ dumpe2fs.static: $(DUMPE2FS_OBJS) $(DEPLIBS) $(DEPLIBS_E2P) $(DEPLIBUUID) $(DEPL > $(STATIC_LIBS) $(STATIC_LIBE2P) $(STATIC_LIBUUID) \ > $(LIBINTL) $(SYSLIBS) $(STATIC_LIBBLKID) $(LIBMAGIC) > > -fsck: $(FSCK_OBJS) $(DEPLIBBLKID) > +fsck: $(FSCK_OBJS) $(DEPLIBBLKID) $(DEPLIBS) > $(E) " LD $@" > $(Q) $(CC) $(ALL_LDFLAGS) -o fsck $(FSCK_OBJS) $(LIBBLKID) \ > - $(LIBINTL) $(SYSLIBS) > + $(LIBINTL) $(SYSLIBS) $(LIBS) $(LIBEXT2FS) $(LIBCOM_ERR) > > -fsck.profiled: $(FSCK_OBJS) $(PROFILED_DEPLIBBLKID) > +fsck.profiled: $(FSCK_OBJS) $(PROFILED_DEPLIBBLKID) $(PROFILED_DEPLIBS) > $(E) " LD $@" > $(Q) $(CC) $(ALL_LDFLAGS) -g -pg -o fsck.profiled $(PROFILED_FSCK_OBJS) \ > $(PROFILED_LIBBLKID) $(LIBINTL) $(SYSLIBS) > @@ -799,7 +799,8 @@ badblocks.o: $(srcdir)/badblocks.c $(top_builddir)/lib/config.h \ > $(top_srcdir)/lib/ext2fs/bitops.h $(top_srcdir)/lib/support/nls-enable.h > fsck.o: $(srcdir)/fsck.c $(top_builddir)/lib/config.h \ > $(top_builddir)/lib/dirpaths.h $(top_srcdir)/version.h \ > - $(top_srcdir)/lib/support/nls-enable.h $(srcdir)/fsck.h > + $(top_srcdir)/lib/support/nls-enable.h $(srcdir)/fsck.h \ > + $(top_srcdir)/lib/support/plausible.h > util.o: $(srcdir)/util.c $(top_builddir)/lib/config.h \ > $(top_builddir)/lib/dirpaths.h $(top_srcdir)/lib/et/com_err.h \ > $(top_srcdir)/lib/e2p/e2p.h $(top_srcdir)/lib/ext2fs/ext2_fs.h \ > diff --git a/misc/e2initrd_helper.c b/misc/e2initrd_helper.c > index 436aab8c..bfa294fa 100644 > --- a/misc/e2initrd_helper.c > +++ b/misc/e2initrd_helper.c > @@ -36,6 +36,7 @@ extern char *optarg; > #include "ext2fs/ext2fs.h" > #include "blkid/blkid.h" > #include "support/nls-enable.h" > +#include "support/plausible.h" > > #include "../version.h" > > @@ -262,7 +263,7 @@ static int parse_fstab_line(char *line, struct fs_info *fs) > parse_escape(freq); > parse_escape(passno); > > - dev = blkid_get_devname(cache, device, NULL); > + dev = get_devname(cache, device, NULL); > if (dev) > device = dev; > > @@ -325,7 +326,7 @@ static void PRS(int argc, char **argv) > } > if (optind < argc - 1 || optind == argc) > usage(); > - device_name = blkid_get_devname(NULL, argv[optind], NULL); > + device_name = get_devname(NULL, argv[optind], NULL); > if (!device_name) { > com_err(program_name, 0, _("Unable to resolve '%s'"), > argv[optind]); > diff --git a/misc/fsck.c b/misc/fsck.c > index 4efe10ec..75c520ee 100644 > --- a/misc/fsck.c > +++ b/misc/fsck.c > @@ -59,6 +59,7 @@ > #endif > > #include "../version.h" > +#include "support/plausible.h" > #include "support/nls-enable.h" > #include "fsck.h" > #include "blkid/blkid.h" > @@ -297,7 +298,7 @@ static int parse_fstab_line(char *line, struct fs_info **ret_fs) > parse_escape(freq); > parse_escape(passno); > > - dev = blkid_get_devname(cache, device, NULL); > + dev = get_devname(cache, device, NULL); > if (dev) > device = dev; > > @@ -1128,7 +1129,7 @@ static void PRS(int argc, char *argv[]) > progname); > exit(EXIT_ERROR); > } > - dev = blkid_get_devname(cache, arg, NULL); > + dev = get_devname(cache, arg, NULL); > if (!dev && strchr(arg, '=')) { > /* > * Check to see if we failed because > diff --git a/misc/tune2fs.c b/misc/tune2fs.c > index 6c162ba5..dfa7427b 100644 > --- a/misc/tune2fs.c > +++ b/misc/tune2fs.c > @@ -1839,7 +1839,7 @@ static void parse_e2label_options(int argc, char ** argv) > io_options = strchr(argv[1], '?'); > if (io_options) > *io_options++ = 0; > - device_name = blkid_get_devname(NULL, argv[1], NULL); > + device_name = get_devname(NULL, argv[1], NULL); > if (!device_name) { > com_err("e2label", 0, _("Unable to resolve '%s'"), > argv[1]); > @@ -2139,7 +2139,7 @@ static void parse_tune2fs_options(int argc, char **argv) > io_options = strchr(argv[optind], '?'); > if (io_options) > *io_options++ = 0; > - device_name = blkid_get_devname(NULL, argv[optind], NULL); > + device_name = get_devname(NULL, argv[optind], NULL); > if (!device_name) { > com_err(program_name, 0, _("Unable to resolve '%s'"), > argv[optind]); > diff --git a/misc/util.c b/misc/util.c > index 48e623dc..2b2ad07b 100644 > --- a/misc/util.c > +++ b/misc/util.c > @@ -45,6 +45,7 @@ > #include "ext2fs/ext2_fs.h" > #include "ext2fs/ext2fs.h" > #include "support/nls-enable.h" > +#include "support/plausible.h" > #include "blkid/blkid.h" > #include "util.h" > > @@ -183,7 +184,7 @@ void parse_journal_opts(const char *opts) > arg ? arg : "NONE"); > #endif > if (strcmp(token, "device") == 0) { > - journal_device = blkid_get_devname(NULL, arg, NULL); > + journal_device = get_devname(NULL, arg, NULL); > if (!journal_device) { > if (arg) > fprintf(stderr, _("\nCould not find " > -- > 2.37.1 >
Hi Lukas, Could you move get_devname() into its own file in lib/support? e.g., create a devname.c and devname.h. The reason for this is that plausible.c tries to call libmagic via dlopen() so we don't need to drag libmagic into the minimal package set for distros. But that means that any executiables that try to use devname() will drag in lib/support/pausible.c, which means if you don't change the makefiles to link in -ldl, static linking will fail: - Ted
On Wed, Aug 10, 2022 at 11:18:23PM -0400, Theodore Ts'o wrote: > Hi Lukas, > > Could you move get_devname() into its own file in lib/support? e.g., > create a devname.c and devname.h. > > The reason for this is that plausible.c tries to call libmagic via > dlopen() so we don't need to drag libmagic into the minimal package > set for distros. But that means that any executiables that try to use > devname() will drag in lib/support/pausible.c, which means if you > don't change the makefiles to link in -ldl, static linking will fail: > > - Ted Ah, I didn't notice that. I'll move it into a separate file, devname.[ch] sounds good. Thanks! -Lukas
diff --git a/e2fsck/unix.c b/e2fsck/unix.c index ae231f93..edd7b9b2 100644 --- a/e2fsck/unix.c +++ b/e2fsck/unix.c @@ -939,8 +939,8 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx) goto sscanf_err; break; case 'j': - ctx->journal_name = blkid_get_devname(ctx->blkid, - optarg, NULL); + ctx->journal_name = get_devname(ctx->blkid, + optarg, NULL); if (!ctx->journal_name) { com_err(ctx->program_name, 0, _("Unable to resolve '%s'"), @@ -1019,7 +1019,7 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx) ctx->io_options = strchr(argv[optind], '?'); if (ctx->io_options) *ctx->io_options++ = 0; - ctx->filesystem_name = blkid_get_devname(ctx->blkid, argv[optind], 0); + ctx->filesystem_name = get_devname(ctx->blkid, argv[optind], 0); if (!ctx->filesystem_name) { com_err(ctx->program_name, 0, _("Unable to resolve '%s'"), argv[optind]); diff --git a/lib/support/plausible.c b/lib/support/plausible.c index bbed2a70..864a7a5e 100644 --- a/lib/support/plausible.c +++ b/lib/support/plausible.c @@ -35,7 +35,6 @@ #include "plausible.h" #include "ext2fs/ext2fs.h" #include "nls-enable.h" -#include "blkid/blkid.h" #ifdef HAVE_MAGIC_H static magic_t (*dl_magic_open)(int); @@ -290,3 +289,37 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev) return 1; } + +char *get_devname(blkid_cache cache, const char *token, const char *value) +{ + int is_file = 0; + char *ret = NULL; + + if (!token) + goto out; + + if (value) { + ret = blkid_get_devname(cache, token, value); + goto out; + } + + if (access(token, F_OK) == 0) + is_file = 1; + + ret = blkid_get_devname(cache, token, NULL); + if (ret) { + if (is_file && (strcmp(ret, token) != 0)) { + fprintf(stderr, + _("Collision found: '%s' refers to both '%s' " + "and a file '%s'. Using '%s'!\n"), + token, ret, token, ret); + } + goto out; + } + +out_strdup: + if (is_file) + ret = strdup(token); +out: + return ret; +} diff --git a/lib/support/plausible.h b/lib/support/plausible.h index b85150c7..8eb6817f 100644 --- a/lib/support/plausible.h +++ b/lib/support/plausible.h @@ -13,6 +13,8 @@ #ifndef PLAUSIBLE_H_ #define PLAUSIBLE_H_ +#include "blkid/blkid.h" + /* * Flags for check_plausibility() */ @@ -25,5 +27,6 @@ extern int check_plausibility(const char *device, int flags, int *ret_is_dev); +char *get_devname(blkid_cache cache, const char *token, const char *value); #endif /* PLAUSIBLE_H_ */ diff --git a/misc/Makefile.in b/misc/Makefile.in index 4db59cdf..5187883f 100644 --- a/misc/Makefile.in +++ b/misc/Makefile.in @@ -360,12 +360,12 @@ dumpe2fs.static: $(DUMPE2FS_OBJS) $(DEPLIBS) $(DEPLIBS_E2P) $(DEPLIBUUID) $(DEPL $(STATIC_LIBS) $(STATIC_LIBE2P) $(STATIC_LIBUUID) \ $(LIBINTL) $(SYSLIBS) $(STATIC_LIBBLKID) $(LIBMAGIC) -fsck: $(FSCK_OBJS) $(DEPLIBBLKID) +fsck: $(FSCK_OBJS) $(DEPLIBBLKID) $(DEPLIBS) $(E) " LD $@" $(Q) $(CC) $(ALL_LDFLAGS) -o fsck $(FSCK_OBJS) $(LIBBLKID) \ - $(LIBINTL) $(SYSLIBS) + $(LIBINTL) $(SYSLIBS) $(LIBS) $(LIBEXT2FS) $(LIBCOM_ERR) -fsck.profiled: $(FSCK_OBJS) $(PROFILED_DEPLIBBLKID) +fsck.profiled: $(FSCK_OBJS) $(PROFILED_DEPLIBBLKID) $(PROFILED_DEPLIBS) $(E) " LD $@" $(Q) $(CC) $(ALL_LDFLAGS) -g -pg -o fsck.profiled $(PROFILED_FSCK_OBJS) \ $(PROFILED_LIBBLKID) $(LIBINTL) $(SYSLIBS) @@ -799,7 +799,8 @@ badblocks.o: $(srcdir)/badblocks.c $(top_builddir)/lib/config.h \ $(top_srcdir)/lib/ext2fs/bitops.h $(top_srcdir)/lib/support/nls-enable.h fsck.o: $(srcdir)/fsck.c $(top_builddir)/lib/config.h \ $(top_builddir)/lib/dirpaths.h $(top_srcdir)/version.h \ - $(top_srcdir)/lib/support/nls-enable.h $(srcdir)/fsck.h + $(top_srcdir)/lib/support/nls-enable.h $(srcdir)/fsck.h \ + $(top_srcdir)/lib/support/plausible.h util.o: $(srcdir)/util.c $(top_builddir)/lib/config.h \ $(top_builddir)/lib/dirpaths.h $(top_srcdir)/lib/et/com_err.h \ $(top_srcdir)/lib/e2p/e2p.h $(top_srcdir)/lib/ext2fs/ext2_fs.h \ diff --git a/misc/e2initrd_helper.c b/misc/e2initrd_helper.c index 436aab8c..bfa294fa 100644 --- a/misc/e2initrd_helper.c +++ b/misc/e2initrd_helper.c @@ -36,6 +36,7 @@ extern char *optarg; #include "ext2fs/ext2fs.h" #include "blkid/blkid.h" #include "support/nls-enable.h" +#include "support/plausible.h" #include "../version.h" @@ -262,7 +263,7 @@ static int parse_fstab_line(char *line, struct fs_info *fs) parse_escape(freq); parse_escape(passno); - dev = blkid_get_devname(cache, device, NULL); + dev = get_devname(cache, device, NULL); if (dev) device = dev; @@ -325,7 +326,7 @@ static void PRS(int argc, char **argv) } if (optind < argc - 1 || optind == argc) usage(); - device_name = blkid_get_devname(NULL, argv[optind], NULL); + device_name = get_devname(NULL, argv[optind], NULL); if (!device_name) { com_err(program_name, 0, _("Unable to resolve '%s'"), argv[optind]); diff --git a/misc/fsck.c b/misc/fsck.c index 4efe10ec..75c520ee 100644 --- a/misc/fsck.c +++ b/misc/fsck.c @@ -59,6 +59,7 @@ #endif #include "../version.h" +#include "support/plausible.h" #include "support/nls-enable.h" #include "fsck.h" #include "blkid/blkid.h" @@ -297,7 +298,7 @@ static int parse_fstab_line(char *line, struct fs_info **ret_fs) parse_escape(freq); parse_escape(passno); - dev = blkid_get_devname(cache, device, NULL); + dev = get_devname(cache, device, NULL); if (dev) device = dev; @@ -1128,7 +1129,7 @@ static void PRS(int argc, char *argv[]) progname); exit(EXIT_ERROR); } - dev = blkid_get_devname(cache, arg, NULL); + dev = get_devname(cache, arg, NULL); if (!dev && strchr(arg, '=')) { /* * Check to see if we failed because diff --git a/misc/tune2fs.c b/misc/tune2fs.c index 6c162ba5..dfa7427b 100644 --- a/misc/tune2fs.c +++ b/misc/tune2fs.c @@ -1839,7 +1839,7 @@ static void parse_e2label_options(int argc, char ** argv) io_options = strchr(argv[1], '?'); if (io_options) *io_options++ = 0; - device_name = blkid_get_devname(NULL, argv[1], NULL); + device_name = get_devname(NULL, argv[1], NULL); if (!device_name) { com_err("e2label", 0, _("Unable to resolve '%s'"), argv[1]); @@ -2139,7 +2139,7 @@ static void parse_tune2fs_options(int argc, char **argv) io_options = strchr(argv[optind], '?'); if (io_options) *io_options++ = 0; - device_name = blkid_get_devname(NULL, argv[optind], NULL); + device_name = get_devname(NULL, argv[optind], NULL); if (!device_name) { com_err(program_name, 0, _("Unable to resolve '%s'"), argv[optind]); diff --git a/misc/util.c b/misc/util.c index 48e623dc..2b2ad07b 100644 --- a/misc/util.c +++ b/misc/util.c @@ -45,6 +45,7 @@ #include "ext2fs/ext2_fs.h" #include "ext2fs/ext2fs.h" #include "support/nls-enable.h" +#include "support/plausible.h" #include "blkid/blkid.h" #include "util.h" @@ -183,7 +184,7 @@ void parse_journal_opts(const char *opts) arg ? arg : "NONE"); #endif if (strcmp(token, "device") == 0) { - journal_device = blkid_get_devname(NULL, arg, NULL); + journal_device = get_devname(NULL, arg, NULL); if (!journal_device) { if (arg) fprintf(stderr, _("\nCould not find "
Currently in varisous e2fsprogs tools, most notably tune2fs and e2fsck we will get the device name by passing the user provided string into blkid_get_devname(). This library function however is primarily intended for parsing "NAME=value" tokens. It will return the device matching the specified token, NULL if nothing is found, or copy of the string if it's not in "NAME=value" format. However in case where we're passing in a file name that contains an equal sign blkid_get_devname() will treat it as a token and will attempt to find the device with the match. Likely finding nothing. Fix it by checking existence of the file first and then attempt to call blkid_get_devname(). In case of a collision, notify the user and automatically prefer the one returned by blkid_get_devname(). Otherwise return either the existing file, or NULL. We do it this way to avoid some existing file in working directory (for example LABEL=volume-name) masking an actual device containing the matchin LABEL. User can specify full, or relative path (e.g. ./LABEL=volume-name) to make sure the file is used instead. Signed-off-by: Lukas Czerner <lczerner@redhat.com> Reported-by: Daniel Ng <danielng@google.com> --- e2fsck/unix.c | 6 +++--- lib/support/plausible.c | 35 ++++++++++++++++++++++++++++++++++- lib/support/plausible.h | 3 +++ misc/Makefile.in | 9 +++++---- misc/e2initrd_helper.c | 5 +++-- misc/fsck.c | 5 +++-- misc/tune2fs.c | 4 ++-- misc/util.c | 3 ++- 8 files changed, 55 insertions(+), 15 deletions(-)