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 <[e-mail] 
> > 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 <[e-mail]
> <mailto:[e-mail]>> 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  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 <[e-mail]
> > > 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 <[e-mail]
> > <mailto:[e-mail]>> 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.