Hi,
Explicitly CC'ing people involved for the e00ab3c0616f ("fsidd:
provide better default socket name.") change:
On Sun, Sep 03, 2023 at 05:21:52PM +0200, Ahelenia Ziemiańska wrote:
> Since e00ab3c0616fe6d83ab0710d9e7d989c299088f7, ss -l looks like this:
> u_seq LISTEN 0 5 @/run/fsid.sock@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ 26989379 * 0
> with fsidd pushing all the addresses to 108 bytes wide, which is deeply
> egregious if you don't filter it out and recolumnate.
>
> This is because, naturally (unix(7)), "Null bytes in the name have
> no special significance": abstract addresses are binary blobs, but
> paths automatically terminate at the first NUL byte, since paths
> can't contain those.
>
> So just specify the correct address length when we're using the abstract domain:
> unix(7) recommends "offsetof(struct sockaddr_un, sun_path) + strlen(sun_path) + 1"
> for paths, but we don't want to include the terminating NUL, so it's just
> "offsetof(struct sockaddr_un, sun_path) + strlen(sun_path)".
> This brings the width back to order:
> -- >8 --
> $ ss -la | grep @
> u_str ESTAB 0 0 @45208536ec96909a/bus/systemd-timesyn/bus-api-timesync 18500238 * 18501249
> u_str ESTAB 0 0 @fecc9657d2315eb7/bus/systemd-network/bus-api-network 18495452 * 18494406
> u_seq LISTEN 0 5 @/run/fsid.sock 27168796 * 0
> u_str ESTAB 0 0 @ac308f35f50797a2/bus/systemd-logind/system 19406 * 15153
> u_str ESTAB 0 0 @b6606e0dfacbae75/bus/systemd/bus-api-system 18494353 * 18495334
> u_str ESTAB 0 0 @5880653d215718a7/bus/systemd/bus-system 26930876 * 26930003
> -- >8 --
>
> Fixes: e00ab3c0616fe6d83ab0710d9e7d989c299088f7 ("fsidd: provide
> better default socket name.")
> Signed-off-by: Ahelenia Ziemiańska <[email protected]>
> ---
> support/reexport/fsidd.c | 8 +++++---
> support/reexport/reexport.c | 7 +++++--
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c
> index d4b245e8..4c377415 100644
> --- a/support/reexport/fsidd.c
> +++ b/support/reexport/fsidd.c
> @@ -171,10 +171,12 @@ int main(void)
> memset(&addr, 0, sizeof(struct sockaddr_un));
> addr.sun_family = AF_UNIX;
> strncpy(addr.sun_path, sock_file, sizeof(addr.sun_path) - 1);
> - if (addr.sun_path[0] == '@')
> + socklen_t addr_len = sizeof(struct sockaddr_un);
> + if (addr.sun_path[0] == '@') {
> /* "abstract" socket namespace */
> + addr_len = offsetof(struct sockaddr_un, sun_path) + strlen(addr.sun_path);
> addr.sun_path[0] = 0;
> - else
> + } else
> unlink(sock_file);
>
> srv = socket(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK, 0);
> @@ -183,7 +185,7 @@ int main(void)
> return 1;
> }
>
> - if (bind(srv, (const struct sockaddr *)&addr, sizeof(struct sockaddr_un)) == -1) {
> + if (bind(srv, (const struct sockaddr *)&addr, addr_len) == -1) {
> xlog(L_WARNING, "Unable to bind %s: %m\n", sock_file);
> return 1;
> }
> diff --git a/support/reexport/reexport.c b/support/reexport/reexport.c
> index d9a700af..b7ee6f46 100644
> --- a/support/reexport/reexport.c
> +++ b/support/reexport/reexport.c
> @@ -40,9 +40,12 @@ static bool connect_fsid_service(void)
> memset(&addr, 0, sizeof(struct sockaddr_un));
> addr.sun_family = AF_UNIX;
> strncpy(addr.sun_path, sock_file, sizeof(addr.sun_path) - 1);
> - if (addr.sun_path[0] == '@')
> + socklen_t addr_len = sizeof(struct sockaddr_un);
> + if (addr.sun_path[0] == '@') {
> /* "abstract" socket namespace */
> + addr_len = offsetof(struct sockaddr_un, sun_path) + strlen(addr.sun_path);
> addr.sun_path[0] = 0;
> + }
>
> s = socket(AF_UNIX, SOCK_SEQPACKET, 0);
> if (s == -1) {
> @@ -50,7 +53,7 @@ static bool connect_fsid_service(void)
> return false;
> }
>
> - ret = connect(s, (const struct sockaddr *)&addr, sizeof(struct sockaddr_un));
> + ret = connect(s, (const struct sockaddr *)&addr, addr_len);
> if (ret == -1) {
> xlog(L_WARNING, "Unable to connect %s: %m, is fsidd running?\n", sock_file);
> return false;
> --
> 2.40.1
Did this one felt trough the cracks?
Regards,
Salvatore
----- Ursprüngliche Mail -----
> Von: "Salvatore Bonaccorso" <[email protected]>
> An: "NeilBrown" <[email protected]>, "richard" <[email protected]>, "Steve Dickson" <[email protected]>
> CC: "Ahelenia Ziemiańska" <[email protected]>, "linux-nfs" <[email protected]>
> Gesendet: Dienstag, 21. November 2023 20:58:45
> Betreff: Re: [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes
> Hi,
>
> Explicitly CC'ing people involved for the e00ab3c0616f ("fsidd:
> provide better default socket name.") change:
>
> On Sun, Sep 03, 2023 at 05:21:52PM +0200, Ahelenia Ziemiańska wrote:
>> Since e00ab3c0616fe6d83ab0710d9e7d989c299088f7, ss -l looks like this:
>> u_seq LISTEN 0 5
>> @/run/fsid.sock@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>> 26989379 * 0
>> with fsidd pushing all the addresses to 108 bytes wide, which is deeply
>> egregious if you don't filter it out and recolumnate.
>>
>> This is because, naturally (unix(7)), "Null bytes in the name have
>> no special significance": abstract addresses are binary blobs, but
>> paths automatically terminate at the first NUL byte, since paths
>> can't contain those.
>>
>> So just specify the correct address length when we're using the abstract domain:
>> unix(7) recommends "offsetof(struct sockaddr_un, sun_path) + strlen(sun_path) +
>> 1"
>> for paths, but we don't want to include the terminating NUL, so it's just
>> "offsetof(struct sockaddr_un, sun_path) + strlen(sun_path)".
>> This brings the width back to order:
>> -- >8 --
>> $ ss -la | grep @
>> u_str ESTAB 0 0
>> @45208536ec96909a/bus/systemd-timesyn/bus-api-timesync 18500238
>> * 18501249
>> u_str ESTAB 0 0
>> @fecc9657d2315eb7/bus/systemd-network/bus-api-network 18495452
>> * 18494406
>> u_seq LISTEN 0 5
>> @/run/fsid.sock 27168796
>> * 0
>> u_str ESTAB 0 0
>> @ac308f35f50797a2/bus/systemd-logind/system 19406
>> * 15153
>> u_str ESTAB 0 0
>> @b6606e0dfacbae75/bus/systemd/bus-api-system 18494353
>> * 18495334
>> u_str ESTAB 0 0
>> @5880653d215718a7/bus/systemd/bus-system 26930876
>> * 26930003
>> -- >8 --
>>
>> Fixes: e00ab3c0616fe6d83ab0710d9e7d989c299088f7 ("fsidd: provide
>> better default socket name.")
>> Signed-off-by: Ahelenia Ziemiańska <[email protected]>
>> ---
>> support/reexport/fsidd.c | 8 +++++---
>> support/reexport/reexport.c | 7 +++++--
>> 2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c
>> index d4b245e8..4c377415 100644
>> --- a/support/reexport/fsidd.c
>> +++ b/support/reexport/fsidd.c
>> @@ -171,10 +171,12 @@ int main(void)
>> memset(&addr, 0, sizeof(struct sockaddr_un));
>> addr.sun_family = AF_UNIX;
>> strncpy(addr.sun_path, sock_file, sizeof(addr.sun_path) - 1);
>> - if (addr.sun_path[0] == '@')
>> + socklen_t addr_len = sizeof(struct sockaddr_un);
>> + if (addr.sun_path[0] == '@') {
>> /* "abstract" socket namespace */
>> + addr_len = offsetof(struct sockaddr_un, sun_path) + strlen(addr.sun_path);
>> addr.sun_path[0] = 0;
>> - else
>> + } else
>> unlink(sock_file);
>>
>> srv = socket(AF_UNIX, SOCK_SEQPACKET | SOCK_NONBLOCK, 0);
>> @@ -183,7 +185,7 @@ int main(void)
>> return 1;
>> }
>>
>> - if (bind(srv, (const struct sockaddr *)&addr, sizeof(struct sockaddr_un)) ==
>> -1) {
>> + if (bind(srv, (const struct sockaddr *)&addr, addr_len) == -1) {
>> xlog(L_WARNING, "Unable to bind %s: %m\n", sock_file);
>> return 1;
>> }
>> diff --git a/support/reexport/reexport.c b/support/reexport/reexport.c
>> index d9a700af..b7ee6f46 100644
>> --- a/support/reexport/reexport.c
>> +++ b/support/reexport/reexport.c
>> @@ -40,9 +40,12 @@ static bool connect_fsid_service(void)
>> memset(&addr, 0, sizeof(struct sockaddr_un));
>> addr.sun_family = AF_UNIX;
>> strncpy(addr.sun_path, sock_file, sizeof(addr.sun_path) - 1);
>> - if (addr.sun_path[0] == '@')
>> + socklen_t addr_len = sizeof(struct sockaddr_un);
>> + if (addr.sun_path[0] == '@') {
>> /* "abstract" socket namespace */
>> + addr_len = offsetof(struct sockaddr_un, sun_path) + strlen(addr.sun_path);
>> addr.sun_path[0] = 0;
>> + }
>>
>> s = socket(AF_UNIX, SOCK_SEQPACKET, 0);
>> if (s == -1) {
>> @@ -50,7 +53,7 @@ static bool connect_fsid_service(void)
>> return false;
>> }
>>
>> - ret = connect(s, (const struct sockaddr *)&addr, sizeof(struct sockaddr_un));
>> + ret = connect(s, (const struct sockaddr *)&addr, addr_len);
>> if (ret == -1) {
>> xlog(L_WARNING, "Unable to connect %s: %m, is fsidd running?\n", sock_file);
>> return false;
>> --
>> 2.40.1
>
> Did this one felt trough the cracks?
At least it never hit my inbox.
Change looks good to me.
Thanks,
//richard