[Spice-devel] [PATCH 1/2] Make sure strncpy'ed string are 0-terminated

[Hans de Goede]

Hi,

On 11/23/2012 01:42 PM, Christophe Fergeau wrote:
> On Fri, Nov 23, 2012 at 01:25:55PM +0100, Hans de Goede wrote:
>> On 11/23/2012 01:18 PM, Christophe Fergeau wrote:
>>> spice_server_set_ticket and spice_server_set_addr get (library)
>>> user-provided strings as arguments, and copy them to fixed-size
>>> buffers using strncpy. However, if these strings are too long,
>>> the copied string will not be 0-terminated, which will cause issues
>>> later. This commit copies one byte less than the size of the
>>> destination buffer. In both cases, this buffer is a static global
>>> variable, so its memory will be set to 0.
>>
>> You're being too clever here, this commit message may explain
>> why explicitly adding the terminating 0 is not necessary even
>> though it usually is with strncpy, but someone just reading the
>> code won't know that.
>
> I agree with you that it's quite magic, I just imitated what was done
> elsewhere in this file where strncpy is used.
>
>>
>> Also because of being way to easy to mis-use strncpy should just
>> die! So I suggest you redo this patch using snprintf instead
>> of strncpy, like this:
>>
>> snprintf(dst, sizeof(dst), "%s", src);
>
> I'll post a followup patch replacing these sizeof()-1 with something else,
> though I have a slight preference for adding a spice_strncpy which always
> nul-terminate the string. Are you ok with that, or do you prefer that we
> use snprintf?

I'm fine with adding a "safe" strcpy but please don't call it
spice_strncpy, as people expect certain (broken) behavior of strncpy, I
would go for the good old bsd strlcpy instead, so add a spice_strlcpy
and add that.

Hmm, doesn't glib have something for this? Aren't we already using glib
in spice-server ? If not it really is time we start using it, as qemu
is using it too...

Regards,

Hans