Re: [ros-dev] [ros-diffs] [jgardou] 65210: [NTOS/PS] - Do not leak a reference to the process object when setting quotas.

[Timo Kreuzer]

Doesn't this change Windows behaviour?
No, luckily it does not change *Windows* behaviour.
It does not even change ReactOS behaviour, but only, because it was 
already broken before.
Since MS implemented this function in an arbitrary and hard to 
comprehend way, we need to follow this pattern. This means, the function 
must not fail on process reference failure,  unless certain conditions 
are met, otherwise we will fail with the wrong Status code.
To fix the situation, we could remember the fact that the process 
couldn't be referenced (possibly by setting Process to NULL) and later 
check that after other checks were done. The alternative is referencing 
the process individually in each and every case, which will make the 
function even more huge and does not really help with it's readability. 
Or we can factor our each case and implement it as a separate function. 
It would add a lot of code duplication, but might help a bit with 
readability and maintainability.

Am 03.11.2014 10:52, schrieb jgardou(a)svn.reactos.org:
>   Author: jgardou
>  Date: Mon Nov  3 09:52:08 2014
>  New Revision: 65210
>
>  URL: http://svn.reactos.org/svn/reactos?rev=65210&view=rev
>  Log:
>  [NTOS/PS]
>    - Do not leak a reference to the process object when setting quotas.
>
>  Modified:
>       trunk/reactos/ntoskrnl/include/internal/ps.h
>       trunk/reactos/ntoskrnl/ps/query.c
>       trunk/reactos/ntoskrnl/ps/quota.c
>
>  Modified: trunk/reactos/ntoskrnl/include/internal/ps.h
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/…
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/include/internal/ps.h	[iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/include/internal/ps.h	[iso-8859-1] Mon Nov  3 09:52:08 2014
>  @@ -303,7 +303,7 @@
>    NTSTATUS
>    NTAPI
>    PspSetQuotaLimits(
>  -    _In_ HANDLE ProcessHandle,
>  +    _In_ PEPROCESS Process,
>        _In_ ULONG Unused,
>        _In_ PVOID QuotaLimits,
>        _In_ ULONG QuotaLimitsLength,
>
>  Modified: trunk/reactos/ntoskrnl/ps/query.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ps/query.c?rev=65…
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/ps/query.c	[iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/ps/query.c	[iso-8859-1] Mon Nov  3 09:52:08 2014
>  @@ -1528,6 +1528,7 @@
>                /* Validate the number */
>                if ((BasePriority > HIGH_PRIORITY) || (BasePriority <=
> LOW_PRIORITY))
>                {
>  +                ObDereferenceObject(Process);
>                    return STATUS_INVALID_PARAMETER;
>                }
>    
>  @@ -1918,11 +1919,12 @@
>    
>            case ProcessQuotaLimits:
>    
>  -            return PspSetQuotaLimits(ProcessHandle,
>  +            Status = PspSetQuotaLimits(Process,
>                                         1,
>                                         ProcessInformation,
>                                         ProcessInformationLength,
>                                         PreviousMode);
>  +            break;
>    
>            case ProcessWorkingSetWatch:
>                DPRINT1("WS watch not implemented\n");
>
>  Modified: trunk/reactos/ntoskrnl/ps/quota.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ps/quota.c?rev=65…
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/ps/quota.c	[iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/ps/quota.c	[iso-8859-1] Mon Nov  3 09:52:08 2014
>  @@ -292,14 +292,13 @@
>    NTSTATUS
>    NTAPI
>    PspSetQuotaLimits(
>  -    _In_ HANDLE ProcessHandle,
>  +    _In_ PEPROCESS Process,
>        _In_ ULONG Unused,
>        _In_ PVOID QuotaLimits,
>        _In_ ULONG QuotaLimitsLength,
>        _In_ KPROCESSOR_MODE PreviousMode)
>    {
>        QUOTA_LIMITS_EX CapturedQuotaLimits;
>  -    PEPROCESS Process;
>        PEPROCESS_QUOTA_BLOCK QuotaBlock, OldQuotaBlock;
>        BOOLEAN IncreaseOkay;
>        KAPC_STATE SavedApcState;
>  @@ -368,19 +367,6 @@
>        }
>        _SEH2_END;
>    
>  -    /* Reference the process */
>  -    Status = ObReferenceObjectByHandle(ProcessHandle,
>  -                                       PROCESS_SET_QUOTA,
>  -                                       PsProcessType,
>  -                                       PreviousMode,
>  -                                       (PVOID*)&Process,
>  -                                       NULL);
>  -    if (!NT_SUCCESS(Status))
>  -    {
>  -        DPRINT1("Failed to reference process handle: 0x%lx\n", Status);
>  -        return Status;
>  -    }
>  -
>        /* Check the caller changes the working set size limits */
>        if ((CapturedQuotaLimits.MinimumWorkingSetSize != 0) &&
>            (CapturedQuotaLimits.MaximumWorkingSetSize != 0))
>  @@ -418,7 +404,6 @@
>            /* Check if the caller has the required privilege */
>            if (!SeSinglePrivilegeCheck(SeIncreaseQuotaPrivilege, PreviousMode))
>            {
>  -            ObDereferenceObject(Process);
>                return STATUS_PRIVILEGE_NOT_HELD;
>            }
>    
>  @@ -460,8 +445,6 @@
>            Status = STATUS_SUCCESS;
>        }
>    
>  -    /* Dereference the process and return the status */
>  -    ObDereferenceObject(Process);
>        return Status;
>    }
>    
>
>
>