`buf_of_pkt` allows creation of unreadable packets

[yomimono]

Particularly, when given an empty list for options (which I would expect to be an error condition, since DHCP messages aren't valid without at least message-type):

utop # t;;
- : pkt =  {srcmac = <abstr>; dstmac = <abstr>; srcip = <abstr>; dstip = <abstr>;
            srcport = 68; dstport = 67; op = BOOTREQUEST; htype = Ethernet_10mb;
            hlen = 6; hops = 0; xid = 2l; secs = 0; flags = Broadcast; ciaddr = <abstr>;
            yiaddr = <abstr>; siaddr = <abstr>; giaddr = <abstr>; chaddr = <abstr>;
            sname = ""; file = ""; options = []}
utop # let b = Dhcp_wire.buf_of_pkt t;;
val b : Cstruct.t = {Cstruct.buffer = <abstr>; off = 0; len = 342}
utop # Dhcp_wire.pkt_of_buf b (Cstruct.len b);;
- : (pkt, string) result = Error "Invalid cookie"   

[haesbaert]

I agree,
"""
The first four octets of the 'options' field of the DHCP message
contain the (decimal) values 99, 130, 83 and 99, respectively (this
is the same magic cookie as is defined in RFC 1497 [17]). The
remainder of the 'options' field consists of a list of tagged
parameters that are called "options". All of the "vendor extensions"
listed in RFC 1497 are also DHCP options. RFC 1533 gives the
complete set of options defined for use with DHCP.

Several options have been defined so far. One particular option -
the "DHCP message type" option - must be included in every DHCP
message. This option defines the "type" of the DHCP message.
Additional options may be allowed, required, or not allowed,
depending on the DHCP message type.
""""

I believe we should just error out if options = []

[yomimono]

There's also a problem when options contains a Parameter_requests [] element:

let t = {t with options = [Message_type DHCPDISCOVER; Parameter_requests []]};;
val t : pkt =                                                                                                                     {srcmac = <abstr>; dstmac = <abstr>; srcip = <abstr>; dstip = <abstr>;                                                           srcport = 68; dstport = 67; op = BOOTREQUEST; htype = Ethernet_10mb;                                                         
   hlen = 6; hops = 0; xid = 1862551154l; secs = 0; flags = Broadcast;
   ciaddr = <abstr>; yiaddr = <abstr>; siaddr = <abstr>; giaddr = <abstr>;
   chaddr = <abstr>; sname = ""; file = "";
   options = [Message_type DHCPDISCOVER; Parameter_requests []]}
utop # let b = Dhcp_wire.buf_of_pkt t;;
val b : Cstruct.t = {Cstruct.buffer = <abstr>; off = 0; len = 342}
utop # Dhcp_wire.pkt_of_buf b (Cstruct.len b);;
- : (pkt, string) result = Error "Malformed len 0 in option 55" 

[yomimono]

I'll make a patch for options = []; also for any empty list presented in the options? I don't have the set of them in my head offhand but I can't quickly think of a situation where that's a valid thing to do.

[haesbaert]

Hmm sadly there are, Mobile IP Home Agent option (or whatever the crap that is):

8.13. Mobile IP Home Agent option

This option specifies a list of IP addresses indicating mobile IP
home agents available to the client. Agents SHOULD be listed in
order of preference.

The code for this option is 68. Its minimum length is 0 (indicating
no home agents are available) and the length MUST be a multiple of 4.
It is expected that the usual length will be four octets, containing
a single home agent's address.

But options = [] is for sure illegal.

[haesbaert]

so if we would check a minimum, we would have to do type by type, and that's like 200 lines of code minimum, it might be worth to be relaxed in this case.

[yomimono]

Hm, unfortunate. :/ I'll take care of options = [] at least, and have a look at all the option types that allow lists to see whether any strategy suggests itself.