Re: [advapi32/service #2] Fix buffersize calculations for GetServiceDisplayNameA

[Alexandre Julliard]

Paul Vriens  writes:
> diff --git a/dlls/advapi32/service.c b/dlls/advapi32/service.c
> index 1ecaf70..7e4ac8e 100644
> --- a/dlls/advapi32/service.c
> +++ b/dlls/advapi32/service.c
> @@ -2298,7 +2298,7 @@ BOOL WINAPI GetServiceDisplayNameA( SC_HANDLE hSCManager, LPCSTR lpServiceName,
>          if (ret == ERROR_MORE_DATA)
>          {
>              SetLastError(ERROR_INSUFFICIENT_BUFFER);
> -
>        *lpcchBuffer = size - 1;
> -
>        *lpcchBuffer = (size - 1) * sizeof(WCHAR);

That's not correct, the A function is not supposed to return WCHARs.
What probably happens on Windows is that it calls the W function and
doubles the size as an upper bound on the buffer size.

[advapi32/service #2] Fix buffersize calculations for GetServiceDisplayNameA

[Paul Vriens]

Alexandre Julliard wrote:
> Paul Vriens  writes:
> > diff --git a/dlls/advapi32/service.c b/dlls/advapi32/service.c
> > index 1ecaf70..7e4ac8e 100644
> > --- a/dlls/advapi32/service.c
> > +++ b/dlls/advapi32/service.c
> > @@ -2298,7 +2298,7 @@ BOOL WINAPI GetServiceDisplayNameA( SC_HANDLE hSCManager, LPCSTR lpServiceName,
> >          if (ret == ERROR_MORE_DATA)
> >          {
> >              SetLastError(ERROR_INSUFFICIENT_BUFFER);
> > -
> >        *lpcchBuffer = size - 1;
> > -
> >        *lpcchBuffer = (size - 1) * sizeof(WCHAR);
>
> That's not correct, the A function is not supposed to return WCHARs.
> What probably happens on Windows is that it calls the W function and
> doubles the size as an upper bound on the buffer size.

So how do you suggest I'm going to fix this? Maybe we should call the W function 
as well as deal with the differences?

Cheers,

Paul.

[advapi32/service #2] Fix buffersize calculations for GetServiceDisplayNameA

[Alexandre Julliard]

Paul Vriens  writes:
> So how do you suggest I'm going to fix this? Maybe we should call the
> W function as well as deal with the differences?

If you want to replicate the exact behavior then yes you have to call
the W function. But returning the correct size is probably OK too, at
least until we find an app that depends on the broken Windows
behavior...

[advapi32/service #2] Fix buffersize calculations for GetServiceDisplayNameA

[Paul Vriens]

Alexandre Julliard wrote:
> Paul Vriens  writes:
> > So how do you suggest I'm going to fix this? Maybe we should call the
> > W function as well as deal with the differences?
>
> If you want to replicate the exact behavior then yes you have to call
> the W function. But returning the correct size is probably OK too, at
> least until we find an app that depends on the broken Windows
> behavior...

Well I just sent in 2 patches for extra tests. The second one means we have to 
implement a chunk of code and again for 2 functions. I'll have a look if it's 
feasible to call W from A.

It would have been worse if the broken scenario was returning a too small 
buffersize.

Cheers,

Paul.

[advapi32/service #2] Fix buffersize calculations for GetServiceDisplayNameA

[Paul Vriens]

Alexandre Julliard wrote:
> Paul Vriens  writes:
> > So how do you suggest I'm going to fix this? Maybe we should call the
> > W function as well as deal with the differences?
>
> If you want to replicate the exact behavior then yes you have to call
> the W function. But returning the correct size is probably OK too, at
> least until we find an app that depends on the broken Windows
> behavior...

Hi again,

What about the attached patch? This fixes all todo_wine's for GetDisplayName 
with respect to buffer sizes.

Cheers,

Paul.

[advapi32/service #2] Fix buffersize calculations for GetServiceDisplayNameA

[Paul Vriens]

Paul Vriens wrote:
> Alexandre Julliard wrote:
> > Paul Vriens  writes:
> > > So how do you suggest I'm going to fix this? Maybe we should call the
> > > W function as well as deal with the differences?
> >
> > If you want to replicate the exact behavior then yes you have to call
> > the W function. But returning the correct size is probably OK too, at
> > least until we find an app that depends on the broken Windows
> > behavior...
>
> Hi again,
>
> What about the attached patch? This fixes all todo_wine's for 
> GetDisplayName with respect to buffer sizes.
>
> Cheers,
>
> Paul.

I could of course leave the 'not-so-clean' way of setting the size out:

if (!lpDisplayName && lpcchBuffer && !ret && (GLE ==
ERROR_INSUFFICIENT_BUFFER))
     {
         /* Request for buffersize.
          *
          * Only set the size for ERROR_INSUFFICIENT_BUFFER
          */
         size = sizeW * sizeof(WCHAR);
     }
     else if (lpDisplayName && lpcchBuffer && !ret)
     {
         /* Request for displayname.
          *
          * size has to be set if this fails
          */
         size = sizeW * sizeof(WCHAR);
     }

This will only mean we leave a few todo_wine's in that will be accompanied with
a comment.

Cheers,

Paul.

[advapi32/service #2] Fix buffersize calculations for GetServiceDisplayNameA

[Alexandre Julliard]

Paul Vriens  writes:
> I could of course leave the 'not-so-clean' way of setting the size out:
> if (!lpDisplayName && lpcchBuffer && !ret && (GLE ==
>
> ERROR_INSUFFICIENT_BUFFER))
>     {
>         /* Request for buffersize.
>          *
>          * Only set the size for ERROR_INSUFFICIENT_BUFFER
>          */
>         size = sizeW * sizeof(WCHAR);
>     }
>     else if (lpDisplayName && lpcchBuffer && !ret)
>     {
>         /* Request for displayname.
>          *
>          * size has to be set if this fails
>          */
>         size = sizeW * sizeof(WCHAR);
>     }

This shouldn't be sizeof(WCHAR), it should be 2 as in "at most 2 A
chars for each W char". Of course the end result is the same...

[advapi32/service #2] Fix buffersize calculations for GetServiceDisplayNameA

[Paul Vriens]

Alexandre Julliard wrote:
> Paul Vriens  writes:
> > I could of course leave the 'not-so-clean' way of setting the size out:
> > if (!lpDisplayName && lpcchBuffer && !ret && (GLE ==
> >
> > ERROR_INSUFFICIENT_BUFFER))
> >     {
> >         /* Request for buffersize.
> >          *
> >          * Only set the size for ERROR_INSUFFICIENT_BUFFER
> >          */
> >         size = sizeW * sizeof(WCHAR);
> >     }
> >     else if (lpDisplayName && lpcchBuffer && !ret)
> >     {
> >         /* Request for displayname.
> >          *
> >          * size has to be set if this fails
> >          */
> >         size = sizeW * sizeof(WCHAR);
> >     }
>
> This shouldn't be sizeof(WCHAR), it should be 2 as in "at most 2 A
> chars for each W char". Of course the end result is the same...

Yeah, I thought about that because it could be confusing.

Will change that and sent the new patches.

Cheers,

Paul.