| Message ID | 20240212184747.30422-1-clayton@craftyguy.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | wiphy: include libgen.h to fix implicit def. with musl libc | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-alpine-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-gitlint | fail | wiphy: include libgen.h to fix implicit def. with musl libc 11: B1 Line exceeds max length (90>80): "1. https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7" |
| prestwoj/iwd-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-incremental_build | success | Incremental build not run PASS |
| prestwoj/iwd-alpine-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-alpine-ci-incremental_build | success | Incremental build not run PASS |
| prestwoj/iwd-ci-build | success | Build - Configure |
| prestwoj/iwd-alpine-ci-build | success | Build - Configure |
| prestwoj/iwd-alpine-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-alpine-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-clang | success | clang PASS |
| prestwoj/iwd-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-ci-testrunner | success | test-runner PASS |
Hi Clayton, On 2/12/24 12:47, Clayton Craft wrote: > This fixes a runtime segfault with musl libc and GCC13. musl dropped a > basename prototype[1] in string.h, causing GCC to define it implicitly > with the wrong function signature. The correct basename definition based > on how wiphy uses it is in libgen.h. Hmm, why did musl do that? man 3 basename indicates that the libgen.h version implementation is free to scratch in the input argument, while the string.h one doesn't. In this particular case it doesn't matter since driver_path is never used again. Still, the string.h version is much more intuitive. Maybe this should be re-implemented in missing.h or ell (say l_file_basename or l_dir_basename) instead? > > For context, see: > https://gitlab.alpinelinux.org/alpine/aports/-/issues/15622 > > 1. https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7 > --- > src/wiphy.c | 1 + > 1 file changed, 1 insertion(+) > Regards, -Denis
Hi Denis, On Tue, 13 Feb 2024 17:09:19 -0600 Denis Kenzior <denkenz@gmail.com> wrote: > Hmm, why did musl do that? man 3 basename indicates that the libgen.h version > implementation is free to scratch in the input argument, while the string.h one > doesn't. From what I can tell, they added it here[1] to appease apps expecting the GNU libc string.h:basename, and removed it here[2] because C23 drops non-proto declarations? Also I think it's not POSIX C compliant, so maybe some of the motivation for musl to drop it? I'm just speculating though... 1. https://git.musl-libc.org/cgit/musl/commit/?id=06aec8d7152dfb8360cb7ed9b3d7215ca0b0b500 2. https://git.musl-libc.org/cgit/musl/commit/?id=725e17ed6dff4d0cd22487bb64470881e86a92e7 > In this particular case it doesn't matter since driver_path is never used again. > Still, the string.h version is much more intuitive. Ahh ok I understand, thanks for explaining the differences. > Maybe this should be re-implemented in missing.h or ell (say l_file_basename or > l_dir_basename) instead? I'd defer to y'all on what the right path forward is here, since I'm really not familiar with the code base here. I'd like to see this fixed somehow, so I'm happy to try implementing something you recommend. -Clayton
Hi Denis, >> This fixes a runtime segfault with musl libc and GCC13. musl dropped a >> basename prototype[1] in string.h, causing GCC to define it implicitly >> with the wrong function signature. The correct basename definition based >> on how wiphy uses it is in libgen.h. > > Hmm, why did musl do that? man 3 basename indicates that the libgen.h version implementation is free to scratch in the input argument, while the string.h one doesn't. > > In this particular case it doesn't matter since driver_path is never used again. Still, the string.h version is much more intuitive. > > Maybe this should be re-implemented in missing.h or ell (say l_file_basename or l_dir_basename) instead? we can also just do a l_util_basename that does the right thing and allocates a new string from it. Regards Marcel
Hi Marcel, On 2/14/24 01:03, Marcel Holtmann wrote: > Hi Denis, > >>> This fixes a runtime segfault with musl libc and GCC13. musl dropped a >>> basename prototype[1] in string.h, causing GCC to define it implicitly >>> with the wrong function signature. The correct basename definition based >>> on how wiphy uses it is in libgen.h. >> >> Hmm, why did musl do that? man 3 basename indicates that the libgen.h version implementation is free to scratch in the input argument, while the string.h one doesn't. >> >> In this particular case it doesn't matter since driver_path is never used again. Still, the string.h version is much more intuitive. >> >> Maybe this should be re-implemented in missing.h or ell (say l_file_basename or l_dir_basename) instead? > > we can also just do a l_util_basename that does the right thing and allocates a new string from it. We have l_path*, so I guess it would be l_path_basename. That would work as well in this case. I only see two instances of basename() use in ofono / connman, both in src/log.c. Given that the only real user is iwd, I wonder if we should just add l_sysctl_get_driver() API instead? Regards, -Denis
Hi Denis, >>>> This fixes a runtime segfault with musl libc and GCC13. musl dropped a >>>> basename prototype[1] in string.h, causing GCC to define it implicitly >>>> with the wrong function signature. The correct basename definition based >>>> on how wiphy uses it is in libgen.h. >>> >>> Hmm, why did musl do that? man 3 basename indicates that the libgen.h version implementation is free to scratch in the input argument, while the string.h one doesn't. >>> >>> In this particular case it doesn't matter since driver_path is never used again. Still, the string.h version is much more intuitive. >>> >>> Maybe this should be re-implemented in missing.h or ell (say l_file_basename or l_dir_basename) instead? >> we can also just do a l_util_basename that does the right thing and allocates a new string from it. > > We have l_path*, so I guess it would be l_path_basename. That would work as well in this case. > > I only see two instances of basename() use in ofono / connman, both in src/log.c. Given that the only real user is iwd, I wonder if we should just add l_sysctl_get_driver() API instead? we can do that of course as well. I have used basename() a lot recently for other code I have been working on and I got annoyed by it, but then just accepted it as is. If musl compilation makes us always stumble, we need a helper that does the right thing. So be it l_util or l_path is something I do not care about. Add a _get_driver is a good idea as well, but what is the reason for l_sysctl since sysctl is some specific system call. Regards Marcel
Hi Marcel, > > Add a _get_driver is a good idea as well, but what is the reason for l_sysctl since sysctl is some specific system call. l_sysctl is used to write stuff into procfs or sysfs. Naming comes from the sysctl(8) utility. Regards, -Denis
Hi Denis, >> Add a _get_driver is a good idea as well, but what is the reason for l_sysctl since sysctl is some specific system call. > > l_sysctl is used to write stuff into procfs or sysfs. Naming comes from the sysctl(8) utility. actually no. sysctl just handles /proc/sys/. It has nothing to do with sysfs. Regards Marcel
Hi Marcel, On 2/14/24 10:39, Marcel Holtmann wrote: > Hi Denis, > >>> Add a _get_driver is a good idea as well, but what is the reason for l_sysctl since sysctl is some specific system call. >> >> l_sysctl is used to write stuff into procfs or sysfs. Naming comes from the sysctl(8) utility. > > actually no. sysctl just handles /proc/sys/. It has nothing to do with sysfs. You're right about sysctl. ell used the same naming since it was meant to control net/ipv* kernel parameters. However, the same API can be used inside sysfs since the semantics are identical. If you don't like l_sysctl_get_driver, then lets call it l_sysfs_get_driver(). Regards, -Denis
Hi Denis, >>>> Add a _get_driver is a good idea as well, but what is the reason for l_sysctl since sysctl is some specific system call. >>> >>> l_sysctl is used to write stuff into procfs or sysfs. Naming comes from the sysctl(8) utility. >> actually no. sysctl just handles /proc/sys/. It has nothing to do with sysfs. > > You're right about sysctl. ell used the same naming since it was meant to control net/ipv* kernel parameters. However, the same API can be used inside sysfs since the semantics are identical. > > If you don't like l_sysctl_get_driver, then lets call it l_sysfs_get_driver(). yes, I would prefer l_sysfs_get_driver. Regards Marcel
On Wed, 14 Feb 2024 17:52:43 +0100 Marcel Holtmann <marcel@holtmann.org> wrote: > Hi Denis, > > >>>> Add a _get_driver is a good idea as well, but what is the reason for l_sysctl since sysctl is some specific system call. > >>> > >>> l_sysctl is used to write stuff into procfs or sysfs. Naming comes from the sysctl(8) utility. > >> actually no. sysctl just handles /proc/sys/. It has nothing to do with sysfs. > > > > You're right about sysctl. ell used the same naming since it was meant to control net/ipv* kernel parameters. However, the same API can be used inside sysfs since the semantics are identical. > > > > If you don't like l_sysctl_get_driver, then lets call it l_sysfs_get_driver(). > > yes, I would prefer l_sysfs_get_driver. Thanks Marcel & Denis for the replies. It's not clear to me what needs to be done to resolve the original issue I set out to fix with this patch. Could you please summarize this so I (or someone else) could try to implement it? -Clayton
Hi Clayton, > > Thanks Marcel & Denis for the replies. It's not clear to me what needs to be > done to resolve the original issue I set out to fix with this patch. Could you > please summarize this so I (or someone else) could try to implement it? Should already be taken care of by ba5a6df2d170 ("wiphy: Remove basename() use"). See [1]. Part of iwd 2.15 release. Regards, -Denis [1] https://git.kernel.org/pub/scm/network/wireless/iwd.git/commit/?id=ba5a6df2d170fce96354f0c9ba281fe049d2f0a3
March 15, 2024 at 6:49 AM, "Denis Kenzior" <denkenz@gmail.com> wrote:
> Should already be taken care of by ba5a6df2d170 ("wiphy: Remove basename() use"). See [1]. Part of iwd 2.15 release.
Oh no, I'm sorry for missing this! Thanks for fixing it :)
-Clayton
diff --git a/src/wiphy.c b/src/wiphy.c index 3258b761..9d9c6c22 100644 --- a/src/wiphy.c +++ b/src/wiphy.c @@ -33,6 +33,7 @@ #include <unistd.h> #include <string.h> #include <limits.h> +#include <libgen.h> #include <ell/ell.h>