Re: userspace doesn't need to set sa_len, sun_len, etc
On Sun, Jan 22, 2017 at 07:07:28AM +1000, Philip Guenther wrote:
> On Sun, 22 Jan 2017, Martin Pieuchot wrote:
> > On 21/01/17(Sat) 20:40, Philip Guenther wrote:
> > >
> > > This is your periodic reminder that it's pointless for userspace to set
> > > the {sun,sin,sin6,etc}_len field in sockaddrs being passed to the POSIX
> > > APIs. An application can use it internally for its own purposes, but the
> > > kernel always overwrites the value when passed in.
> >
> > I'd say explicit is better than implicit. Is there any bug in the code
> > below?
> >
> > I believe it is simpler to always set the length, it makes our life easier
> > when review code between kernel and userland. Or should we have two
> > different practices because history screwed up?
>
> Let's something that has no effect, just for visual consistency? When the
> value there is ignored, what reason is there to believe the correct value
> is being used?
>
> Meanwhile, new code coming in from elsewhere is unlikely to set it, as
> that field doesn't exist on non-BSDs, so we either (a) add ignored
> assignments, or (b) be inconsistent inside userland. Oh, and -portable
> projects have to patch out use of it.
>
> I would rather have userland be consistent with itself and inconsistent
> with the kernel, than try for an incomplete consistency between userland
> and the kernel that spreads the inconsistency inside userland.
>
> Kernel and userland best practices are already quite different, including
> this in that list is the lesser evil.
>
I would love to remove this from smtpd to reduce diff with portable,
so i'm generally ok with the idea
--
Gilles Chehade
https://www.poolp.org @poolpOrg
Re: userspace doesn't need to set sa_len, sun_len, etc
On Sun, 22 Jan 2017, Martin Pieuchot wrote:
> On 21/01/17(Sat) 20:40, Philip Guenther wrote:
> >
> > This is your periodic reminder that it's pointless for userspace to set
> > the {sun,sin,sin6,etc}_len field in sockaddrs being passed to the POSIX
> > APIs. An application can use it internally for its own purposes, but the
> > kernel always overwrites the value when passed in.
>
> I'd say explicit is better than implicit. Is there any bug in the code
> below?
>
> I believe it is simpler to always set the length, it makes our life easier
> when review code between kernel and userland. Or should we have two
> different practices because history screwed up?
Let's something that has no effect, just for visual consistency? When the
value there is ignored, what reason is there to believe the correct value
is being used?
Meanwhile, new code coming in from elsewhere is unlikely to set it, as
that field doesn't exist on non-BSDs, so we either (a) add ignored
assignments, or (b) be inconsistent inside userland. Oh, and -portable
projects have to patch out use of it.
I would rather have userland be consistent with itself and inconsistent
with the kernel, than try for an incomplete consistency between userland
and the kernel that spreads the inconsistency inside userland.
Kernel and userland best practices are already quite different, including
this in that list is the lesser evil.
Philip
Re: userspace doesn't need to set sa_len, sun_len, etc
On 21/01/17(Sat) 20:40, Philip Guenther wrote:
>
> This is your periodic reminder that it's pointless for userspace to set
> the {sun,sin,sin6,etc}_len field in sockaddrs being passed to the POSIX
> APIs. An application can use it internally for its own purposes, but the
> kernel always overwrites the value when passed in.
I'd say explicit is better than implicit. Is there any bug in the code
below?
I believe it is simpler to always set the length, it makes our life easier
when review code between kernel and userland. Or should we have two
different practices because history screwed up?
>
> There are more of these that can be cleaned up, but here's the chunk I've
> had sitting in my tree.
>
> oks for any of these?
>
>
> Philip
>
>
> Index: usr.bin/netstat/inet6.c
> ===
> RCS file: /data/src/openbsd/src/usr.bin/netstat/inet6.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 inet6.c
> --- usr.bin/netstat/inet6.c 22 Dec 2016 11:04:44 - 1.51
> +++ usr.bin/netstat/inet6.c 23 Dec 2016 22:55:54 -
> @@ -983,7 +983,6 @@ inet6name(struct in6_addr *in6p)
> strlcpy(line, cp, sizeof(line));
> else {
> memset(, 0, sizeof(sin6));
> - sin6.sin6_len = sizeof(sin6);
> sin6.sin6_family = AF_INET6;
> sin6.sin6_addr = *in6p;
> #ifdef __KAME__
> @@ -996,7 +995,7 @@ inet6name(struct in6_addr *in6p)
> sin6.sin6_addr.s6_addr[3] = 0;
> }
> #endif
> - if (getnameinfo((struct sockaddr *), sin6.sin6_len,
> + if (getnameinfo((struct sockaddr *), sizeof(sin6),
> hbuf, sizeof(hbuf), NULL, 0, niflag) != 0)
> strlcpy(hbuf, "?", sizeof hbuf);
> strlcpy(line, hbuf, sizeof(line));
> Index: usr.bin/netstat/show.c
> ===
> RCS file: /data/src/openbsd/src/usr.bin/netstat/show.c,v
> retrieving revision 1.52
> diff -u -p -r1.52 show.c
> --- usr.bin/netstat/show.c3 Sep 2016 14:23:14 - 1.52
> +++ usr.bin/netstat/show.c3 Sep 2016 14:23:32 -
> @@ -449,7 +449,6 @@ routename(struct sockaddr *sa)
>
> memset(, 0, sizeof(sin6));
> memcpy(, sa, sa->sa_len);
> - sin6.sin6_len = sizeof(struct sockaddr_in6);
> sin6.sin6_family = AF_INET6;
> if (sa->sa_len == sizeof(struct sockaddr_in6) &&
> (IN6_IS_ADDR_LINKLOCAL(_addr) ||
> @@ -520,7 +519,7 @@ routename6(struct sockaddr_in6 *sin6)
> else
> niflags |= NI_NOFQDN;
>
> - if (getnameinfo((struct sockaddr *)sin6, sin6->sin6_len,
> + if (getnameinfo((struct sockaddr *)sin6, sizeof(*sin6),
> line, sizeof(line), NULL, 0, niflags) != 0)
> strncpy(line, "invalid", sizeof(line));
>
> @@ -640,7 +639,7 @@ netname6(struct sockaddr_in6 *sa6, struc
>
> if (nflag)
> flag |= NI_NUMERICHOST;
> - error = getnameinfo((struct sockaddr *), sin6.sin6_len,
> + error = getnameinfo((struct sockaddr *), sizeof(sin6),
> hbuf, sizeof(hbuf), NULL, 0, flag);
> if (error)
> snprintf(hbuf, sizeof(hbuf), "invalid");
> Index: usr.bin/rusers/rusers.c
> ===
> RCS file: /data/src/openbsd/src/usr.bin/rusers/rusers.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 rusers.c
> --- usr.bin/rusers/rusers.c 5 Aug 2016 10:34:18 - 1.39
> +++ usr.bin/rusers/rusers.c 6 Aug 2016 18:41:08 -
> @@ -418,7 +418,7 @@ retry:
> msgp->acpted_rply.ar_results.where = (caddr_t)resp;
> msgp->acpted_rply.ar_results.proc = xdr_rmtcallres;
>
> - fromlen = sizeof(struct sockaddr);
> + fromlen = sizeof(raddr);
> inlen = recvfrom(sock, inbuf, sizeof(inbuf), 0,
> (struct sockaddr *), );
> if (inlen < 0) {
> @@ -534,7 +534,6 @@ allhosts(void)
> outlen[1] = xdr_getpos();
> xdr_destroy();
>
> - baddr.sin_len = sizeof(struct sockaddr_in);
> baddr.sin_family = AF_INET;
> baddr.sin_port = htons(PMAPPORT);
> baddr.sin_addr.s_addr = htonl(INADDR_ANY);
> @@ -572,12 +571,12 @@ allhosts(void)
> if (i & 1) {
> if (sendto(sock[0], buf[0], outlen[0], 0,
> (struct sockaddr *),
> - sizeof(struct sockaddr)) != outlen[0])
> + sizeof(baddr)) != outlen[0])
> err(1, "can't send broadcast packet");
> } else {
> if (sendto(sock[1], buf[1], outlen[1], 0,
> (struct sockaddr *),
> - sizeof(struct sockaddr)) != outlen[1])
> + sizeof(baddr)) !=
Re: userspace doesn't need to set sa_len, sun_len, etc
Philip Guenther(guent...@gmail.com) on 2017.01.21 20:40:53 +1000:
>
> This is your periodic reminder that it's pointless for userspace to set
> the {sun,sin,sin6,etc}_len field in sockaddrs being passed to the POSIX
> APIs. An application can use it internally for its own purposes, but the
> kernel always overwrites the value when passed in.
>
> There are more of these that can be cleaned up, but here's the chunk I've
> had sitting in my tree.
>
> oks for any of these?
ok except for ssh because of -portability
also, check if netstat/show.c applies to route/show.c
>
> Philip
>
>
> Index: usr.bin/netstat/inet6.c
> ===
> RCS file: /data/src/openbsd/src/usr.bin/netstat/inet6.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 inet6.c
> --- usr.bin/netstat/inet6.c 22 Dec 2016 11:04:44 - 1.51
> +++ usr.bin/netstat/inet6.c 23 Dec 2016 22:55:54 -
> @@ -983,7 +983,6 @@ inet6name(struct in6_addr *in6p)
> strlcpy(line, cp, sizeof(line));
> else {
> memset(, 0, sizeof(sin6));
> - sin6.sin6_len = sizeof(sin6);
> sin6.sin6_family = AF_INET6;
> sin6.sin6_addr = *in6p;
> #ifdef __KAME__
> @@ -996,7 +995,7 @@ inet6name(struct in6_addr *in6p)
> sin6.sin6_addr.s6_addr[3] = 0;
> }
> #endif
> - if (getnameinfo((struct sockaddr *), sin6.sin6_len,
> + if (getnameinfo((struct sockaddr *), sizeof(sin6),
> hbuf, sizeof(hbuf), NULL, 0, niflag) != 0)
> strlcpy(hbuf, "?", sizeof hbuf);
> strlcpy(line, hbuf, sizeof(line));
> Index: usr.bin/netstat/show.c
> ===
> RCS file: /data/src/openbsd/src/usr.bin/netstat/show.c,v
> retrieving revision 1.52
> diff -u -p -r1.52 show.c
> --- usr.bin/netstat/show.c3 Sep 2016 14:23:14 - 1.52
> +++ usr.bin/netstat/show.c3 Sep 2016 14:23:32 -
> @@ -449,7 +449,6 @@ routename(struct sockaddr *sa)
>
> memset(, 0, sizeof(sin6));
> memcpy(, sa, sa->sa_len);
> - sin6.sin6_len = sizeof(struct sockaddr_in6);
> sin6.sin6_family = AF_INET6;
> if (sa->sa_len == sizeof(struct sockaddr_in6) &&
> (IN6_IS_ADDR_LINKLOCAL(_addr) ||
> @@ -520,7 +519,7 @@ routename6(struct sockaddr_in6 *sin6)
> else
> niflags |= NI_NOFQDN;
>
> - if (getnameinfo((struct sockaddr *)sin6, sin6->sin6_len,
> + if (getnameinfo((struct sockaddr *)sin6, sizeof(*sin6),
> line, sizeof(line), NULL, 0, niflags) != 0)
> strncpy(line, "invalid", sizeof(line));
>
> @@ -640,7 +639,7 @@ netname6(struct sockaddr_in6 *sa6, struc
>
> if (nflag)
> flag |= NI_NUMERICHOST;
> - error = getnameinfo((struct sockaddr *), sin6.sin6_len,
> + error = getnameinfo((struct sockaddr *), sizeof(sin6),
> hbuf, sizeof(hbuf), NULL, 0, flag);
> if (error)
> snprintf(hbuf, sizeof(hbuf), "invalid");
> Index: usr.bin/rusers/rusers.c
> ===
> RCS file: /data/src/openbsd/src/usr.bin/rusers/rusers.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 rusers.c
> --- usr.bin/rusers/rusers.c 5 Aug 2016 10:34:18 - 1.39
> +++ usr.bin/rusers/rusers.c 6 Aug 2016 18:41:08 -
> @@ -418,7 +418,7 @@ retry:
> msgp->acpted_rply.ar_results.where = (caddr_t)resp;
> msgp->acpted_rply.ar_results.proc = xdr_rmtcallres;
>
> - fromlen = sizeof(struct sockaddr);
> + fromlen = sizeof(raddr);
> inlen = recvfrom(sock, inbuf, sizeof(inbuf), 0,
> (struct sockaddr *), );
> if (inlen < 0) {
> @@ -534,7 +534,6 @@ allhosts(void)
> outlen[1] = xdr_getpos();
> xdr_destroy();
>
> - baddr.sin_len = sizeof(struct sockaddr_in);
> baddr.sin_family = AF_INET;
> baddr.sin_port = htons(PMAPPORT);
> baddr.sin_addr.s_addr = htonl(INADDR_ANY);
> @@ -572,12 +571,12 @@ allhosts(void)
> if (i & 1) {
> if (sendto(sock[0], buf[0], outlen[0], 0,
> (struct sockaddr *),
> - sizeof(struct sockaddr)) != outlen[0])
> + sizeof(baddr)) != outlen[0])
> err(1, "can't send broadcast packet");
> } else {
> if (sendto(sock[1], buf[1], outlen[1], 0,
> (struct sockaddr *),
> - sizeof(struct sockaddr)) != outlen[1])
> + sizeof(baddr)) != outlen[1])
> err(1, "can't send broadcast packet");
> }
> }
> Index:
userspace doesn't need to set sa_len, sun_len, etc
This is your periodic reminder that it's pointless for userspace to set
the {sun,sin,sin6,etc}_len field in sockaddrs being passed to the POSIX
APIs. An application can use it internally for its own purposes, but the
kernel always overwrites the value when passed in.
There are more of these that can be cleaned up, but here's the chunk I've
had sitting in my tree.
oks for any of these?
Philip
Index: usr.bin/netstat/inet6.c
===
RCS file: /data/src/openbsd/src/usr.bin/netstat/inet6.c,v
retrieving revision 1.51
diff -u -p -r1.51 inet6.c
--- usr.bin/netstat/inet6.c 22 Dec 2016 11:04:44 - 1.51
+++ usr.bin/netstat/inet6.c 23 Dec 2016 22:55:54 -
@@ -983,7 +983,6 @@ inet6name(struct in6_addr *in6p)
strlcpy(line, cp, sizeof(line));
else {
memset(, 0, sizeof(sin6));
- sin6.sin6_len = sizeof(sin6);
sin6.sin6_family = AF_INET6;
sin6.sin6_addr = *in6p;
#ifdef __KAME__
@@ -996,7 +995,7 @@ inet6name(struct in6_addr *in6p)
sin6.sin6_addr.s6_addr[3] = 0;
}
#endif
- if (getnameinfo((struct sockaddr *), sin6.sin6_len,
+ if (getnameinfo((struct sockaddr *), sizeof(sin6),
hbuf, sizeof(hbuf), NULL, 0, niflag) != 0)
strlcpy(hbuf, "?", sizeof hbuf);
strlcpy(line, hbuf, sizeof(line));
Index: usr.bin/netstat/show.c
===
RCS file: /data/src/openbsd/src/usr.bin/netstat/show.c,v
retrieving revision 1.52
diff -u -p -r1.52 show.c
--- usr.bin/netstat/show.c 3 Sep 2016 14:23:14 - 1.52
+++ usr.bin/netstat/show.c 3 Sep 2016 14:23:32 -
@@ -449,7 +449,6 @@ routename(struct sockaddr *sa)
memset(, 0, sizeof(sin6));
memcpy(, sa, sa->sa_len);
- sin6.sin6_len = sizeof(struct sockaddr_in6);
sin6.sin6_family = AF_INET6;
if (sa->sa_len == sizeof(struct sockaddr_in6) &&
(IN6_IS_ADDR_LINKLOCAL(_addr) ||
@@ -520,7 +519,7 @@ routename6(struct sockaddr_in6 *sin6)
else
niflags |= NI_NOFQDN;
- if (getnameinfo((struct sockaddr *)sin6, sin6->sin6_len,
+ if (getnameinfo((struct sockaddr *)sin6, sizeof(*sin6),
line, sizeof(line), NULL, 0, niflags) != 0)
strncpy(line, "invalid", sizeof(line));
@@ -640,7 +639,7 @@ netname6(struct sockaddr_in6 *sa6, struc
if (nflag)
flag |= NI_NUMERICHOST;
- error = getnameinfo((struct sockaddr *), sin6.sin6_len,
+ error = getnameinfo((struct sockaddr *), sizeof(sin6),
hbuf, sizeof(hbuf), NULL, 0, flag);
if (error)
snprintf(hbuf, sizeof(hbuf), "invalid");
Index: usr.bin/rusers/rusers.c
===
RCS file: /data/src/openbsd/src/usr.bin/rusers/rusers.c,v
retrieving revision 1.39
diff -u -p -r1.39 rusers.c
--- usr.bin/rusers/rusers.c 5 Aug 2016 10:34:18 - 1.39
+++ usr.bin/rusers/rusers.c 6 Aug 2016 18:41:08 -
@@ -418,7 +418,7 @@ retry:
msgp->acpted_rply.ar_results.where = (caddr_t)resp;
msgp->acpted_rply.ar_results.proc = xdr_rmtcallres;
- fromlen = sizeof(struct sockaddr);
+ fromlen = sizeof(raddr);
inlen = recvfrom(sock, inbuf, sizeof(inbuf), 0,
(struct sockaddr *), );
if (inlen < 0) {
@@ -534,7 +534,6 @@ allhosts(void)
outlen[1] = xdr_getpos();
xdr_destroy();
- baddr.sin_len = sizeof(struct sockaddr_in);
baddr.sin_family = AF_INET;
baddr.sin_port = htons(PMAPPORT);
baddr.sin_addr.s_addr = htonl(INADDR_ANY);
@@ -572,12 +571,12 @@ allhosts(void)
if (i & 1) {
if (sendto(sock[0], buf[0], outlen[0], 0,
(struct sockaddr *),
- sizeof(struct sockaddr)) != outlen[0])
+ sizeof(baddr)) != outlen[0])
err(1, "can't send broadcast packet");
} else {
if (sendto(sock[1], buf[1], outlen[1], 0,
(struct sockaddr *),
- sizeof(struct sockaddr)) != outlen[1])
+ sizeof(baddr)) != outlen[1])
err(1, "can't send broadcast packet");
}
}
Index: usr.bin/showmount/showmount.c
===
RCS file: /data/src/openbsd/src/usr.bin/showmount/showmount.c,v
retrieving revision 1.20
diff -u -p -r1.20 showmount.c
--- usr.bin/showmount/showmount.c 16 Mar 2016
