libmultipath: always use glibc basename()

[kraj]

There is a use of basename() which expects it to be GNU version of basename, which is not available in other libcs e.g. musl on Linux therefore provide a version for such cases

[bmarzins]

I thought musl would give you the gnu basename() implementation if you defined _GNU_SOURCE, and the libmultipath Makefile does this. If you build this with V=1, do you see _GNU_SOURCE getting defined. Something like

building configure.o because of configure.c ../libmpathcmd/mpath_cmd.h checkers.h list.h defaults.h ../libmpathutil/vector.h devmapper.h autoconfig.h structs.h prio.h byteorder.h generic.h structs_vec.h config.h ../libmpathutil/globals.h lock.h dmparser.h blacklist.h propsel.h discovery.h ../libmpathutil/debug.h ../libmpathutil/log_pthread.h switchgroup.h dm-generic.h print.h configure.h pgpolicies.h dict.h alias.h ../libmpathutil/util.h ../libmpathutil/uxsock.h wwids.h sysfs.h ../libmpathutil/strbuf.h io_err_stat.h cc -D_FORTIFY_SOURCE=3 -DURCU_VERSION=0x000d02 -D_FILE_OFFSET_BITS=64 -DBIN_DIR=\"/sbin\" -DMULTIPATH_DIR=\"/lib64/multipath\" -DRUNTIME_DIR=\"/run\" -DCONFIG_DIR=\"/etc/multipath/conf.d\" -DDEFAULT_CONFIGFILE=\"/etc/multipath.conf\" -DSTATE_DIR=\"/etc/multipath\" -DEXTRAVERSION=\"-g1c4301a\" -MMD -MP -I../libmpathutil -I../libmpathcmd -I../libmultipath/nvme -D_GNU_SOURCE -DUSE_SYSTEMD=253 -std=gnu99 -O2 -g -fstack-protector-strong --param=ssp-buffer-size=4 -Werror -Wall -Wextra -Wformat=2 -Wformat-overflow=2 -Werror=implicit-int -Werror=implicit-function-declaration -Werror=format-security -Wno-clobbered -Wno-error=clobbered -Werror=cast-qual -Werror=discarded-qualifiers -pipe -fPIC -c -o configure.o configure.c with -D_GNU_SOURCE in the command? If I'm wrong, then I guess something like this is necessary. Either that or we put this code into a .h file that both util.c and configure.c include, so we don't need to have two copies of it.

[kraj]

I thought musl would give you the gnu basename() implementation if you defined _GNU_SOURCE, and the libmultipath Makefile does this. If you build this with V=1, do you see _GNU_SOURCE getting defined. Something like

building configure.o because of configure.c ../libmpathcmd/mpath_cmd.h checkers.h list.h defaults.h ../libmpathutil/vector.h devmapper.h autoconfig.h structs.h prio.h byteorder.h generic.h structs_vec.h config.h ../libmpathutil/globals.h lock.h dmparser.h blacklist.h propsel.h discovery.h ../libmpathutil/debug.h ../libmpathutil/log_pthread.h switchgroup.h dm-generic.h print.h configure.h pgpolicies.h dict.h alias.h ../libmpathutil/util.h ../libmpathutil/uxsock.h wwids.h sysfs.h ../libmpathutil/strbuf.h io_err_stat.h cc -D_FORTIFY_SOURCE=3 -DURCU_VERSION=0x000d02 -D_FILE_OFFSET_BITS=64 -DBIN_DIR=\"/sbin\" -DMULTIPATH_DIR=\"/lib64/multipath\" -DRUNTIME_DIR=\"/run\" -DCONFIG_DIR=\"/etc/multipath/conf.d\" -DDEFAULT_CONFIGFILE=\"/etc/multipath.conf\" -DSTATE_DIR=\"/etc/multipath\" -DEXTRAVERSION=\"-g1c4301a\" -MMD -MP -I../libmpathutil -I../libmpathcmd -I../libmultipath/nvme -D_GNU_SOURCE -DUSE_SYSTEMD=253 -std=gnu99 -O2 -g -fstack-protector-strong --param=ssp-buffer-size=4 -Werror -Wall -Wextra -Wformat=2 -Wformat-overflow=2 -Werror=implicit-int -Werror=implicit-function-declaration -Werror=format-security -Wno-clobbered -Wno-error=clobbered -Werror=cast-qual -Werror=discarded-qualifiers -pipe -fPIC -c -o configure.o configure.c with -D_GNU_SOURCE in the command? If I'm wrong, then I guess something like this is necessary. Either that or we put this code into a .h file that both util.c and configure.c include, so we don't need to have two copies of it.

musl does not provide GNU basename implementation at all. In the past it provided a prototype in string.h which infact was not correct because it will compile programs which expected GNU basename without errors and link it to posix implementation, I am sure compiler warned about it, but warnings are warnings not errors so it was never addressed.

[mwilck]

I see this on alpine

# grep PRETTY /etc/os-release 
PRETTY_NAME="Alpine Linux v3.19"
# apk list musl
musl-1.2.4_git20230717-r4 x86_64 {musl} (MIT) [installed]
# grep -C1 basename /usr/include/string.h
#ifndef __cplusplus
char *basename();
#endif

The code is here in musl 1.2.4, too. musl 1.2.5 seems to have removed it.

I'd expect that gcc warns about this when -Wstrict-prototypes is enabled. We use -Wextra -Werror, but this doesn't imply -Wstrict-prototypes. However, for reasons I don't understand, even with -Wstrict-prototypes the above code doesn't trigger a warning. Perhaps it's because the declaration is in an include file?

[kraj]

yes we need to use musl 1.2.5+ and distros might be reverting the patch so manually remove it from string.h for tests.

[mwilck]

We shouldn't duplicate this code between two different files, though. Rather, we should export our basename() function from util.c.

FTR: The musl version of basename() (see above) only alters the string if it ends with / (slash). AFAICS this can't happen in our code. Only the DEV_DEVMAP code path is affected. In the multipathd case (cli_add_map()) only map names will be accepted. In the multipath case, device names will also be accepted, but these won't end with /.

Thus unless I am mistaken, there is neither a security risk nor a risk of crashing / major breakage by using the musl version of basename() in get_udev_device(). This also explains that we haven't had any previous bug reports from Alpine / musl users before.

[bmarzins]

Maybe I'm just being paranoid, but personally, I'd rather we export something not named basename() from libmpathutil, and have a header file that util.c and configure.c can include to redefine basename(). I'd rather not be redefining common functions in libraries that could be included in third-party programs (both libmpathpersist and libmpathvalid have been used outside of the multipath-tools codebase, and both pull in libmpathutil).

[mwilck]

Maybe I'm just being paranoid, but personally, I'd rather we export something not named basename() from libmpathutil

Exporting it shouldn't be an issue because ld.so searches breadth-first, and will thus find our own basename() first. But I agree that it's better to play safe. And we need to be careful with header files and prototypes. So yes, we should rather use a different function name.

[mwilck]

However, for reasons I don't understand, even with -Wstrict-prototypes the above code doesn't trigger a warning.

The reason is that gcc never prints warnings about system header code.

[mwilck]

I've pushed an alternative patch to my tip branch.

@kraj, @bmarzins, please take a look.

[bmarzins]

@mwilck LGTM.

[mwilck]

Thanks. According to our policy, I'll add your reviewed-by tags, send it to dm-devel, and push it to https://github.com/openSUSE/multipath-tools (queue branch).