Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP messages in the driver

On Sun, Mar 11, 2012 at 08:53:57PM +0000, KY Srinivasan wrote:

quoted

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
Sent: Sunday, March 11, 2012 2:49 PM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; virtualization@lists.osdl.org; ohering@suse.com;
Alan Stern
Subject: Re: [PATCH 2/4] Drivers: hv: Support the newly introduced KVP
messages in the driver

On Sun, Mar 11, 2012 at 04:56:06PM +0000, KY Srinivasan wrote:

quoted

quoted

Probably that's not enough to make a difference and we'd need to
introduce a new function.

Btw I don't know if utf16s_to_utf8s() counts the NUL char or not.
It feels like maybe we could end up with ->value_size equal to
HV_KVP_EXCHANGE_MAX_VALUE_SIZE + 1.

The MAX value is set to accommodate the maximum string that will ever
be handled including the string terminator. The function utf16s_to_utf8s()
returns the converted string length but the returned length does not
include the string terminator (like strlen), hence the "+1".

sprintf() and friends copy the NUL terminator but utf16s_to_utf8s()
doesn't so the code isn't right and it does seem like maybe we could
end up with a ->value_size equal to HV_KVP_EXCHANGE_MAX_VALUE_SIZE +
1.

You are right in that utf16s_to_utf8s() does not copy the string
terminator. This is not an issue in this case since the buffer for
the utf8 string is zeroed out to begin with (this memory was
allocated using kzalloc()). The return value of the
utf16s_to_utf8s() is the length of the utf8s string as what would
be returned by strlen.

There is no strlen() involved...  It returns the number of bytes
copied to the output string.  It doesn't copy a NUL.  We pass
HV_KVP_EXCHANGE_MAX_VALUE_SIZE bytes as the limit.  So it fills up
the buffer with non-null characters and we have an off-by-one.

I add one to take into account the string
terminator character for further processing. As I said before the
MAX value takes into account the terminating character for all the
strings handled.

So you're saying that since we control the input string, we'll never
hit the max?  Still, why not pass HV_KVP_EXCHANGE_MAX_VALUE_SIZE - 1
to leave room for the NUL just for correctness?  We'd still add one
to the return value but we wouldn't go over the size of the buffer.

Again, I don't really know how utf16s_to_utf8s() works so I might
have misunderstood.

regards,
dan carpenter