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

[Hans de Goede]

Hi,

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.

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);

Regards,

Hans


> ---
>   server/reds.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/server/reds.c b/server/reds.c
> index 98c8706..5a03043 100644
> --- a/server/reds.c
> +++ b/server/reds.c
> @@ -3952,7 +3952,7 @@ SPICE_GNUC_VISIBLE int spice_server_set_port(SpiceServer *s, int port)
>   SPICE_GNUC_VISIBLE void spice_server_set_addr(SpiceServer *s, const char *addr, int flags)
>   {
>       spice_assert(reds == s);
> -    strncpy(spice_addr, addr, sizeof(spice_addr));
> +    strncpy(spice_addr, addr, sizeof(spice_addr)-1);
>       if (flags & SPICE_ADDR_FLAG_IPV4_ONLY) {
>           spice_family = PF_INET;
>       }
> @@ -4043,7 +4043,7 @@ SPICE_GNUC_VISIBLE int spice_server_set_ticket(SpiceServer *s,
>           taTicket.expiration_time = now + lifetime;
>       }
>       if (passwd != NULL) {
> -        strncpy(taTicket.password, passwd, sizeof(taTicket.password));
> +        strncpy(taTicket.password, passwd, sizeof(taTicket.password)-1);
>       } else {
>           memset(taTicket.password, 0, sizeof(taTicket.password));
>           taTicket.expiration_time = 0;
>