Re: route(4) manual and sockaddrs; ROUNDUP()
On 7/15/19 12:45 PM, Claudio Jeker wrote: On Mon, Jul 15, 2019 at 11:08:53AM +0300, Vadim Penzin wrote: On 7/15/19 10:28 AM, Claudio Jeker wrote: On Sun, Jul 14, 2019 at 01:15:40PM +0300, Vadim Penzin wrote: Since `sa_len' describes the size of a `sockaddr' (or one of its derivatives) /including/ `sa_len' (maybe I am wrong, but this is my interpretation of the comment `total length' that appears near the definition of `struct sockaddr' in ), `sa_len' just cannot be zero. Yes, it can not be zero. The current definition of ROUNDUP() might hide a bug. In addition, on some machines, it disturbs the pipeline of the CPU by introducing a branch (for no real reason, as it seems, while I might be nitpicking). At very least, it looks confusing. The following patch amends the issue: Index: route.c === RCS file: /cvs/src/sys/net/route.c,v retrieving revision 1.387 diff -u -p -r1.387 route.c --- route.c 24 Jun 2019 22:26:25 - 1.387 +++ route.c 14 Jul 2019 10:05:04 - @@ -138,7 +138,7 @@ #include #endif -#define ROUNDUP(a) (a>0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long)) +#define ROUNDUP(a) (1 + (((a) - 1) | (sizeof(long) - 1))) /* Give some jitter to hash, to avoid synchronization between routers. */ static uint32_trt_hashjitter; This only touches only one version of ROUNDUP() and looking at how ROUNDUP() is used in route.c I wonder if it is actually needed at all. Looking at the code in route.c and rtsock.c I think it is better to leave this like it is or fix both files making sure that the necessary checks are put in place to make sure that a sa_len of 0 can't get into the system. If you wish to remove ROUNDUP() then fine, however, leaving it as is does not help. Imagine the behaviour of the original ROUNDUP() in the unlikely event when the kernel does somehow produce a sockaddr with sa_len of zero. The kernel will send an unvalid sockaddr to the user. ROUNDUP() alone will not help detection or correction of that fault. Now imagine the behaviour of the new version. The kernel will not send an incorrect sockaddr to the user (ROUNDUP() returns zero) but a sockaddr will be missing entirely. What is better: having something unusable and misleading or not having it (with a good way of knowing that it is missing)? That's philosophy to me. In any case, ROUNDUP() cannot magically help when sa_len is zero. Some time ago there was an infinite loop because of a 0 length advance and so the parser code never moved over the data. At least this does not happen with a ROUNDUP() that ensures that at least sizeof(long) data is consumed on every call. Keep in mind userland can send in any kind of crap and syzkaller told me that the rtsock code is way to trusting. This is why I think this is incomplete. The ROUNDUP() version in rtsock.c and route.c should be kept in sync (and maybe moved to a .h file). Unless I am missing something, ROUTE() is not used directly for parsing user data. ADVANCE() does `call' ROUTE() for parsing, and it would look more natural if ADVANCE() were checking that it, well, advances :-) I think that explicit consistency checks and aborting calls early is a better strategy than relying on ROUNDUP()'s sweeping the garbage under the rug. At any rate, providing a user-visible macro that reports the size of memory that is allocated for a sockaddr is a much-needed companion to route(4). Digging knowledge up from the kernel sources should not be a strict condition for application development.
Re: route(4) manual and sockaddrs; ROUNDUP()
On 7/15/19 10:28 AM, Claudio Jeker wrote: On Sun, Jul 14, 2019 at 01:15:40PM +0300, Vadim Penzin wrote: Since `sa_len' describes the size of a `sockaddr' (or one of its derivatives) /including/ `sa_len' (maybe I am wrong, but this is my interpretation of the comment `total length' that appears near the definition of `struct sockaddr' in ), `sa_len' just cannot be zero. Yes, it can not be zero. The current definition of ROUNDUP() might hide a bug. In addition, on some machines, it disturbs the pipeline of the CPU by introducing a branch (for no real reason, as it seems, while I might be nitpicking). At very least, it looks confusing. The following patch amends the issue: Index: route.c === RCS file: /cvs/src/sys/net/route.c,v retrieving revision 1.387 diff -u -p -r1.387 route.c --- route.c 24 Jun 2019 22:26:25 - 1.387 +++ route.c 14 Jul 2019 10:05:04 - @@ -138,7 +138,7 @@ #include #endif -#define ROUNDUP(a) (a>0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long)) +#define ROUNDUP(a) (1 + (((a) - 1) | (sizeof(long) - 1))) /* Give some jitter to hash, to avoid synchronization between routers. */ static uint32_trt_hashjitter; This only touches only one version of ROUNDUP() and looking at how ROUNDUP() is used in route.c I wonder if it is actually needed at all. Looking at the code in route.c and rtsock.c I think it is better to leave this like it is or fix both files making sure that the necessary checks are put in place to make sure that a sa_len of 0 can't get into the system. If you wish to remove ROUNDUP() then fine, however, leaving it as is does not help. Imagine the behaviour of the original ROUNDUP() in the unlikely event when the kernel does somehow produce a sockaddr with sa_len of zero. The kernel will send an unvalid sockaddr to the user. ROUNDUP() alone will not help detection or correction of that fault. Now imagine the behaviour of the new version. The kernel will not send an incorrect sockaddr to the user (ROUNDUP() returns zero) but a sockaddr will be missing entirely. What is better: having something unusable and misleading or not having it (with a good way of knowing that it is missing)? That's philosophy to me. In any case, ROUNDUP() cannot magically help when sa_len is zero.
Re: ifstated(8): Fix Hard-Coded Message Size
On 7/15/19 10:15 AM, Claudio Jeker wrote:
On Sun, Jul 14, 2019 at 05:28:17PM +0300, Vadim Penzin wrote:
The following patch replaces the hard-coded message size of 2048 bytes in
ifstated.c:rt_msg_handler() with RTM_MAXSIZE (which is currently defined as
2048 bytes) that, I believe, was the intent of the author.
Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.64
diff -u -p -r1.64 ifstated.c
--- ifstated.c 28 Jun 2019 13:32:47 - 1.64
+++ ifstated.c 14 Jul 2019 14:22:32 -
@@ -236,7 +236,7 @@ load_config(void)
void
rt_msg_handler(int fd, short event, void *arg)
{
- char msg[2048];
+ char msg[RTM_MAXSIZE];
struct rt_msghdr *rtm = (struct rt_msghdr *)
struct if_msghdr ifm;
struct if_announcemsghdr ifan;
I don't think RTM_MAXSIZE should be used outside of the kernel.
Especially for reading messages. I would leave the code as is.
In that case, please hide RTM_MAXSIZE using _KERNEL. The definition of
RTM_MAXSIZE is right above RTM_ADD which is user-visible according to
the current documentation.
What about other defines? Can user-space programs use RTM_VERSION, for
instance?
portmap(8): Remove an odd call to endpwent(3)
portmap(8) does not use setpassent(3) which is the only way of causing a
successful call to getpwnam(3) not to close the password database,
therefore the call to endpwent(3) seems to be unnecessary.
The following patch removes the call to endpwent(3):
Index: portmap.c
===
RCS file: /cvs/src/usr.sbin/portmap/portmap.c,v
retrieving revision 1.50
diff -u -p -r1.50 portmap.c
--- portmap.c 28 Jun 2019 13:32:49 - 1.50
+++ portmap.c 14 Jul 2019 15:29:27 -
@@ -245,7 +245,6 @@ main(int argc, char *argv[])
exit(1);
}
}
- endpwent();
if (pledge("stdio inet proc", NULL) == -1)
err(1, "pledge");
ifstated(8): Fix Hard-Coded Message Size
The following patch replaces the hard-coded message size of 2048 bytes
in ifstated.c:rt_msg_handler() with RTM_MAXSIZE (which is currently
defined as 2048 bytes) that, I believe, was the intent of the author.
Index: ifstated.c
===
RCS file: /cvs/src/usr.sbin/ifstated/ifstated.c,v
retrieving revision 1.64
diff -u -p -r1.64 ifstated.c
--- ifstated.c 28 Jun 2019 13:32:47 - 1.64
+++ ifstated.c 14 Jul 2019 14:22:32 -
@@ -236,7 +236,7 @@ load_config(void)
void
rt_msg_handler(int fd, short event, void *arg)
{
- char msg[2048];
+ char msg[RTM_MAXSIZE];
struct rt_msghdr *rtm = (struct rt_msghdr *)
struct if_msghdr ifm;
struct if_announcemsghdr ifan;
route(4): Manpage Clarifications + An Example
Please find below a patch that contains the following:
(a) An explanation of alignment of address structures within a routing
message;
(b) An example (I admit, it is somewhat lengthy --- 146 lines) of a
basic parser of route(4) messages.
(This message is a follow-up to
https://marc.info/?l=openbsd-tech=155627978527725)
Index: route.4
===
RCS file: /cvs/src/share/man/man4/route.4,v
retrieving revision 1.52
diff -u -p -r1.52 route.4
--- route.4 30 Apr 2019 16:34:19 - 1.52
+++ route.4 14 Jul 2019 13:39:33 -
@@ -137,13 +137,13 @@ routing information for all address fami
to a specific address family by specifying which one is desired.
There can be more than one routing socket open per system.
.Pp
-Messages are formed by a header followed by a small
+A routing message consists of a header that may be followed by a
number of
.Vt sockaddr
-structures (which are variable length),
-interpreted by position, and delimited
-by the length entry in the
-.Vt sockaddr .
+structures of a variable size (the size of a particular structure
+is determined by its
+.Vt sa_len
+entry; every structure is aligned on the boundary of sizeof(long)).
An example of a message with four addresses might be an
IPv4 route addition: the destination, netmask, gateway, and label,
since both netmasks and labels are sent over the routing socket as
@@ -477,6 +477,119 @@ Specifiers for which addresses are prese
#define RTA_SRC0x100 /* source sockaddr present */
#define RTA_SRCMASK0x200 /* source netmask present */
#define RTA_LABEL 0x400 /* route label present */
+.Ed
+.Sh EXAMPLES
+Parsing a routing message.
+.Bd -literal
+#include
+#include
+#include
+#include
+#include
+
+/* An abstraction of a routing message. */
+union rt_msg {
+ struct rt_msghdr rt;
+ struct if_msghdr ifm;
+ struct ifa_msghdr ifa;
+ struct if_announcemsghdr ifan;
+ uint8_t buf [ RTM_MAXSIZE ];
+};
+
+#define rt_msglen rt.rtm_msglen
+#define rt_version rt.rtm_version
+#define rt_type rt.rtm_type
+#define rt_hdrlen rt.rtm_hdrlen
+#define rt_buf buf
+
+/* Round up the size of a struct sockaddr as required by route(4). */
+#define ROUNDUP(x) (1 + (((x) - 1) | (sizeof(long) - 1)))
+
+/* Parse address structures that may follow the header. */
+
+static union rt_msg *
+rt_msg_parse_addrs (union rt_msg * const msg, const int addrs)
+{
+ const void *p = (void *) msg + msg->rt_hdrlen;
+ const void * const q = (void *) msg + msg->rt_msglen;
+ int mask = addrs;
+
+ while (p < q && mask ) {
+ const struct sockaddr * const sa = p;
+ const int rta = mask ^ (mask & mask - 1);
+
+ /*
+* Here the variable `sa' points to the current sockadr
+* and the variable `rta' holds the corresponding
+* RTA_ value.
+*
+* NOTE: Before using these variables, you MUST validate
+* that (a) the value of `rta' is defined and expected;
+* (b) `sa_len' of `sa' must correspond to its `sa_family'.
+*/
+
+ p += ROUNDUP (sa->sa_len);
+ mask &= ~rta;
+ }
+
+ if (mask)
+ warn ("A route(4) message is missing (some) addresses.");
+
+ if (p < q)
+ warn ("A route(4) message contains odd data.");
+
+ if (p > q) {
+ err (4, "A route(4) message is malformed.");
+ return NULL;
+ }
+
+ return msg;
+}
+
+/* Parse a message. */
+
+static union rt_msg *
+rt_msg_parse ( union rt_msg * const msg )
+{
+ int addrs = 0;
+
+ if (msg -> rt_version != RTM_VERSION ) {
+ err (4, "Unexpected version of a route(4) message: %d",
+ msg->rt_version);
+ return NULL;
+ }
+
+ switch (msg -> rt_type) {
+ case RTM_ADD:
+ case RTM_DELETE:
+ case RTM_CHANGE:
+ case RTM_GET:
+ case RTM_LOSING:
+ case RTM_REDIRECT:
+ case RTM_MISS:
+ case RTM_RESOLVE:
+ case RTM_DESYNC:
+ addrs = msg->rt.rtm_addrs;
+ break;
+ case RTM_NEWADDR:
+ case RTM_DELADDR:
+ addrs = msg->ifa.ifam_addrs;
+ break;
+ case RTM_IFINFO:
+ addrs = msg->ifm.ifm_addrs;
+ break;
+ case RTM_IFANNOUNCE:
+ return msg;
+ case RTM_INVALIDATE:
+ err (4, "RTM_INVALIDATE received from route(4).");
+ return NULL;
+ default:
+ warn ("Unknown route(4) message: %d", msg->rt_type );
+ return NULL;
+ }
+
+ return rt_msg_parse_addrs (msg, addrs);
+}
.Ed
.Sh SEE ALSO
.Xr netstat 1 ,
Re: route(4) manual and sockaddrs; ROUNDUP()
Since `sa_len' describes the size of a `sockaddr' (or one of its derivatives) /including/ `sa_len' (maybe I am wrong, but this is my interpretation of the comment `total length' that appears near the definition of `struct sockaddr' in ), `sa_len' just cannot be zero. Yes, it can not be zero. The current definition of ROUNDUP() might hide a bug. In addition, on some machines, it disturbs the pipeline of the CPU by introducing a branch (for no real reason, as it seems, while I might be nitpicking). At very least, it looks confusing. The following patch amends the issue: Index: route.c === RCS file: /cvs/src/sys/net/route.c,v retrieving revision 1.387 diff -u -p -r1.387 route.c --- route.c 24 Jun 2019 22:26:25 - 1.387 +++ route.c 14 Jul 2019 10:05:04 - @@ -138,7 +138,7 @@ #include #endif -#define ROUNDUP(a) (a>0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long)) +#define ROUNDUP(a) (1 + (((a) - 1) | (sizeof(long) - 1))) /* Give some jitter to hash, to avoid synchronization between routers. */ static uint32_trt_hashjitter;
Qt5's libtool link scripts are unusable
I admit that I am not familiar with the release process of pre-built
binary packages; I might be writing to a wrong mailing list and I
apologize in advance.
All libtool scripts from qt5 (/usr/local/lib/qt5/*.la) contain the
following on their third line:
LIBQt5XXX_VERSION=5.9# The name that we can dlopen(3).
(XXX above stands for the name of a particular library, such as Core,
Network, etc.)
Since shell scripts (that libtool generates) source .la files, sh(1)
fails on unexpected '(' because someone, somewhere omitted a space
before the comment.
Patching /usr/local/lib/qt5/*.la files programmatically by tucking a
space before the hash character brings linking with Qt5 assisted
(encumbered?) by libtool back to life.
--Vadim
Re: route(4) manual and sockaddrs; ROUNDUP()
Well, the manual shall tell the truth, whatever it is: Messages are formed by a header followed by a small number of sockaddr structures of variable length. The size of every sockaddr structure can be computed by rounding the value of the `sa_len' field of the current structure up to sizeof(long) bytes. I provided a reference to the code in question in my original message: It took me some time to figure out that, apparently, I am looking at the combined effect of in_socktrim() (see src/sys/netinet/in.c) and ROUNDUP() (see src/sys/net/rtsock.c) applied to the netmask of 0xff00 that was stored as sockaddr_in'. The following is a more precise description of its location: See https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/rtsock.c?annotate=1.285 The macro ROUNDUP is defined on lines 1347-1348 It is also used in the definition of the macro ADVANCE on line 1349 The first direct use of ROUNDUP that I quoted is on line 1431. The second is on line 1476. --Vadim On 4/26/19 6:14 PM, Claudio Jeker wrote: On Fri, Apr 26, 2019 at 01:55:52PM +0300, Vadim Penzin wrote: Greetings, 1. The manual of route(4) explains the structure of its messages thus: `Messages are formed by a header followed by a small number of sockaddr structures (which are variable length), interpreted by position, and delimited by the length entry in the sockaddr.' (That phrase is quite unfortunate in itself: `sa_len' is supposed to /determine/ the length of a structure. To me, the word `delimited' implies the presence of a memory object between two adjacent structures, which certainly was not the intent of the author.) Yes this is not quite right since the lenght is rounded to the next long word to ensure that structures are properly aligned on strict align architectures. Armed with that knowledge, I wrote a parser the other day. I very much enjoyed OpenBSD until my program fatally stumbled on a peculiar structure that does not fit the above description: 05 00 00 00 ff 00 00 00 (The value of `sa_len' of that `sockaddr' is nothing that I am familiar with, the address family is `AF_UNSPEC', and the most disturbing: `sa_len' does not match the physical size of this structure. These eight bytes were immediately followed by a perfectly valid `sockaddr_in'.) This is a probably a netmask which are special in many senses from the old time of the radix routing tree. For AF_UNSPEC it defines how many bytes are used to store the netmask. It is wierd I agree. It took me some time to figure out that, apparently, I am looking at the combined effect of in_socktrim() (see src/sys/netinet/in.c) and ROUNDUP() (see src/sys/net/rtsock.c) applied to the netmask of 0xff00 that was stored as sockaddr_in'. I believe that manuals should be more merciful. In the absence of proper documentation, the part of my program that handles this case looks like a mysterious ritual; it should not be that way. Please send suggestions to better documentation to this list. route(4) is not perfect for sure and could use some work. 2. The definition of ROUNDUP() somewhat surprised me: it silently handles zero. The macro is local to the compilation unit; it is used this way: if (rtinfo == NULL || (sa = rtinfo->rti_info[i]) == NULL) continue; rtinfo->rti_addrs |= (1 << i); dlen = ROUNDUP(sa->sa_len); and this way: if ((sa = rtinfo->rti_info[i]) == NULL) continue; rtinfo->rti_addrs |= (1 << i); dlen = ROUNDUP(sa->sa_len); What code are you talking about? (In the second case, the programmer does not check `rtinfo' for being NULL. I only hope that there exists a good explanation to that.) Since `sa_len' describes the size of a `sockaddr' (or one of its derivatives) /including/ `sa_len' (maybe I am wrong, but this is my interpretation of the comment `total length' that appears near the definition of `struct sockaddr' in ), `sa_len' just cannot be zero. Yes, it can not be zero. The current definition of ROUNDUP() might hide a bug. In addition, on some machines, it disturbs the pipeline of the CPU by introducing a branch (for no real reason, as it seems, while I might be nitpicking). At very least, it looks confusing.
route(4) manual and sockaddrs; ROUNDUP()
Greetings, 1. The manual of route(4) explains the structure of its messages thus: `Messages are formed by a header followed by a small number of sockaddr structures (which are variable length), interpreted by position, and delimited by the length entry in the sockaddr.' (That phrase is quite unfortunate in itself: `sa_len' is supposed to /determine/ the length of a structure. To me, the word `delimited' implies the presence of a memory object between two adjacent structures, which certainly was not the intent of the author.) Armed with that knowledge, I wrote a parser the other day. I very much enjoyed OpenBSD until my program fatally stumbled on a peculiar structure that does not fit the above description: 05 00 00 00 ff 00 00 00 (The value of `sa_len' of that `sockaddr' is nothing that I am familiar with, the address family is `AF_UNSPEC', and the most disturbing: `sa_len' does not match the physical size of this structure. These eight bytes were immediately followed by a perfectly valid `sockaddr_in'.) It took me some time to figure out that, apparently, I am looking at the combined effect of in_socktrim() (see src/sys/netinet/in.c) and ROUNDUP() (see src/sys/net/rtsock.c) applied to the netmask of 0xff00 that was stored as sockaddr_in'. I believe that manuals should be more merciful. In the absence of proper documentation, the part of my program that handles this case looks like a mysterious ritual; it should not be that way. 2. The definition of ROUNDUP() somewhat surprised me: it silently handles zero. The macro is local to the compilation unit; it is used this way: if (rtinfo == NULL || (sa = rtinfo->rti_info[i]) == NULL) continue; rtinfo->rti_addrs |= (1 << i); dlen = ROUNDUP(sa->sa_len); and this way: if ((sa = rtinfo->rti_info[i]) == NULL) continue; rtinfo->rti_addrs |= (1 << i); dlen = ROUNDUP(sa->sa_len); (In the second case, the programmer does not check `rtinfo' for being NULL. I only hope that there exists a good explanation to that.) Since `sa_len' describes the size of a `sockaddr' (or one of its derivatives) /including/ `sa_len' (maybe I am wrong, but this is my interpretation of the comment `total length' that appears near the definition of `struct sockaddr' in ), `sa_len' just cannot be zero. The current definition of ROUNDUP() might hide a bug. In addition, on some machines, it disturbs the pipeline of the CPU by introducing a branch (for no real reason, as it seems, while I might be nitpicking). At very least, it looks confusing. Thank you for your time, -- Vadim
