Re: [Qemu-devel] [PATCH] qga-win: fall back to hardcoded user and group names if LookupAccountSidW failed

[Michael Roth]

Quoting Tomáš Golembiovský (2017-10-26 08:54:37)
> On Wed, 25 Oct 2017 16:58:07 -0500
> Michael Roth <address@hidden> wrote:
> 
> > Quoting Tomáš Golembiovský (2017-09-29 16:31:02)
> > > On Fri, 29 Sep 2017 17:11:22 +0800
> > > Chen Hanxiao <address@hidden> wrote:
> > >   
> > > > From: Chen Hanxiao <address@hidden>
> > > > 
> > > > On some of windows (win08 sp2),
> > > > it doesn't work by calling LookupAccountSidW with
> > > > well-known SIDs,
> > > > We got an error:
> > > >     error 997 overlapped I/O operation is in progress
> > > > 
> > > > But hardcoded names work.
> > > > 
> > > > This patch introduces a workaroud for this issue:
> > > > if LookupAccountSidW failed, try hardcoded one.
> > > > 
> > > > Signed-off-by: Chen Hanxiao <address@hidden>
> > > > ---
> > > >  qga/vss-win32/install.cpp | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > >   
> > > 
> > > I wonder if it's caused by qga itself or a race/conflict with some other
> > > app. But still...
> > > 
> > > 
> > > Reviewed-by: Tomáš Golembiovský <address@hidden>  
> > 
> > I think it might be getNameByStringSID()/LookupAccountSidW() reporting a
> > stale GetLastError() value.
> > 
> 
> I suppose you're right! Taking a closer look there's one more issue with
> getNameByStringSID(). The call ConvertStringSidToSidW() does not return
> HRESULT so using chk() makes no sense.
> 
> I propose slightly modified version of your fix:
> 
> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> index ba7c94eb25..65f68f94a3 100644
> --- a/qga/vss-win32/install.cpp
> +++ b/qga/vss-win32/install.cpp
> @@ -148,10 +148,15 @@ static HRESULT getNameByStringSID(
>      DWORD domainNameLen = BUFFER_SIZE;
>      wchar_t domainName[BUFFER_SIZE];
>  
> -    chk(ConvertStringSidToSidW(sid, &psid));
> -    LookupAccountSidW(NULL, psid, buffer, bufferLen,
> -                domainName, &domainNameLen, &groupType);
> -    hr = HRESULT_FROM_WIN32(GetLastError());
> +    if (!ConvertStringSidToSidW(sid, &psid) {
> +        hr = HRESULT_FROM_WIN32(GetLastError());
> +        goto out;
> +    }
> +    if (!LookupAccountSidW(NULL, psid, buffer, bufferLen,
> +                           domainName, &domainNameLen, &groupType)) {
> +        hr = HRESULT_FROM_WIN32(GetLastError());
> +        /* Fall through and free psid */
> +    }
>  
>      LocalFree(psid);


Thanks! It's not yet clear if this fixes the issue completely (though it
seems likely), but it's clearly a bug either way so I've gone ahead and
applied it to the qga tree:

  https://github.com/mdroth/qemu/commit/8cedc80555e5a395a4fda7130b0115015e775c48

Chen, if there's still an issue with the updated patch please let me know.

>  
> 
> -- 
> Tomáš Golembiovský <address@hidden>
>