>
> 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?
>
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;
> > }
> >
>