Linux NFS development
 help / color / mirror / Atom feed
* [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes
@ 2023-09-03 15:21 Ahelenia Ziemiańska
  2023-09-03 15:22 ` [PATCH nfs-utils 2/2] testlk: format off_t as llong instead of ssize_t Ahelenia Ziemiańska
  2023-11-21 19:58 ` [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes Salvatore Bonaccorso
  0 siblings, 2 replies; 5+ messages in thread
From: Ahelenia Ziemiańska @ 2023-09-03 15:21 UTC (permalink / raw)
  To: linux-nfs

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

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 <nabijaczleweli@nabijaczleweli.xyz>
---
 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


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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH nfs-utils 2/2] testlk: format off_t as llong instead of ssize_t
  2023-09-03 15:21 [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes Ahelenia Ziemiańska
@ 2023-09-03 15:22 ` Ahelenia Ziemiańska
  2023-11-21 19:58 ` [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes Salvatore Bonaccorso
  1 sibling, 0 replies; 5+ messages in thread
From: Ahelenia Ziemiańska @ 2023-09-03 15:22 UTC (permalink / raw)
  To: linux-nfs

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

This, naturally, produces a warning on x32 (and other ILP32 platforms
with 64-bit off_t, presumably, but you need to ask for it explicitly
there usually):
gcc -DHAVE_CONFIG_H -I. -I../../support/include  -D_GNU_SOURCE -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -g -O2 -ffile-prefix-map=/tmp/nfs-utils-2.6.3=. -specs=/usr/share/dpkg/pie-compile.specs -fstack-protector-strong -Wformat -Werror=format-security -g -O2 -ffile-prefix-map=/tmp/nfs-utils-2.6.3=. -specs=/usr/share/dpkg/pie-compile.specs -fstack-protector-strong -Wformat -Werror=format-security -c -o testlk-testlk.o `test -f 'testlk.c' || echo './'`testlk.c
testlk.c: In function ‘main’:
testlk.c:84:66: warning: format ‘%zd’ expects argument of type ‘signed size_t’, but argument 4 has type ‘__off_t’ {aka ‘long long int’} [-Wformat=]
   84 |                         printf("%s: conflicting lock by %d on (%zd;%zd)\n",
      |                                                                ~~^
      |                                                                  |
      |                                                                  int
      |                                                                %lld
   85 |                                 fname, fl.l_pid, fl.l_start, fl.l_len);
      |                                                  ~~~~~~~~~~
      |                                                    |
      |                                                    __off_t {aka long long int}
testlk.c:84:70: warning: format ‘%zd’ expects argument of type ‘signed size_t’, but argument 5 has type ‘__off_t’ {aka ‘long long int’} [-Wformat=]
   84 |                         printf("%s: conflicting lock by %d on (%zd;%zd)\n",
      |                                                                    ~~^
      |                                                                      |
      |                                                                      int
      |                                                                    %lld
   85 |                                 fname, fl.l_pid, fl.l_start, fl.l_len);
      |                                                              ~~~~~~~~
      |                                                                |
      |                                                                __off_t {aka long long int}

Upcast to long long, doesn't really matter.

It does, of course, raise the question of whether other bits of
nfs-utils do something equally broken that just isn't caught by the
format validator.

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 tools/locktest/testlk.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/locktest/testlk.c b/tools/locktest/testlk.c
index ea51f788..c9bd6bac 100644
--- a/tools/locktest/testlk.c
+++ b/tools/locktest/testlk.c
@@ -81,8 +81,8 @@ main(int argc, char **argv)
 		if (fl.l_type == F_UNLCK) {
 			printf("%s: no conflicting lock\n", fname);
 		} else {
-			printf("%s: conflicting lock by %d on (%zd;%zd)\n",
-				fname, fl.l_pid, fl.l_start, fl.l_len);
+			printf("%s: conflicting lock by %d on (%lld;%lld)\n",
+				fname, fl.l_pid, (long long)fl.l_start, (long long)fl.l_len);
 		}
 		return 0;
 	}
-- 
2.40.1

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes
  2023-09-03 15:21 [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes Ahelenia Ziemiańska
  2023-09-03 15:22 ` [PATCH nfs-utils 2/2] testlk: format off_t as llong instead of ssize_t Ahelenia Ziemiańska
@ 2023-11-21 19:58 ` Salvatore Bonaccorso
  2023-11-21 20:41   ` Richard Weinberger
  1 sibling, 1 reply; 5+ messages in thread
From: Salvatore Bonaccorso @ 2023-11-21 19:58 UTC (permalink / raw)
  To: NeilBrown, Richard Weinberger, Steve Dickson
  Cc: Ahelenia Ziemiańska, linux-nfs

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 <nabijaczleweli@nabijaczleweli.xyz>
> ---
>  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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes
  2023-11-21 19:58 ` [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes Salvatore Bonaccorso
@ 2023-11-21 20:41   ` Richard Weinberger
  0 siblings, 0 replies; 5+ messages in thread
From: Richard Weinberger @ 2023-11-21 20:41 UTC (permalink / raw)
  To: Salvatore Bonaccorso
  Cc: NeilBrown, Steve Dickson, Ahelenia Ziemiańska, linux-nfs

----- Ursprüngliche Mail -----
> Von: "Salvatore Bonaccorso" <carnil@debian.org>
> An: "NeilBrown" <neilb@suse.de>, "richard" <richard@nod.at>, "Steve Dickson" <steved@redhat.com>
> CC: "Ahelenia Ziemiańska" <nabijaczleweli@nabijaczleweli.xyz>, "linux-nfs" <linux-nfs@vger.kernel.org>
> 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 <nabijaczleweli@nabijaczleweli.xyz>
>> ---
>>  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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes
@ 2023-11-21 20:49 NeilBrown
  0 siblings, 0 replies; 5+ messages in thread
From: NeilBrown @ 2023-11-21 20:49 UTC (permalink / raw)
  To: Ahelenia Ziemiańska; +Cc: linux-nfs, Salvatore Bonaccorso

On Mon, 04 Sep 2023, 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 <nabijaczleweli@nabijaczleweli.xyz>
> ---
>  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);

Could you please move the declaration of addr_len up to the top of the
block - for consistency with the rest of the code.

Then resend to the list, and to Steved and me?

Thanks,
NeilBrown


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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-11-21 20:49 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-03 15:21 [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes Ahelenia Ziemiańska
2023-09-03 15:22 ` [PATCH nfs-utils 2/2] testlk: format off_t as llong instead of ssize_t Ahelenia Ziemiańska
2023-11-21 19:58 ` [PATCH nfs-utils 1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes Salvatore Bonaccorso
2023-11-21 20:41   ` Richard Weinberger
  -- strict thread matches above, loose matches on Subject: below --
2023-11-21 20:49 NeilBrown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox