Re: [PATCH v1] kernel32: make GetEnvironmentStringsW returns a copy of the environment

[Nikolay Sivov]

On 3/17/19 6:27 PM, Jon Doron wrote:
> Also one thing worth noting how do you want me to code a test for 
> this? This part in the PEB is opaque 
> (https://docs.microsoft.com/en-us/windows/desktop/api/winternl/)

Just call it two times and compare pointers.
> Thanks,
> Jon.
>
> On Sun, Mar 17, 2019, 17:23 Jon Doron <arilou(a)gmail.com 
> <mailto:arilou(a)gmail.com>> wrote:
>
> Hi Nikolay I looked and noticed on XP the behavior is what's
>     currently in Wine but from NT6 aka Vista it's the new behavior
>     that I have implemented
>
> Do you want me to code a test that runs only if it's nt 6 and above?

It should run on both, with one case marked broken() for example. I 
think the real question is why this has anything to do with PEB lock, if 
current implementation does not require locking.
> Thanks,
>     Jon.
>
> On Sat, Mar 16, 2019, 21:02 Nikolay Sivov <nsivov(a)codeweavers.com
>     <mailto:nsivov(a)codeweavers.com>> wrote:
>
> On 3/16/19 9:57 PM, Jon Doron wrote:
>         > There are certain applications which try to traverse the
>         environement
>         > being returned, but this is problematic since they cannot
>         acquire the
>         > PEB Lock (i.e cl.exe on Visual Studio 14.15) . To resolve
>         the issue
>         > provide a copy of the current environment same as in
>         > GetEnvironmentStringsA .
>         Please add a test to confirm this change.

[PATCH v1] kernel32: make GetEnvironmentStringsW returns a copy of the environment

[Jon Doron]

Good idea! :) ill do that, as for why the PEB lock matters, in CL.exe
there is a code that does the following:
PWCHAR GetMergedEnv(PWCHAR MoreEnv)
{
    ULONG Len;
    PWCHAR Env = GetEnviornmentStringW();
    Len = GetEnvLength(Env) + GetEnvLength(MoreEnv);   // while it
traverses the Env in the PEB without a lock there is another thread
which is doing SetEnvVar, which modifies the buffer it traverses

                      // this leads into the wrong Len being
calulcated... and eventually the fatal error during Concat...
    PWCHAR NewEnv = malloc(Len);
    ConcatEnv(NewEnv, Env);
    ConcatEnv(NewEnv, MoreEnv);
    return NewEnv;
}

I'll send a new patchset with a test tomorrow
-- Jon.

On Sun, Mar 17, 2019 at 5:51 PM Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
> On 3/17/19 6:27 PM, Jon Doron wrote:
> > Also one thing worth noting how do you want me to code a test for
> > this? This part in the PEB is opaque
> > (https://docs.microsoft.com/en-us/windows/desktop/api/winternl/)
>
> Just call it two times and compare pointers.
> > Thanks,
> > Jon.
> >
> > On Sun, Mar 17, 2019, 17:23 Jon Doron <arilou(a)gmail.com
> > <mailto:arilou(a)gmail.com>> wrote:
> >
> > Hi Nikolay I looked and noticed on XP the behavior is what's
> >     currently in Wine but from NT6 aka Vista it's the new behavior
> >     that I have implemented
> >
> > Do you want me to code a test that runs only if it's nt 6 and above?
>
> It should run on both, with one case marked broken() for example. I
> think the real question is why this has anything to do with PEB lock, if
> current implementation does not require locking.
> > Thanks,
> >     Jon.
> >
> > On Sat, Mar 16, 2019, 21:02 Nikolay Sivov <nsivov(a)codeweavers.com
> >     <mailto:nsivov(a)codeweavers.com>> wrote:
> >
> > On 3/16/19 9:57 PM, Jon Doron wrote:
> >         > There are certain applications which try to traverse the
> >         environement
> >         > being returned, but this is problematic since they cannot
> >         acquire the
> >         > PEB Lock (i.e cl.exe on Visual Studio 14.15) . To resolve
> >         the issue
> >         > provide a copy of the current environment same as in
> >         > GetEnvironmentStringsA .
> >         Please add a test to confirm this change.