[Spice-devel] [vdagent-win PATCH v4 07/19] Allow one more character reading strings from registry

[Jonathon Jongsma]

I guess this avoids the error I just pointed out in the previous patch.
   This patch looks fine, but the previous patch should still be fixed
in case this one gets reverted sometime in the future for some reason.

One minor comment below.

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>


On Mon, 2018-07-02 at 08:43 +0100, Frediano Ziglio wrote:
> The strings in the registry are usually NUL-terminated but this
> is not a requirement.
> Handle the case when the string, considering the terminator, fit
> into the reading buffer. In this case accept the string. In the
> case the string fit into the buffer but is not terminated
> returns ERROR_MORE_DATA (the error that would be returned if the
> string didn't fit in the buffer as there is no place to add the
> terminator).
> 
> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> ---
>  vdagent/display_setting.cpp | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/vdagent/display_setting.cpp
> b/vdagent/display_setting.cpp
> index b68ef1c..4b47276 100644
> --- a/vdagent/display_setting.cpp
> +++ b/vdagent/display_setting.cpp
> @@ -295,6 +295,20 @@ static bool RegReadString(HKEY key, const TCHAR
> *name, TCHAR *buffer, size_t buf
>      LONG status;
>  
>      status = RegQueryValueEx(key, name, NULL, &value_type,
> (LPBYTE)buffer, &value_size);
> +    if (status == ERROR_SUCCESS && value_type == REG_SZ) {
> +        // assure NUL-terminated
> +        value_size /= sizeof(buffer[0]);
> +        if (value_size == buffer_len) {
> +            // full buffer but not terminated?
> +            if (buffer[value_size-1] != '\0') {
> +                status = ERROR_MORE_DATA;
> +            }
> +        } else {

I'm being overly paranoid here since ReadQueryValueEx() surely won't
return a value greater than buffer_len. But looking only at the logic
of code, value_size could theoretically be greater or less than
buffer_len here. Is it worth asserting? Or something else?

> +            // append a NUL. If there's already a NUL character this
> +            // new one will be ignored
> +            buffer[value_size] = '\0';
> +        }
> +    }
>      if (status != ERROR_SUCCESS) {
>          vd_printf("RegQueryValueEx(%" PRIsTSTR ") : fail %ld", name,
> status);
>          return false;
> @@ -305,12 +319,6 @@ static bool RegReadString(HKEY key, const TCHAR
> *name, TCHAR *buffer, size_t buf
>          return false;
>      }
>  
> -    // assure NUL-terminated
> -    value_size /= sizeof(buffer[0]);
> -    if (!value_size || buffer[value_size - 1] != '\0') {
> -        buffer[value_size] = '\0';
> -    }
> -
>      return true;
>  }
>