Re: [vpp-dev] A Curious DHCP Hostname Terminator Choice

[Burt Silverman]

Way back when I must have been dealing with u8* versus char* rather than
u8[] versus char[], but that slight bit of confusion on my part does not
change my suggestion:-)

Burt

On Thu, Apr 27, 2017 at 4:09 PM, Burt Silverman  wrote:

> If I were coding, in addition to Jon's change, I probably would change
> "u8" to "char" for the type of hostname elements in the dhcp_client_config
> and dhcp_compl_event in dhcp.api, because way back when I liked to think of
> u8 as a hint that the array is a vector style string, i.e., not terminated
> with a null. I don't know how carefully we maintained the convention years
> ago, and I haven't looked at much code recently. I didn't write "unsigned
> char" above because strings are usually "char".
>
> Burt
>
> On Thu, Apr 27, 2017 at 2:19 PM, Luke, Chris 
> wrote:
>
>> > Jon Loeliger said:
>> >> On Thu, Apr 27, 2017 at 12:19 PM, Luke, Chris > chris_l...@comcast.com> wrote:
>> >> [...] What I am not sure of is NUL in DHCP hostname strings – I
>> remember reading somewhere
>> >> it’s optional, so I suspect that means the TLV length is used to
>> determine the string length;
>> >> meaning it might be possible to have a hostname that is 64 printable
>> characters long. Maybe.
>> >
>> > According to my RFC digging, 1 to 63 characters.
>> >
>> > NUL is not a valid hostname character.
>>
>> What it actually says is:
>>
>>sname64  Optional server host name, null terminated string.
>>
>> So yes, you're semantically correct. I forgot in the original DHCP this
>> was a static field and not a TLV. In DHCPv6 however it doesn't exist.
>>
>> Chris.
>> ___
>> vpp-dev mailing list
>> vpp-dev@lists.fd.io
>> https://lists.fd.io/mailman/listinfo/vpp-dev
>
>
>
___
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev

Re: [vpp-dev] A Curious DHCP Hostname Terminator Choice

[Burt Silverman]

If I were coding, in addition to Jon's change, I probably would change "u8"
to "char" for the type of hostname elements in the dhcp_client_config and
dhcp_compl_event in dhcp.api, because way back when I liked to think of u8
as a hint that the array is a vector style string, i.e., not terminated
with a null. I don't know how carefully we maintained the convention years
ago, and I haven't looked at much code recently. I didn't write "unsigned
char" above because strings are usually "char".

Burt

On Thu, Apr 27, 2017 at 2:19 PM, Luke, Chris  wrote:

> > Jon Loeliger said:
> >> On Thu, Apr 27, 2017 at 12:19 PM, Luke, Chris  chris_l...@comcast.com> wrote:
> >> [...] What I am not sure of is NUL in DHCP hostname strings – I
> remember reading somewhere
> >> it’s optional, so I suspect that means the TLV length is used to
> determine the string length;
> >> meaning it might be possible to have a hostname that is 64 printable
> characters long. Maybe.
> >
> > According to my RFC digging, 1 to 63 characters.
> >
> > NUL is not a valid hostname character.
>
> What it actually says is:
>
>sname64  Optional server host name, null terminated string.
>
> So yes, you're semantically correct. I forgot in the original DHCP this
> was a static field and not a TLV. In DHCPv6 however it doesn't exist.
>
> Chris.
> ___
> vpp-dev mailing list
> vpp-dev@lists.fd.io
> https://lists.fd.io/mailman/listinfo/vpp-dev
___
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev

Re: [vpp-dev] A Curious DHCP Hostname Terminator Choice

[Luke, Chris]

> Jon Loeliger said:
>> On Thu, Apr 27, 2017 at 12:19 PM, Luke, Chris 
>>  wrote:
>> [...] What I am not sure of is NUL in DHCP hostname strings – I remember 
>> reading somewhere 
>> it’s optional, so I suspect that means the TLV length is used to determine 
>> the string length; 
>> meaning it might be possible to have a hostname that is 64 printable 
>> characters long. Maybe.
>
> According to my RFC digging, 1 to 63 characters.
>
> NUL is not a valid hostname character.

What it actually says is:

   sname64  Optional server host name, null terminated string.

So yes, you're semantically correct. I forgot in the original DHCP this was a 
static field and not a TLV. In DHCPv6 however it doesn't exist.

Chris.
___
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev

Re: [vpp-dev] A Curious DHCP Hostname Terminator Choice

[Jon Loeliger]

On Thu, Apr 27, 2017 at 12:19 PM, Luke, Chris 
wrote:

> It looks, to me, like someone fixed a problem in the wrong place.
>
>
>
> I would certainly entertain the patch, especially if it included logic to
> make sure mp->hostname doesn’t overrun. Not likely since the DHCP option is
> limited to the same size but it’s still good practice. What I am not sure
> of is NUL in DHCP hostname strings – I remember reading somewhere it’s
> optional, so I suspect that means the TLV length is used to determine the
> string length; meaning it might be possible to have a hostname that is 64
> printable characters long. Maybe.
>

According to my RFC digging, 1 to 63 characters.

NUL is not a valid hostname character.

HTH,
jdl
___
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev

Re: [vpp-dev] A Curious DHCP Hostname Terminator Choice

[Luke, Chris]

It looks, to me, like someone fixed a problem in the wrong place.

I would certainly entertain the patch, especially if it included logic to make 
sure mp->hostname doesn’t overrun. Not likely since the DHCP option is limited 
to the same size but it’s still good practice. What I am not sure of is NUL in 
DHCP hostname strings – I remember reading somewhere it’s optional, so I 
suspect that means the TLV length is used to determine the string length; 
meaning it might be possible to have a hostname that is 64 printable characters 
long. Maybe.

Chris.

From: Jon Loeliger [mailto:j...@netgate.com]
Sent: Thursday, April 27, 2017 11:28
To: Luke, Chris 
Cc: vpp-dev 
Subject: Re: [vpp-dev] A Curious DHCP Hostname Terminator Choice

On Wed, Apr 26, 2017 at 3:37 PM, Luke, Chris 
mailto:chris_l...@comcast.com>> wrote:
Definitely looks spurious to me.

Chris.

Spurious or wrong?

Will folks entertain a patch converting that newline to a 0?

jdl

From: vpp-dev-boun...@lists.fd.io<mailto:vpp-dev-boun...@lists.fd.io> 
[mailto:vpp-dev-boun...@lists.fd.io<mailto:vpp-dev-boun...@lists.fd.io>] On 
Behalf Of Jon Loeliger
Sent: Wednesday, April 26, 2017 4:24 PM
To: vpp-dev mailto:vpp-dev@lists.fd.io>>
Subject: [vpp-dev] A Curious DHCP Hostname Terminator Choice

Hi Guys,

Way over in src/vnet/dhcp/dhcp_api.c, we find the function


  clib_memcpy (&mp->hostname, hostname, vec_len (hostname));
  mp->hostname[vec_len (hostname) + 1] = '\n';
  clib_memcpy (&mp->host_address[0], host_address, 16);
  clib_memcpy (&mp->router_address[0], router_address, 16);

___
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev

Re: [vpp-dev] A Curious DHCP Hostname Terminator Choice

[Jon Loeliger]

On Wed, Apr 26, 2017 at 3:37 PM, Luke, Chris  wrote:

> Definitely looks spurious to me.
>
>
>
> Chris.
>

Spurious or wrong?

Will folks entertain a patch converting that newline to a 0?

jdl


> *From:* vpp-dev-boun...@lists.fd.io [mailto:vpp-dev-boun...@lists.fd.io] *On
> Behalf Of *Jon Loeliger
> *Sent:* Wednesday, April 26, 2017 4:24 PM
> *To:* vpp-dev 
> *Subject:* [vpp-dev] A Curious DHCP Hostname Terminator Choice
>
>
>
> Hi Guys,
>
>
>
> Way over in src/vnet/dhcp/dhcp_api.c, we find the function
>
>
>
>
>   clib_memcpy (&mp->hostname, hostname, vec_len (hostname));
>
>   mp->hostname[vec_len (hostname) + 1] = '\n';
>
>   clib_memcpy (&mp->host_address[0], host_address, 16);
>
>   clib_memcpy (&mp->router_address[0], router_address, 16);
>
>
>
___
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev

Re: [vpp-dev] A Curious DHCP Hostname Terminator Choice

[Luke, Chris]

Definitely looks spurious to me.

Chris.

From: vpp-dev-boun...@lists.fd.io [mailto:vpp-dev-boun...@lists.fd.io] On 
Behalf Of Jon Loeliger
Sent: Wednesday, April 26, 2017 4:24 PM
To: vpp-dev 
Subject: [vpp-dev] A Curious DHCP Hostname Terminator Choice

Hi Guys,

Way over in src/vnet/dhcp/dhcp_api.c, we find the function

void
dhcp_compl_event_callback (u32 client_index, u32 pid, u8 * hostname,
   u8 is_ipv6, u8 * host_address, u8 * router_address,
   u8 * host_mac)

which contains a curious use of the newline character:

  mp = vl_msg_api_alloc (sizeof (*mp));
  mp->client_index = client_index;
  mp->pid = pid;
  mp->is_ipv6 = is_ipv6;
  clib_memcpy (&mp->hostname, hostname, vec_len (hostname));
  mp->hostname[vec_len (hostname) + 1] = '\n';
  clib_memcpy (&mp->host_address[0], host_address, 16);
  clib_memcpy (&mp->router_address[0], router_address, 16);

So, is that '\n' supposed to be 0 instead?
Or is that value used in some odd location that requires an
actual newline here?
What happens if the user supplies a hostname of exactly 63
octets and this newline is added?  There is at least one
use of strncpy() on this hostname value, so it seems fragile
to me at the fencepost.

Pedantically yours,
jdl

___
vpp-dev mailing list
vpp-dev@lists.fd.io
https://lists.fd.io/mailman/listinfo/vpp-dev