Re: [PATCH 2/3] server: Fix get_handle for global handles. It doesn't need process for them.

[Alexandre Julliard]

Vitaliy Margolen  writes:
> Resend as-is. I need this function to work on global handles when
> process is NULL.

If you need this you are doing something wrong. Callers are not
supposed to know that a handle is global.

[PATCH 2/3] server: Fix get_handle for global handles. It doesn't need process for them.

[Vitaliy Margolen]

Alexandre Julliard wrote:
> Vitaliy Margolen  writes:
> > Resend as-is. I need this function to work on global handles when
> > process is NULL.
>
> If you need this you are doing something wrong. Callers are not
> supposed to know that a handle is global.

http://source.winehq.org/source/server/handle.c#L523 calls
duplicate_handle with NULL process.

Vitaliy.

[PATCH 2/3] server: Fix get_handle for global handles. It doesn't need process for them.

[Alexandre Julliard]

Vitaliy Margolen  writes:
> http://source.winehq.org/source/server/handle.c#L523 calls
> duplicate_handle with NULL process.

Yes, but duplicate_handle has no reason to call get_handle on the dst
process. I see that your 3/3 patch does this, but that's broken.

[PATCH 2/3] server: Fix get_handle for global handles. It doesn't need process for them.

[Vitaliy Margolen]

Alexandre Julliard wrote:
> Vitaliy Margolen  writes:
> > http://source.winehq.org/source/server/handle.c#L523 calls
> > duplicate_handle with NULL process.
>
> Yes, but duplicate_handle has no reason to call get_handle on the dst

Sure there is. How else do I set the requested access mask?
> process. I see that your 3/3 patch does this, but that's broken.

Right that's the way it's designed by the original!

I tried to explain the reason by I guess all that and tests are not
enough. If you have a better solution, please implement it by all means.

I have other things to worry about ATM.

Vitaliy.

[PATCH 2/3] server: Fix get_handle for global handles. It doesn't need process for them.

[Alexandre Julliard]

Vitaliy Margolen  writes:
> Alexandre Julliard wrote:
> > Yes, but duplicate_handle has no reason to call get_handle on the dst
>
> Sure there is. How else do I set the requested access mask?

By creating the handle with the correct mask right away, instead of
setting a wrong mask and patching it up afterwards.

[PATCH 2/3] server: Fix get_handle for global handles. It doesn't need process for them.

[Vitaliy Margolen]

Alexandre Julliard wrote:
> Vitaliy Margolen  writes:
> > Alexandre Julliard wrote:
> > > Yes, but duplicate_handle has no reason to call get_handle on the dst
> >
> > Sure there is. How else do I set the requested access mask?
>
> By creating the handle with the correct mask right away, instead of
> setting a wrong mask and patching it up afterwards.

That can only be done if permissions are not checked. I thought it was
clear from the tests that even when expressly prohibited, you can create
a handle with full access rights to the process if you can duplicate handle.
I tried to keep all the changes in the one place (duplicate_handle) and
not spread them around into create_handle.

Vitaliy.

[PATCH 2/3] server: Fix get_handle for global handles. It doesn't need process for them.

[Alexandre Julliard]

Vitaliy Margolen  writes:
> Alexandre Julliard wrote:
> > Vitaliy Margolen  writes:
> > > Alexandre Julliard wrote:
> > > > Yes, but duplicate_handle has no reason to call get_handle on the dst
> > >
> > > Sure there is. How else do I set the requested access mask?
> >
> > By creating the handle with the correct mask right away, instead of
> > setting a wrong mask and patching it up afterwards.
>
> That can only be done if permissions are not checked. I thought it was
> clear from the tests that even when expressly prohibited, you can create
> a handle with full access rights to the process if you can duplicate handle.
> I tried to keep all the changes in the one place (duplicate_handle) and
> not spread them around into create_handle.

No, all the mess is because you added checks in alloc_handle (which is
fine) then try to hack around them in duplicate_handle. What you
should do is do the appropriate checks in duplicate_handle and then
call alloc_entry directly.