Re: Added check for PROCESS_VM_WRITE, without which, some programs go into an infinite loop Henry Blum <[e-mail]>

[Andrey Turkin]

08.05.2010 9:10, Henry Blum wrote:
> ---
>  dlls/kernel32/process.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c
> index 9a1f2f5..6dcad8f 100644
> --- a/dlls/kernel32/process.c
> +++ b/dlls/kernel32/process.c
> @@ -2657,7 +2657,7 @@ HANDLE WINAPI OpenProcess( DWORD access, BOOL 
> inherit, DWORD id )
>      if (GetVersion() & 0x80000000) access = PROCESS_ALL_ACCESS;
>       status = NtOpenProcess(&handle, access, &attr, &cid);
> -    if (status != STATUS_SUCCESS)
> +    if (status != STATUS_SUCCESS || (access & PROCESS_VM_WRITE))
>      {
>          SetLastError( RtlNtStatusToDosError(status) );
>          return NULL;

Wouldn't this pretend operation failed for every request with 
PROCESS_VM_WRITE, even if NtOpenProcess succeeded, and leak a handle? 
Which bug should it fix?

Added check for PROCESS_VM_WRITE, without which, some programs go into an infinite loop Henry Blum <[e-mail]>

[Henry Blum]

I looked further into the problem I set out to fix, and realize that this patch is pretty terrible. The real bug is 22006 http://bugs.winehq.org/show_bug.cgi?id=22006 and affects the game Continuum http://appdb.winehq.org/appview.php?iVersionId=3703 basically, OpenProcess doesn't enfore ACLs

I am a user of the game, and the instructions that other users have provided involve a patch which allow the game to function. I was applying the patch from the git source, and wanted to try my hand at submitting a patch. The patch provided is described in the appdb page, and I thought I would improve it. 

That patch (not the one I submitted) wouldn't leak any handles, since it returns null before attempting NtOpenProcess when (access & PROCESS_VM_WRITE). I'm not sure if that is proper behavior, but it still seems dangerous to me.

Long story short, please disregard this patch, and while I will continue my efforts, any suggestions on how to approach bug 22006 would be greatly appreciated.

Many thanks,
Henry Blum

----- Original message -----
> 08.05.2010 9:10, Henry Blum wrote:
> > ---
> >   dlls/kernel32/process.c |      2 +-
> >   1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c
> > index 9a1f2f5..6dcad8f 100644
> > --- a/dlls/kernel32/process.c
> > +++ b/dlls/kernel32/process.c
> > @@ -2657,7 +2657,7 @@ HANDLE WINAPI OpenProcess( DWORD access, BOOL
> > inherit, DWORD id )
> >           if (GetVersion() & 0x80000000) access = PROCESS_ALL_ACCESS;
> >             status = NtOpenProcess(&handle, access, &attr, &cid);
> > -      if (status != STATUS_SUCCESS)
> > +      if (status != STATUS_SUCCESS || (access & PROCESS_VM_WRITE))
> >           {
> >                   SetLastError( RtlNtStatusToDosError(status) );
> >                   return NULL;
>
> Wouldn't this pretend operation failed for every request with
> PROCESS_VM_WRITE, even if NtOpenProcess succeeded, and leak a handle?
> Which bug should it fix?

Added check for PROCESS_VM_WRITE, without which, some programs go into an infinite loop Henry Blum <[e-mail]>

[Vitaliy Margolen]

On 05/09/2010 01:12 PM, Henry Blum wrote:
> I'm not sure if that is proper behavior
> It's not. That patch is an outright hack specifically for your game.

The proper way to fix it is to send correct ACLs to the server when new
process is created. Wine still doesn't do it.

Vitaliy.