Bugs in ASCII2BINARY handling

[Ruslan Ermilov]

Hi,

I've been experimenting with IP multicasting under Netgraph, and
needed to use setsockopt(2)/getsockopt(2) on an ng_ksocket(4) node
using the ASCII form of the "setopt" and "getopt" messages.

Problem #1: ASCII message "getopt" doesn't work.

: + mkpeer . ksocket s inet/dgram/udp
: + msg .:s getopt { level=0 name=10 }
: ngctl: send msg: Invalid argument

Analysis:

The NGM_KSOCKET_GETOPT message type handler expects the argument
of exactly sizeof(struct ng_ksocket_sockopt) == 8 bytes long.

When doing the ASCII2BINARY conversion (in ng_base.c), the "ascii"
message's header gets copied into the "binary" message header,
including the header.arglen, which is 20 in the above example,
strlen("{ level=0 name=10 }") + 1.

Since "struct ng_ksocket_sockopt" doesn't include the field to
denote the length of the "value" byte array, the function
ng_parse_sockoptval_getLength() computes the length of the "value"
byte array (which is not present in the above "getopt" message
at all) using the passed message's argument len, which in the
above example equals 20 - sizeof(struct ng_ksocket_sockopt) == 12,
instead of the expected 0, hence the handler returns EINVAL.

Workaround to make "getopt" message work:

%%%
Index: ng_ksocket.c
===================================================================
RCS file: /home/ncvs/src/sys/netgraph/ng_ksocket.c,v
retrieving revision 1.39
diff -u -p -r1.39 ng_ksocket.c
--- ng_ksocket.c	26 Jan 2004 14:05:31 -0000	1.39
+++ ng_ksocket.c	8 Apr 2004 14:22:34 -0000
@@ -809,7 +809,7 @@ ng_ksocket_rcvmsg(node_p node, item_p it
 			struct sockopt sopt;
 
 			/* Sanity check */
-			if (msg->header.arglen != sizeof(*ksopt))
+			if (msg->header.arglen < sizeof(*ksopt))
 				ERROUT(EINVAL);
 			if (so == NULL)
 				ERROUT(ENXIO);
%%%

Problem #2: ASCII message "setopt" may not always work.

Analysis:

Basically we have the same problem, because the "binary" message
gets arglen from its "ascii" representation.  The result is that
we either send the excessive bytes:

: + mkpeer . ksocket s inet/dgram/udp
: + debug 2
: + msg .:s setopt { level=0 name=10 value=[ 3 ] }
: [...]
: ngctl: SENDING MESSAGE:
: ngctl: SOCKADDR: { fam=32 len=6 addr=".:s" }
: ngctl: NG_MESG :
: ngctl:   vers   0
: ngctl:   arglen 32
: ngctl:   flags  0
: ngctl:   token  19
: ngctl:   cookie KSOCKET (942710669)
: ngctl:   cmd    7
: ngctl:   args (32 bytes)
: ngctl: 0000:  00 00 00 00 0a 00 00 00 03 00 00 00 00 00 00 00   ................
: ngctl: 0010:  00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00   ................

(everything after byte 03 shouldn't have been sent), or it doesn't
work at all:

: + msg .:s setopt { level=0 name=10 value=[ 40=3 ] }
: ngctl: send msg: Argument list too long

In the first case, we send excessive bytes because the function
ng_parse_sockoptval_getLength() computes incorrect length of the
"value" byte array, which is arglen - 8, where arglen is actually
a rounded up strlen() of the ASCII representation of the message
(I'm clueless as to why it gets rounded to the 4 byte boundary).

In the second case, ASCII2BINARY bogusly assumes that the binary
form will be always shorter than its ASCII representation, so it
sets binary's arglen to that of ascii's arglen, albeit it uses
a static buffer of 2000 bytes.  This should be easily fixed:

%%%
Index: ng_base.c
===================================================================
RCS file: /home/ncvs/src/sys/netgraph/ng_base.c,v
retrieving revision 1.74
diff -u -p -r1.74 ng_base.c
--- ng_base.c	26 Jan 2004 14:05:30 -0000	1.74
+++ ng_base.c	8 Apr 2004 14:00:58 -0000
@@ -2805,6 +2805,7 @@ ng_generic_msg(node_p here, item_p item,
 
 		/* Copy ASCII message header to response message payload */
 		bcopy(ascii, binary, sizeof(*ascii));
+		binary->header.arglen = bufSize;
 
 		/* Find command by matching ASCII command string */
 		for (c = here->nd_type->cmdlist;
%%%

Conclusion: ASCII2BINARY doesn't behave well with variable size
arrays that do not have a field describing their size.  If
"getopt" and "setopt" had such a field, only second patch would
be necessary, which is generic enough anyway.

Archie, Julian, could you please comment on the patches proposed?


Cheers,
-- 
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-net/attachments/20040408/9d4cac9a/attachment.bin