Bugs in ASCII2BINARY handling

From: Ruslan Ermilov (ru_at_FreeBSD.org)
Date: 04/08/04

  • Next message: Don Bowman: "RE: Stupid question about managed switches" 
    Date: Thu, 8 Apr 2004 20:38:55 +0300
To: Archie Cobbs <archie@FreeBSD.org>, Julian Elischer <julian@FreeBSD.org>

    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@FreeBSD.org
FreeBSD committer

  • Next message: Don Bowman: "RE: Stupid question about managed switches"