| Message ID | b38ecca96762d939d377c381bf34521ee5945129.1700601199.git.nabijaczleweli@nabijaczleweli.xyz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [nfs-utils,v2,1/2] fsidd: call anonymous sockets by their name only, don't fill with NULs to 108 bytes | expand |
On Wed, 22 Nov 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> > --- > v1: <04f3fe71defa757d518468f04f08334a5d0dfbb9.1693754442.git.nabijaczleweli@nabijaczleweli.xyz> > v2 NFC, addr_len declared at top of function > > support/reexport/fsidd.c | 9 ++++++--- > support/reexport/reexport.c | 8 ++++++-- > 2 files changed, 12 insertions(+), 5 deletions(-) Reviewed-by: NeilBrown <neilb@suse.de> Thanks, NeilBrown > > diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c > index 3e62b3fc..8a70b78f 100644 > --- a/support/reexport/fsidd.c > +++ b/support/reexport/fsidd.c > @@ -147,6 +147,7 @@ int main(void) > { > struct event *srv_ev; > struct sockaddr_un addr; > + socklen_t addr_len; > char *sock_file; > int srv; > > @@ -161,10 +162,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] == '@') > + 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); > @@ -173,7 +176,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 78516586..0fb49a46 100644 > --- a/support/reexport/reexport.c > +++ b/support/reexport/reexport.c > @@ -21,6 +21,7 @@ static int fsidd_srv = -1; > static bool connect_fsid_service(void) > { > struct sockaddr_un addr; > + socklen_t addr_len; > char *sock_file; > int ret; > int s; > @@ -33,9 +34,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] == '@') > + 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) { > @@ -43,7 +47,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.39.2 > >
On 11/21/23 4:14 PM, 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> Committed... (tag: nfs-utils-2-7-1-rc1) steved > --- > v1: <04f3fe71defa757d518468f04f08334a5d0dfbb9.1693754442.git.nabijaczleweli@nabijaczleweli.xyz> > v2 NFC, addr_len declared at top of function > > support/reexport/fsidd.c | 9 ++++++--- > support/reexport/reexport.c | 8 ++++++-- > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c > index 3e62b3fc..8a70b78f 100644 > --- a/support/reexport/fsidd.c > +++ b/support/reexport/fsidd.c > @@ -147,6 +147,7 @@ int main(void) > { > struct event *srv_ev; > struct sockaddr_un addr; > + socklen_t addr_len; > char *sock_file; > int srv; > > @@ -161,10 +162,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] == '@') > + 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); > @@ -173,7 +176,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 78516586..0fb49a46 100644 > --- a/support/reexport/reexport.c > +++ b/support/reexport/reexport.c > @@ -21,6 +21,7 @@ static int fsidd_srv = -1; > static bool connect_fsid_service(void) > { > struct sockaddr_un addr; > + socklen_t addr_len; > char *sock_file; > int ret; > int s; > @@ -33,9 +34,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] == '@') > + 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) { > @@ -43,7 +47,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;
On Tue, Nov 21, 2023 at 4:15 PM Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> 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> > --- > v1: <04f3fe71defa757d518468f04f08334a5d0dfbb9.1693754442.git.nabijaczleweli@nabijaczleweli.xyz> > v2 NFC, addr_len declared at top of function > > support/reexport/fsidd.c | 9 ++++++--- > support/reexport/reexport.c | 8 ++++++-- > 2 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c > index 3e62b3fc..8a70b78f 100644 > --- a/support/reexport/fsidd.c > +++ b/support/reexport/fsidd.c > @@ -147,6 +147,7 @@ int main(void) > { > struct event *srv_ev; > struct sockaddr_un addr; > + socklen_t addr_len; > char *sock_file; > int srv; > > @@ -161,10 +162,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] == '@') > + 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); > @@ -173,7 +176,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 78516586..0fb49a46 100644 > --- a/support/reexport/reexport.c > +++ b/support/reexport/reexport.c > @@ -21,6 +21,7 @@ static int fsidd_srv = -1; > static bool connect_fsid_service(void) > { > struct sockaddr_un addr; > + socklen_t addr_len; > char *sock_file; > int ret; > int s; > @@ -33,9 +34,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] == '@') > + 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) { > @@ -43,7 +47,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.39.2 > Hi folks, I'm hitting the following compile error (in RHEL9.2). I believe the code is missing an include for stddef.h. When I add it, things compile. I'll submit a patch. gcc -DHAVE_CONFIG_H -I. -I../../support/include -I/usr/include/tirpc -D_GNU_SOURCE -pipe -Wall -Wextra -Werror=strict-prototypes -Werror=missing-prototypes -Werror=missing-declarations -Werror=format=2 -Werror=undef -Werror=missing-include-dirs -Werror=strict-aliasing=2 -Werror=init-self -Werror=implicit-function-declaration -Werror=return-type -Werror=switch -Werror=overflow -Werror=parentheses -Werror=aggregate-return -Werror=unused-result -fno-strict-aliasing -Werror=format-overflow=2 -Werror=int-conversion -Werror=incompatible-pointer-types -Werror=misleading-indentation -Wno-cast-function-type -g -O2 -MT reexport.o -MD -MP -MF .deps/reexport.Tpo -c -o reexport.o reexport.c reexport.c: In function ‘connect_fsid_service’: reexport.c:40:28: error: implicit declaration of function ‘offsetof’ [-Werror=implicit-function-declaration] 40 | addr_len = offsetof(struct sockaddr_un, sun_path) + strlen(addr.sun_path); | ^~~~~~~~ reexport.c:18:1: note: ‘offsetof’ is defined in header ‘<stddef.h>’; did you forget to ‘#include <stddef.h>’? 17 | #include "xlog.h" +++ |+#include <stddef.h> 18 | reexport.c:40:37: error: expected expression before ‘struct’ 40 | addr_len = offsetof(struct sockaddr_un, sun_path) + strlen(addr.sun_path); | ^~~~~~ cc1: some warnings being treated as errors make[2]: *** [Makefile:529: reexport.o] Error 1 make[2]: Leaving directory '/home/aglo/nfs-utils/support/reexport' make[1]: *** [Makefile:448: all-recursive] Error 1 make[1]: Leaving directory '/home/aglo/nfs-utils/support'
On Thu, 07 Dec 2023, Olga Kornievskaia wrote: > On Tue, Nov 21, 2023 at 4:15 PM Ahelenia Ziemiańska > <nabijaczleweli@nabijaczleweli.xyz> 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> > > --- > > v1: <04f3fe71defa757d518468f04f08334a5d0dfbb9.1693754442.git.nabijaczleweli@nabijaczleweli.xyz> > > v2 NFC, addr_len declared at top of function > > > > support/reexport/fsidd.c | 9 ++++++--- > > support/reexport/reexport.c | 8 ++++++-- > > 2 files changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c > > index 3e62b3fc..8a70b78f 100644 > > --- a/support/reexport/fsidd.c > > +++ b/support/reexport/fsidd.c > > @@ -147,6 +147,7 @@ int main(void) > > { > > struct event *srv_ev; > > struct sockaddr_un addr; > > + socklen_t addr_len; > > char *sock_file; > > int srv; > > > > @@ -161,10 +162,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] == '@') > > + 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); > > @@ -173,7 +176,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 78516586..0fb49a46 100644 > > --- a/support/reexport/reexport.c > > +++ b/support/reexport/reexport.c > > @@ -21,6 +21,7 @@ static int fsidd_srv = -1; > > static bool connect_fsid_service(void) > > { > > struct sockaddr_un addr; > > + socklen_t addr_len; > > char *sock_file; > > int ret; > > int s; > > @@ -33,9 +34,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] == '@') > > + 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) { > > @@ -43,7 +47,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.39.2 > > > > Hi folks, > > I'm hitting the following compile error (in RHEL9.2). I believe the > code is missing an include for stddef.h. When I add it, things > compile. I'll submit a patch. > > gcc -DHAVE_CONFIG_H -I. -I../../support/include -I/usr/include/tirpc > -D_GNU_SOURCE -pipe -Wall -Wextra -Werror=strict-prototypes > -Werror=missing-prototypes -Werror=missing-declarations > -Werror=format=2 -Werror=undef -Werror=missing-include-dirs > -Werror=strict-aliasing=2 -Werror=init-self > -Werror=implicit-function-declaration -Werror=return-type > -Werror=switch -Werror=overflow -Werror=parentheses > -Werror=aggregate-return -Werror=unused-result -fno-strict-aliasing > -Werror=format-overflow=2 -Werror=int-conversion > -Werror=incompatible-pointer-types -Werror=misleading-indentation > -Wno-cast-function-type -g -O2 -MT reexport.o -MD -MP -MF > .deps/reexport.Tpo -c -o reexport.o reexport.c > reexport.c: In function ‘connect_fsid_service’: > reexport.c:40:28: error: implicit declaration of function ‘offsetof’ > [-Werror=implicit-function-declaration] > 40 | addr_len = offsetof(struct sockaddr_un, > sun_path) + strlen(addr.sun_path); > | ^~~~~~~~ > reexport.c:18:1: note: ‘offsetof’ is defined in header ‘<stddef.h>’; > did you forget to ‘#include <stddef.h>’? > 17 | #include "xlog.h" > +++ |+#include <stddef.h> > 18 | > reexport.c:40:37: error: expected expression before ‘struct’ > 40 | addr_len = offsetof(struct sockaddr_un, > sun_path) + strlen(addr.sun_path); > | ^~~~~~ > cc1: some warnings being treated as errors > make[2]: *** [Makefile:529: reexport.o] Error 1 > make[2]: Leaving directory '/home/aglo/nfs-utils/support/reexport' > make[1]: *** [Makefile:448: all-recursive] Error 1 > make[1]: Leaving directory '/home/aglo/nfs-utils/support' > Interesting - in openSUSE, dlfcn.h includes stddef.h, which is why the compile works for me. Obviously we cannot rely on that. reexport.c and fsidd.c will both need stddef. Thanks! NeilBrown
On Thu, Dec 7, 2023 at 7:08 PM NeilBrown <neilb@suse.de> wrote: > > On Thu, 07 Dec 2023, Olga Kornievskaia wrote: > > On Tue, Nov 21, 2023 at 4:15 PM Ahelenia Ziemiańska > > <nabijaczleweli@nabijaczleweli.xyz> 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> > > > --- > > > v1: <04f3fe71defa757d518468f04f08334a5d0dfbb9.1693754442.git.nabijaczleweli@nabijaczleweli.xyz> > > > v2 NFC, addr_len declared at top of function > > > > > > support/reexport/fsidd.c | 9 ++++++--- > > > support/reexport/reexport.c | 8 ++++++-- > > > 2 files changed, 12 insertions(+), 5 deletions(-) > > > > > > diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c > > > index 3e62b3fc..8a70b78f 100644 > > > --- a/support/reexport/fsidd.c > > > +++ b/support/reexport/fsidd.c > > > @@ -147,6 +147,7 @@ int main(void) > > > { > > > struct event *srv_ev; > > > struct sockaddr_un addr; > > > + socklen_t addr_len; > > > char *sock_file; > > > int srv; > > > > > > @@ -161,10 +162,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] == '@') > > > + 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); > > > @@ -173,7 +176,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 78516586..0fb49a46 100644 > > > --- a/support/reexport/reexport.c > > > +++ b/support/reexport/reexport.c > > > @@ -21,6 +21,7 @@ static int fsidd_srv = -1; > > > static bool connect_fsid_service(void) > > > { > > > struct sockaddr_un addr; > > > + socklen_t addr_len; > > > char *sock_file; > > > int ret; > > > int s; > > > @@ -33,9 +34,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] == '@') > > > + 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) { > > > @@ -43,7 +47,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.39.2 > > > > > > > Hi folks, > > > > I'm hitting the following compile error (in RHEL9.2). I believe the > > code is missing an include for stddef.h. When I add it, things > > compile. I'll submit a patch. > > > > gcc -DHAVE_CONFIG_H -I. -I../../support/include -I/usr/include/tirpc > > -D_GNU_SOURCE -pipe -Wall -Wextra -Werror=strict-prototypes > > -Werror=missing-prototypes -Werror=missing-declarations > > -Werror=format=2 -Werror=undef -Werror=missing-include-dirs > > -Werror=strict-aliasing=2 -Werror=init-self > > -Werror=implicit-function-declaration -Werror=return-type > > -Werror=switch -Werror=overflow -Werror=parentheses > > -Werror=aggregate-return -Werror=unused-result -fno-strict-aliasing > > -Werror=format-overflow=2 -Werror=int-conversion > > -Werror=incompatible-pointer-types -Werror=misleading-indentation > > -Wno-cast-function-type -g -O2 -MT reexport.o -MD -MP -MF > > .deps/reexport.Tpo -c -o reexport.o reexport.c > > reexport.c: In function ‘connect_fsid_service’: > > reexport.c:40:28: error: implicit declaration of function ‘offsetof’ > > [-Werror=implicit-function-declaration] > > 40 | addr_len = offsetof(struct sockaddr_un, > > sun_path) + strlen(addr.sun_path); > > | ^~~~~~~~ > > reexport.c:18:1: note: ‘offsetof’ is defined in header ‘<stddef.h>’; > > did you forget to ‘#include <stddef.h>’? > > 17 | #include "xlog.h" > > +++ |+#include <stddef.h> > > 18 | > > reexport.c:40:37: error: expected expression before ‘struct’ > > 40 | addr_len = offsetof(struct sockaddr_un, > > sun_path) + strlen(addr.sun_path); > > | ^~~~~~ > > cc1: some warnings being treated as errors > > make[2]: *** [Makefile:529: reexport.o] Error 1 > > make[2]: Leaving directory '/home/aglo/nfs-utils/support/reexport' > > make[1]: *** [Makefile:448: all-recursive] Error 1 > > make[1]: Leaving directory '/home/aglo/nfs-utils/support' > > > > > Interesting - in openSUSE, dlfcn.h includes stddef.h, which is why the > compile works for me. Obviously we cannot rely on that. > > reexport.c and fsidd.c will both need stddef. Hm. ok once I added to reexport.c things recompiled for me. so my patch only covered that. I can write a v2 of the patch I submitted to include both. > > Thanks! > > NeilBrown
diff --git a/support/reexport/fsidd.c b/support/reexport/fsidd.c index 3e62b3fc..8a70b78f 100644 --- a/support/reexport/fsidd.c +++ b/support/reexport/fsidd.c @@ -147,6 +147,7 @@ int main(void) { struct event *srv_ev; struct sockaddr_un addr; + socklen_t addr_len; char *sock_file; int srv; @@ -161,10 +162,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] == '@') + 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); @@ -173,7 +176,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 78516586..0fb49a46 100644 --- a/support/reexport/reexport.c +++ b/support/reexport/reexport.c @@ -21,6 +21,7 @@ static int fsidd_srv = -1; static bool connect_fsid_service(void) { struct sockaddr_un addr; + socklen_t addr_len; char *sock_file; int ret; int s; @@ -33,9 +34,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] == '@') + 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) { @@ -43,7 +47,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;
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> --- v1: <04f3fe71defa757d518468f04f08334a5d0dfbb9.1693754442.git.nabijaczleweli@nabijaczleweli.xyz> v2 NFC, addr_len declared at top of function support/reexport/fsidd.c | 9 ++++++--- support/reexport/reexport.c | 8 ++++++-- 2 files changed, 12 insertions(+), 5 deletions(-)