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

["Tomáš Golembiovský" <tgolembi@redhat.com>]

On Wed, 25 Oct 2017 16:58:07 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Quoting Tomáš Golembiovský (2017-09-29 16:31:02)
> > On Fri, 29 Sep 2017 17:11:22 +0800
> > Chen Hanxiao <chen_han_xiao@126.com> wrote:
> >   
> > > From: Chen Hanxiao <chenhanxiao@gmail.com>
> > > 
> > > 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 <chenhanxiao@gmail.com>
> > > ---
> > >  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ý <tgolembi@redhat.com>  
> 
> 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);
 

-- 
Tomáš Golembiovský <tgolembi@redhat.com>