Re: advapi: test and fix GetUserName() in case where buffer is too small

[Hans Leidekker]

On Mon, 2011-05-30 at 21:13 -0700, Dan Kegel wrote:
> @@ -91,7 +91,7 @@ GetUserNameW( LPWSTR lpszName, LPDWORD lpSize )
>  if (len > *lpSize)
>  {
> -
>    SetLastError(ERROR_MORE_DATA);
> -
>    SetLastError(ERROR_INSUFFICIENT_BUFFER);

GetUserNameA also needs to be fixed. It sets ERROR_MORE_DATA when
WideCharToMultiByte fails, which can simply be removed because that
function will set ERROR_INSUFFICIENT_BUFFER itself if the buffer is too
small.

And there are at least two cases, in dlls/crypt32/protectdata.c and
dlls/msi/package.c, where we depend on the wrong error.

advapi: test and fix GetUserName() in case where buffer is too small

[Andrew Nguyen]

On 05/31/2011 02:17 AM, Hans Leidekker wrote:
> On Mon, 2011-05-30 at 21:13 -0700, Dan Kegel wrote:
> > @@ -91,7 +91,7 @@ GetUserNameW( LPWSTR lpszName, LPDWORD lpSize )
> >  if (len > *lpSize)
> >  {
> > -
> >    SetLastError(ERROR_MORE_DATA);
> > -
> >    SetLastError(ERROR_INSUFFICIENT_BUFFER);
>
> GetUserNameA also needs to be fixed. It sets ERROR_MORE_DATA when
> WideCharToMultiByte fails, which can simply be removed because that
> function will set ERROR_INSUFFICIENT_BUFFER itself if the buffer is too
> small.
>
> And there are at least two cases, in dlls/crypt32/protectdata.c and
> dlls/msi/package.c, where we depend on the wrong error.

I didn't intend to scoop Dan's work, but I have a version of his fix,
which is attached, with deeper tests which I wrote before Hans expressed
his concerns. Should I just send in the tests with todo_wine and let Dan
fix those, or should I send a revised version and allow a "winner" to be
picked?

advapi: test and fix GetUserName() in case where buffer is too small

[Hans Leidekker]

On Tue, 2011-05-31 at 06:32 -0500, Andrew Nguyen wrote:
> - DWORD sizeW = *lpSize * 2;
> - DWORD sizeW = 0;
> -
> - GetUserNameW( NULL, &sizeW );

Can we avoid the extra call?
> I didn't intend to scoop Dan's work, but I have a version of his fix,
> which is attached, with deeper tests which I wrote before Hans expressed
> his concerns. Should I just send in the tests with todo_wine and let Dan
> fix those, or should I send a revised version and allow a "winner" to be
> picked?

I vote for getting some tests in first.

advapi: test and fix GetUserName() in case where buffer is too small

[Andrew Nguyen]

On 05/31/2011 08:17 AM, Hans Leidekker wrote:
> On Tue, 2011-05-31 at 06:32 -0500, Andrew Nguyen wrote:
> > - DWORD sizeW = *lpSize * 2;
> > - DWORD sizeW = 0;
> > -
> > - GetUserNameW( NULL, &sizeW );
>
> Can we avoid the extra call?

I was trying to avoid making the assumption that a Unicode buffer with
*lpSize Unicode characters would necessarily be appropriate for the
second GetUserNameW call. Considering the failure path for the
GetUserNameW call in the old version, I don't believe we know what the
number of ANSI characters actually is unless we had WideCharToMultiByte
figure it out. Thus, my intent was to never allow a failure due to
insufficient buffer size so GetUserNameA knows the appropriate value for
sizeA.

That said, I suspect Windows does allocate the Unicode buffer based on
the passed buffer size, so I think just going back to the old approach
of unconditionally allocating *lpSize Unicode characters should be okay
in practice.
> > I didn't intend to scoop Dan's work, but I have a version of his fix,
> > which is attached, with deeper tests which I wrote before Hans expressed
> > his concerns. Should I just send in the tests with todo_wine and let Dan
> > fix those, or should I send a revised version and allow a "winner" to be
> > picked?
>
> I vote for getting some tests in first.

I'll go ahead and extract the tests to a separate patch then.

advapi: test and fix GetUserName() in case where buffer is too small

[Dan Kegel]

I'm looking forward to Andrew's test-only patch...

When I saw he had a test on testbot, I probably should
have dropped mine.