Ship It!

Generally looks good to me.  I'd prefer to change those %d's to %u's, and I would have preferred the use of strnlen() if available (I think we do have it), but I leave the decision here up to you -- feel free to list me as a reviewer either way.

usr/src/cmd/cmd-inet/usr.sbin/snoop/snoop_tftp.c (Diff revision 1)

Show all issues

The %d in this should %u.  These are unsigned values.  Having said that, the uint16_t is losslessly convertible to int, so it's ok, but %u would be more pedantically correct.

usr/src/cmd/cmd-inet/usr.sbin/snoop/snoop_tftp.c (Diff revision 1)

Show all issues

Instead of all this logic to constrain the length by stuffing a \0 at the end of tftp_options, why not just use strnlen() to ensure that we don't walk past the end?
Also, the way this works it means that the options is constrained to 127 bytes (128 with \0)... is that intended?

1. 

   Toomas Soome Oct. 26, 2018, 10:35 a.m.

   We need the \0 anyhow, but using strnlen() is good idea anyhow. Also cleaned up the len vs size variables. 
   By RFC the request package can indeed be up to 512B, in reality, the list of options is rather short, even with all provided, we should be fine with 127B buffer - if not, we can increase it:D 
   Note, Oracle did drop snoop in favor to tshark (+wireshark), given how buggy and limited the snoop is, we probably should drop it too, but that is another topic.

usr/src/cmd/cmd-inet/usr.sbin/snoop/snoop_tftp.c (Diff revision 1)

Show all issues

A comment here indicating that the spec requires that the options be treated case insensitively would be helpful.  (I just checked the RFC.)

Ship It!

Ship It!