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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> 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@xxxxxxxxxx>
> 
> 
> 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@xxxxxxxxxx>
> > ---
> >  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?
> 

Can't be greater, RegQueryValueEx would return ERROR_MORE_DATA.
And we trust the APIs we call, if memcpy copies more than the value
we requested there's no assert that can fix this!

> > +            // 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;
> >  }
> >  
> 
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]