2022-08-05 09:59:00

by Lukas Czerner

[permalink] [raw]
Subject: [PATCH] e2fsprogs: fix device name parsing to resolve names containing '='

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 <[email protected]>
Reported-by: Daniel Ng <[email protected]>
---
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 [email protected]"
$(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 [email protected]"
$(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



2022-08-08 03:32:09

by Daniel Ng

[permalink] [raw]
Subject: Re: [PATCH] e2fsprogs: fix device name parsing to resolve names containing '='

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 <[email protected]> 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 <[email protected]>
> Reported-by: Daniel Ng <[email protected]>
> ---
> 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 [email protected]"
> $(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 [email protected]"
> $(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
>

2022-08-11 03:38:50

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH] e2fsprogs: fix device name parsing to resolve names containing '='

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

2022-08-12 07:15:21

by Lukas Czerner

[permalink] [raw]
Subject: Re: [PATCH] e2fsprogs: fix device name parsing to resolve names containing '='

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

2022-08-12 13:15:49

by Lukas Czerner

[permalink] [raw]
Subject: [PATCH v2] [PATCH] e2fsprogs: fix device name parsing to resolve names containing '='

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 <[email protected]>
Reported-by: Daniel Ng <[email protected]>
---
v2: Move get_devname() into it's own file

e2fsck/Makefile.in | 3 +-
e2fsck/unix.c | 7 +++--
lib/support/Makefile.in | 8 +++--
lib/support/devname.c | 66 +++++++++++++++++++++++++++++++++++++++++
lib/support/devname.h | 20 +++++++++++++
misc/Makefile.in | 17 ++++++-----
misc/e2initrd_helper.c | 5 ++--
misc/fsck.c | 5 ++--
misc/tune2fs.c | 5 ++--
misc/util.c | 3 +-
10 files changed, 118 insertions(+), 21 deletions(-)
create mode 100644 lib/support/devname.c
create mode 100644 lib/support/devname.h

diff --git a/e2fsck/Makefile.in b/e2fsck/Makefile.in
index 71ac3cf5..2112df57 100644
--- a/e2fsck/Makefile.in
+++ b/e2fsck/Makefile.in
@@ -465,7 +465,8 @@ unix.o: $(srcdir)/unix.c $(top_builddir)/lib/config.h \
$(top_srcdir)/lib/ext2fs/fast_commit.h $(top_srcdir)/lib/ext2fs/jfs_compat.h \
$(top_srcdir)/lib/ext2fs/kernel-list.h $(top_srcdir)/lib/ext2fs/compiler.h \
$(srcdir)/problem.h $(srcdir)/jfs_user.h \
- $(top_srcdir)/lib/ext2fs/kernel-jbd.h $(top_srcdir)/version.h
+ $(top_srcdir)/lib/ext2fs/kernel-jbd.h $(top_srcdir)/version.h \
+ $(top_srcdir)/lib/support/devname.h
dirinfo.o: $(srcdir)/dirinfo.c $(top_builddir)/lib/config.h \
$(top_builddir)/lib/dirpaths.h $(srcdir)/e2fsck.h \
$(top_srcdir)/lib/ext2fs/ext2_fs.h $(top_builddir)/lib/ext2fs/ext2_types.h \
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index ae231f93..0154aa93 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -52,6 +52,7 @@ extern int optind;
#include "e2p/e2p.h"
#include "uuid/uuid.h"
#include "support/plausible.h"
+#include "support/devname.h"
#include "e2fsck.h"
#include "problem.h"
#include "jfs_user.h"
@@ -939,8 +940,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 +1020,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/Makefile.in b/lib/support/Makefile.in
index f3c7981e..3dfec776 100644
--- a/lib/support/Makefile.in
+++ b/lib/support/Makefile.in
@@ -23,7 +23,8 @@ OBJS= cstring.o \
quotaio.o \
quotaio_v2.o \
quotaio_tree.o \
- dict.o
+ dict.o \
+ devname.o

SRCS= $(srcdir)/argv_parse.c \
$(srcdir)/cstring.c \
@@ -36,7 +37,8 @@ SRCS= $(srcdir)/argv_parse.c \
$(srcdir)/quotaio.c \
$(srcdir)/quotaio_tree.c \
$(srcdir)/quotaio_v2.c \
- $(srcdir)/dict.c
+ $(srcdir)/dict.c \
+ $(srcdir)/devname.c

LIBRARY= libsupport
LIBDIR= support
@@ -169,3 +171,5 @@ quotaio_v2.o: $(srcdir)/quotaio_v2.c $(top_builddir)/lib/config.h \
$(srcdir)/quotaio_tree.h
dict.o: $(srcdir)/dict.c $(top_builddir)/lib/config.h \
$(top_builddir)/lib/dirpaths.h $(srcdir)/dict.h
+devname.o: $(srcdir)/devname.c $(top_builddir)/lib/config.h \
+ $(top_builddir)/lib/dirpaths.h $(srcdir)/nls-enable.h $(srcdir)/devname.h
diff --git a/lib/support/devname.c b/lib/support/devname.c
new file mode 100644
index 00000000..8c2349a3
--- /dev/null
+++ b/lib/support/devname.c
@@ -0,0 +1,66 @@
+/*
+ * devname.c --- Support function to translate a user provided string
+ * identifying a device to an actual device path
+ *
+ * Copyright (C) 2022 Red Hat, Inc., Lukas Czerner <[email protected]>
+ *
+ * %Begin-Header%
+ * This file may be redistributed under the terms of the GNU Public
+ * License.
+ * %End-Header%
+ */
+
+#include <unistd.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "config.h"
+#include "devname.h"
+#include "nls-enable.h"
+
+/*
+ * blkid_get_devname() 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.
+ * get_devname() takes the same parameters and works the same way as
+ * blkid_get_devname() except it can handle '=' in the file name.
+ */
+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) {
+ /*
+ * In case of collision prefer the result from
+ * blkid_get_devname() to avoid a file masking file system with
+ * existing tag.
+ */
+ 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/devname.h b/lib/support/devname.h
new file mode 100644
index 00000000..9d411512
--- /dev/null
+++ b/lib/support/devname.h
@@ -0,0 +1,20 @@
+/*
+ * devname.c --- Figure out if a pathname is ext* or something else.
+ *
+ * Copyright (C) 2022 Red Hat, Inc., Lukas Czerner <[email protected]>
+ *
+ * %Begin-Header%
+ * This file may be redistributed under the terms of the GNU Public
+ * License.
+ * %End-Header%
+ */
+
+#ifndef DEVNAME_H_
+#define DEVNAME_H_
+
+#include "blkid/blkid.h"
+
+char *get_devname(blkid_cache cache, const char *token, const char *value);
+
+#endif /* DEVNAME_H_ */
+
diff --git a/misc/Makefile.in b/misc/Makefile.in
index 4db59cdf..96e36871 100644
--- a/misc/Makefile.in
+++ b/misc/Makefile.in
@@ -360,15 +360,15 @@ 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 [email protected]"
$(Q) $(CC) $(ALL_LDFLAGS) -o fsck $(FSCK_OBJS) $(LIBBLKID) \
- $(LIBINTL) $(SYSLIBS)
+ $(LIBINTL) $(SYSLIBS) $(LIBS)

-fsck.profiled: $(FSCK_OBJS) $(PROFILED_DEPLIBBLKID)
+fsck.profiled: $(FSCK_OBJS) $(PROFILED_DEPLIBBLKID) $(PROFILED_DEPLIBS)
$(E) " LD [email protected]"
$(Q) $(CC) $(ALL_LDFLAGS) -g -pg -o fsck.profiled $(PROFILED_FSCK_OBJS) \
- $(PROFILED_LIBBLKID) $(LIBINTL) $(SYSLIBS)
+ $(PROFILED_LIBBLKID) $(LIBINTL) $(SYSLIBS) $(PROFILED_LIBS)

badblocks: $(BADBLOCKS_OBJS) $(DEPLIBS)
$(E) " LD [email protected]"
@@ -736,8 +736,8 @@ tune2fs.o: $(srcdir)/tune2fs.c $(top_builddir)/lib/config.h \
$(top_srcdir)/lib/ext2fs/jfs_compat.h $(top_srcdir)/lib/ext2fs/kernel-list.h \
$(top_srcdir)/lib/ext2fs/compiler.h $(top_srcdir)/lib/support/plausible.h \
$(top_srcdir)/lib/support/quotaio.h $(top_srcdir)/lib/support/dqblk_v2.h \
- $(top_srcdir)/lib/support/quotaio_tree.h $(top_srcdir)/lib/e2p/e2p.h \
- $(srcdir)/util.h $(top_srcdir)/version.h \
+ $(top_srcdir)/lib/support/quotaio_tree.h $(top_srcdir)/lib/support/devname.h \
+ $(top_srcdir)/lib/e2p/e2p.h $(srcdir)/util.h $(top_srcdir)/version.h \
$(top_srcdir)/lib/support/nls-enable.h
mklost+found.o: $(srcdir)/mklost+found.c $(top_builddir)/lib/config.h \
$(top_builddir)/lib/dirpaths.h $(top_srcdir)/lib/ext2fs/ext2_fs.h \
@@ -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 $(top_srcdir)/lib/support/devname.h \
+ $(srcdir)/fsck.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 \
@@ -808,7 +809,7 @@ util.o: $(srcdir)/util.c $(top_builddir)/lib/config.h \
$(top_builddir)/lib/ext2fs/ext2_err.h \
$(top_srcdir)/lib/ext2fs/ext2_ext_attr.h $(top_srcdir)/lib/ext2fs/hashmap.h \
$(top_srcdir)/lib/ext2fs/bitops.h $(top_srcdir)/lib/support/nls-enable.h \
- $(srcdir)/util.h
+ $(srcdir)/util.h $(top_srcdir)/lib/support/devname.h
uuidgen.o: $(srcdir)/uuidgen.c $(top_builddir)/lib/config.h \
$(top_builddir)/lib/dirpaths.h $(top_srcdir)/lib/support/nls-enable.h
blkid.o: $(srcdir)/blkid.c $(top_builddir)/lib/config.h \
diff --git a/misc/e2initrd_helper.c b/misc/e2initrd_helper.c
index 436aab8c..b39fe15d 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/devname.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..1f6ec7d9 100644
--- a/misc/fsck.c
+++ b/misc/fsck.c
@@ -59,6 +59,7 @@
#endif

#include "../version.h"
+#include "support/devname.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..65e32711 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -62,6 +62,7 @@ extern int optind;
#include "et/com_err.h"
#include "support/plausible.h"
#include "support/quotaio.h"
+#include "support/devname.h"
#include "uuid/uuid.h"
#include "e2p/e2p.h"
#include "util.h"
@@ -1839,7 +1840,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 +2140,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..e84ebab5 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/devname.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

2022-08-13 14:59:13

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH v2] [PATCH] e2fsprogs: fix device name parsing to resolve names containing '='

On Fri, 12 Aug 2022 15:01:22 +0200, Lukas Czerner 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.
>
> [...]

Applied, thanks!

[1/1] e2fsprogs: fix device name parsing to resolve names containing '='
commit: 18ebcf26f478702cd09dd4229320d449469f1490

Best regards,
--
Theodore Ts'o <[email protected]>