On Feb 4, 2016, at 11:21 AM, Chaigneau, Nicolas <nicolas.chaigneau@capgemini.com> wrote
I've noticed that FreeRADIUS DHCP decoder is handling strings from the DHCP header fields a bit oddly.
Basically, it allocates a value pair buffer with the maximum field length (64 or 128 octets, for DHCP-Server-Host-Name and DHCP-Boot-Filename respectively) + 1, and memcpy the data then add a '\0'.
That's not good.
But RFC 2131 explicitly states that these DHCP fields are null terminated strings. So why not handle them as such ?
It should.
Current code from "fr_dhcp_decode" function:
case PW_TYPE_STRING: q = talloc_array(vp, char, dhcp_header_sizes[i] + 1); memcpy(q, p, dhcp_header_sizes[i]); q[dhcp_header_sizes[i]] = '\0'; fr_pair_value_strsteal(vp, q);
if (vp->vp_length == 0) fr_pair_list_free(&vp); break;
That's inefficient for a whole host of reasons.
How about something like this instead ? (copy a string, but do not trust the DHCP field to be actually null terminated)
case PW_TYPE_STRING: if (*p != '\0') { uint8_t *end; int len; end = memchr(p, '\0', dhcp_header_sizes[i]); len = end ? end - p : dhcp_header_sizes[i]; q = talloc_array(vp, char, len + 1); memcpy(q, p, len); q[len] = '\0'; fr_pair_value_strsteal(vp, q); } if (vp->vp_length == 0) fr_pair_list_free(&vp);
That should work. Though the check for a vp_length == 0 should really be done before allocating any memory.
This would avoid having things like:
DHCP-Server-Host-Name = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000" DHCP-Boot-Filename = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
That's good.
(I can do a pull request for 3.1.x if you wish)
That would be good. It should also be applied to 3.0. Alan DeKok.