From: Kou Wenqi <[email protected]>
On Thu, 2026-07-16 at 15:49 +0200, [email protected] wrote:
> Thanks again. This looks quite good. However, what about this?
>
> enum {
> DFP_DEVICE_CREATE = DM_DEVICE_CREATE,
> DFP_DEVICE_RELOAD = DM_DEVICE_RELOAD,
> DFP_ERR = -1,
> };
Good idea. I've defined this enum in kpartx/devmapper.h (which now
also includes <libdevmapper.h> to make the header self-contained),
and dm_find_part() uses these enum values explicitly in all return
statements.
> Then use values of this enum as return values of dm_find_part().
> If it returns DFP_ERR, bail out; otherwise use the return value as
> "op", like this:
>
> if ((op = dm_find_part(...)) == DFP_ERR)
> continue;
Done. In the ADD/UPDATE loops, the return value is assigned directly
to 'op', and DFP_ERR is handled with r++ and continue. In the DELETE
loop, I check against DFP_DEVICE_RELOAD to proceed with removal.
> If you want to be extra careful, you could add something like
> this at the beginning of dm_find_part():
>
> BUILD_BUG_ON(DM_DEVICE_CREATE < 0 || DM_DEVICE_RELOAD < 0);
>
> But I think we can trust that these values will always be positive.
I've omitted this check, as I agree that DM_DEVICE_CREATE and
DM_DEVICE_RELOAD are defined by libdevmapper as positive constants
and are unlikely to change.
Changes since v3:
- Defined enum { DFP_DEVICE_CREATE, DFP_DEVICE_RELOAD, DFP_ERR }
in kpartx/devmapper.h
- dm_find_part() returns enum values explicitly in all return paths
- Callers assign return value directly to 'op' and check for DFP_ERR
- kpartx/devmapper.h now includes <libdevmapper.h> directly
Here is the new patch.
When the -p delimiter is long enough to make the formatted partition
name exceed PARTNAME_SIZE (128 bytes), three issues occur:
1. format_partname() fails but snprintf has already written a truncated
name into the buffer. dm_find_part() returns 0 and the caller
proceeds to dm_addmap() with the truncated name, creating a device
that was never intended.
2. dm_find_part() returns early without setting *part_uuid. The
uninitialized local variable part_uuid then gets passed to
check_uuid() -> strchr(), causing a SIGSEGV.
3. The callers cannot distinguish between 'partition not found, create
new' and 'name construction failed' since both return 0.
Fix by:
- Having dm_find_part() return DFP_ERR when format_partname() fails,
with an error message printed by dm_find_part() itself
- Defining an enum to represent the return values of dm_find_part():
enum {
DFP_DEVICE_CREATE = DM_DEVICE_CREATE,
DFP_DEVICE_RELOAD = DM_DEVICE_RELOAD,
DFP_ERR = -1
};
This enum is placed in kpartx/devmapper.h, which now also includes
<libdevmapper.h> directly to make the header self-contained.
dm_find_part() uses these enum values explicitly in all return
statements.
- In the ADD/UPDATE loops (both main and container), assigning the
return value directly to 'op' and checking for DFP_ERR to handle
errors
- In the DELETE loop, checking the return value against
DFP_DEVICE_RELOAD to proceed with removal, skipping on error or
not-found
- Initializing part_uuid to NULL in all three partition loop bodies
(ADD/UPDATE main loop, container partition loop, DELETE loop) so
that the 'if (part_uuid && uuid)' guard correctly skips the UUID
check when dm_find_part() returns early
Reproduce steps:
# Create test image
dd if=/dev/zero of=/tmp/vhlg-test.img bs=1M count=10
parted /tmp/vhlg-test.img mklabel msdos
parted /tmp/vhlg-test.img mkpart primary ext4 1MiB 5MiB
# Reproduce
kpartx -a -p $(python3 -c "print('A'*200)") /tmp/vhlg-test.img
# Cleanup
kpartx -d /tmp/vhlg-test.img
rm -f /tmp/vhlg-test.img
Signed-off-by: Kou Wenqi <[email protected]>
---
kpartx/devmapper.c | 17 +++++++++--------
kpartx/devmapper.h | 9 +++++++++
kpartx/kpartx.c | 38 ++++++++++++++++++++++----------------
3 files changed, 40 insertions(+), 24 deletions(-)
diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c
index 45dac585..091d2a49 100644
--- a/kpartx/devmapper.c
+++ b/kpartx/devmapper.c
@@ -644,18 +644,19 @@ int dm_find_part(const char *parent, const char *delim,
int part,
char dev_t[32];
if (!format_partname(name, namesiz, parent, delim, part)) {
- if (verbose)
- fprintf(stderr, "partname too small\n");
- return 0;
+ fprintf(stderr, "partition name too long for partition %d\n",
part);
+ return DFP_ERR;
}
r = dm_map_present(name, part_uuid);
- if (r == 1 || parent_uuid == NULL || *parent_uuid == '\0')
- return r;
+ if (r == 1)
+ return DFP_DEVICE_RELOAD;
+ if (parent_uuid == NULL || *parent_uuid == '\0')
+ return DFP_DEVICE_CREATE;
uuid = make_prefixed_uuid(part, parent_uuid);
if (!uuid)
- return 0;
+ return DFP_DEVICE_CREATE;
tmp = dm_find_uuid(uuid);
if (tmp == NULL)
@@ -688,14 +689,14 @@ int dm_find_part(const char *parent, const char *delim,
int part,
if (r == 1) {
free(tmp);
*part_uuid = uuid;
- return 1;
+ return DFP_DEVICE_RELOAD;
}
if (verbose)
fprintf(stderr, "renaming %s->%s failed\n", tmp, name);
out:
free(uuid);
free(tmp);
- return r;
+ return DFP_DEVICE_CREATE;
}
char *nondm_create_uuid(dev_t devt)
diff --git a/kpartx/devmapper.h b/kpartx/devmapper.h
index e4db8621..99bab221 100644
--- a/kpartx/devmapper.h
+++ b/kpartx/devmapper.h
@@ -1,6 +1,8 @@
#ifndef KPARTX_DEVMAPPER_H_INCLUDED
#define KPARTX_DEVMAPPER_H_INCLUDED
+#include <libdevmapper.h>
+
#ifdef DM_SUBSYSTEM_UDEV_FLAG0
#define MPATH_UDEV_RELOAD_FLAG DM_SUBSYSTEM_UDEV_FLAG0
#else
@@ -18,6 +20,13 @@ dev_t dm_get_first_dep(char *devname);
char * dm_mapuuid(const char *mapname);
int dm_devn (const char * mapname, unsigned int *major, unsigned int *minor);
int dm_remove_partmaps (char * mapname, char *uuid, dev_t devt, int verbose);
+
+enum {
+ DFP_DEVICE_CREATE = DM_DEVICE_CREATE,
+ DFP_DEVICE_RELOAD = DM_DEVICE_RELOAD,
+ DFP_ERR = -1
+};
+
int dm_find_part(const char *parent, const char *delim, int part,
const char *parent_uuid,
char *name, size_t namesiz, char **part_uuid, int verbose);
diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c
index cfd82128..1a12f55d 100644
--- a/kpartx/kpartx.c
+++ b/kpartx/kpartx.c
@@ -437,7 +437,7 @@ main(int argc, char **argv){
case UPDATE:
/* ADD and UPDATE share the same code that adds new
partitions. */
for (j = 0, c = 0; j < n; j++) {
- char *part_uuid, *reason;
+ char *part_uuid = NULL, *reason;
if (slices[j].size == 0)
continue;
@@ -454,10 +454,13 @@ main(int argc, char **argv){
exit(1);
}
- op = (dm_find_part(mapname, delim, j + 1, uuid,
- partname, sizeof(partname),
- &part_uuid, verbose) ?
- DM_DEVICE_RELOAD : DM_DEVICE_CREATE);
+ op = dm_find_part(mapname, delim, j + 1, uuid,
+ partname, sizeof(partname),
+ &part_uuid, verbose);
+ if (op == DFP_ERR) {
+ r++;
+ continue;
+ }
if (part_uuid && uuid) {
if (check_uuid(uuid, part_uuid,
&reason) != 0) {
@@ -500,7 +503,7 @@ main(int argc, char **argv){
d = c;
while (c) {
for (j = 0; j < n; j++) {
- char *part_uuid, *reason;
+ char *part_uuid = NULL, *reason;
int k = slices[j].container - 1;
if (slices[j].size == 0)
@@ -526,11 +529,14 @@ main(int argc, char **argv){
exit(1);
}
- op = (dm_find_part(mapname, delim, j +
1, uuid,
- partname,
- sizeof(partname),
- &part_uuid, verbose)
?
- DM_DEVICE_RELOAD :
DM_DEVICE_CREATE);
+ op = dm_find_part(mapname, delim, j +
1, uuid,
+ partname,
+ sizeof(partname),
+ &part_uuid, verbose);
+ if (op == DFP_ERR) {
+ r++;
+ continue;
+ }
if (part_uuid && uuid) {
if (check_uuid(uuid, part_uuid,
&reason) != 0) {
@@ -570,11 +576,11 @@ main(int argc, char **argv){
}
for (j = MAXSLICES-1; j >= 0; j--) {
- char *part_uuid, *reason;
- if (slices[j].size ||
- !dm_find_part(mapname, delim, j + 1, uuid,
- partname, sizeof(partname),
- &part_uuid, verbose))
+ char *part_uuid = NULL, *reason;
+ int res = dm_find_part(mapname, delim, j + 1,
uuid,
+ partname,
sizeof(partname),
+ &part_uuid, verbose);
+ if (slices[j].size || res != DFP_DEVICE_RELOAD)
continue;
if (part_uuid && uuid) {
--
2.43.0
