[PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Clayton Craft]

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.

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(+)

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>
 
-- 
2.43.1

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Denis Kenzior]

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

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Clayton Craft]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Marcel Holtmann]

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

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Denis Kenzior]

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

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Marcel Holtmann]

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

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Denis Kenzior]

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

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Marcel Holtmann]

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

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Denis Kenzior]

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

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Marcel Holtmann]

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

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Clayton Craft]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Denis Kenzior]

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

Re: [PATCH] wiphy: include libgen.h to fix implicit def. with musl libc

[Clayton Craft]

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