[ros-dev] [ros-diffs] [hbelusca] 57504: [NTOSKRNL] Rearrange the NtQuerySystemEnvironmentValue code to have successive logical checks.

[Timo Kreuzer]

The question that you need to answer yourself is not whether something 
is more logical or cleaner or whatever, but the question is: How does it 
work on Windows.
For all internal APIs you can do as you like, as long as it doesn't 
affect the externally visible behaviour. But in this case we have an 
external API, which should
function 100% like the original (if possible)

Regards,
Timo


Am 07.10.2012 11:24, schrieb Hermès BÉLUSCA - MAÏTO:
> Hello !
>
> If you want I will try to write a test ; however if I made this
> rearrangement it's because it makes the code more "logic" : first of all you
> probe for read/write the arguments then you check whether or not you have
> the right to continue (thus you don't start un-useful operation if you don't
> have right to do so), then, in case of success, you initialize the variable
> and AT THE VERY LAST, the user buffer just before filling it with the
> information you gather.
>
> Hermes
>
> -----Message d'origine-----
> De : ros-dev-bounces at reactos.org [mailto:ros-dev-bounces at reactos.org] De la
> part de Timo Kreuzer
> Envoyé : samedi 6 octobre 2012 22:21
> À : ros-dev at reactos.org
> Objet : Re: [ros-dev] [ros-diffs] [hbelusca] 57504: [NTOSKRNL] Rearrange the
> NtQuerySystemEnvironmentValue code to have successive logical checks.
>
>
> This changes the logic. I'm not sure how exact the previous version was, but
> these changes should be tested.
>
>
> Am 06.10.2012 21:50, schrieb hbelusca at svn.reactos.org:
>> Author: hbelusca
>> Date: Sat Oct  6 19:50:17 2012
>> New Revision: 57504
>>
>> URL: http://svn.reactos.org/svn/reactos?rev=57504&view=rev
>> Log:
>> [NTOSKRNL]
>> Rearrange the NtQuerySystemEnvironmentValue code to have successive
> logical checks.
>> Modified:
>>       trunk/reactos/ntoskrnl/ex/sysinfo.c
>>
>> Modified: trunk/reactos/ntoskrnl/ex/sysinfo.c
>> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ex/sysinfo.c?rev=5
> 7504&r1=57503&r2=57504&view=diff
> ============================================================================
> ==
>> --- trunk/reactos/ntoskrnl/ex/sysinfo.c [iso-8859-1] (original)
>> +++ trunk/reactos/ntoskrnl/ex/sysinfo.c [iso-8859-1] Sat Oct  6 19:50:17
> 2012
>> @@ -235,44 +235,40 @@
>>                _SEH2_YIELD(return _SEH2_GetExceptionCode());
>>            }
>>            _SEH2_END;
>> -
>> -    }
>> -
>> -    /* Allocate a buffer for the value */
>> -    AnsiValueBuffer = ExAllocatePoolWithTag(NonPagedPool,
> ValueBufferLength, 'pmeT');
>> -    if (AnsiValueBuffer == NULL)
>> -    {
>> -        return STATUS_INSUFFICIENT_RESOURCES;
>> -    }
>> -
>> -    /*
>> -     * Copy the name to kernel space if necessary and convert it to ANSI.
>> -     */
>> +    }
>> +
>> +    /* According to NTInternals the SeSystemEnvironmentName privilege is
> required! */
>> +    if (!SeSinglePrivilegeCheck(SeSystemEnvironmentPrivilege,
> PreviousMode))
>> +    {
>> +        DPRINT1("NtQuerySystemEnvironmentValue: Caller requires the
> SeSystemEnvironmentPrivilege privilege!\n");
>> +        return STATUS_PRIVILEGE_NOT_HELD;
>> +    }
>> +
>> +    /* Copy the name to kernel space if necessary */
>>        Status = ProbeAndCaptureUnicodeString(&WName, PreviousMode,
> VariableName);
>>        if (!NT_SUCCESS(Status))
>>        {
>>            return Status;
>>        }
>>    
>> -    /*
>> -     * according to ntinternals the SeSystemEnvironmentName privilege is
> required!
>> -     */
>> -    if (!SeSinglePrivilegeCheck(SeSystemEnvironmentPrivilege,
> PreviousMode))
>> -    {
>> -        ReleaseCapturedUnicodeString(&WName, PreviousMode);
>> -        DPRINT1("NtQuerySystemEnvironmentValue: Caller requires the
> SeSystemEnvironmentPrivilege privilege!\n");
>> -        return STATUS_PRIVILEGE_NOT_HELD;
>> -    }
>> -
>> -    /* Convert the value name to ansi and release the captured unicode
> string */
>> +    /* Convert the name to ANSI and release the captured UNICODE string
> */
>>        Status = RtlUnicodeStringToAnsiString(&AName, &WName, TRUE);
>>        ReleaseCapturedUnicodeString(&WName, PreviousMode);
>>        if (!NT_SUCCESS(Status)) return Status;
>>    
>> -    /* Get the environment variable */
>> +    /* Allocate a buffer for the ANSI environment variable */
>> +    AnsiValueBuffer = ExAllocatePoolWithTag(NonPagedPool,
> ValueBufferLength, 'pmeT');
>> +    if (AnsiValueBuffer == NULL)
>> +    {
>> +        RtlFreeAnsiString(&AName);
>> +        return STATUS_INSUFFICIENT_RESOURCES;
>> +    }
>> +
>> +    /* Get the environment variable and free the ANSI name */
>>        Result = HalGetEnvironmentVariable(AName.Buffer,
>>                                           (USHORT)ValueBufferLength,
>>                                           AnsiValueBuffer);
>> +    RtlFreeAnsiString(&AName);
>>    
>>        /* Check if we had success */
>>        if (Result == ESUCCESS)
>> @@ -280,13 +276,13 @@
>>            /* Copy the result back to the caller. */
>>            _SEH2_TRY
>>            {
>> -            /* Initialize ansi string from the result */
>> +            /* Initialize ANSI string from the result */
>>                RtlInitAnsiString(&AValue, AnsiValueBuffer);
>>    
>> -            /* Initialize a unicode string from the callers buffer */
>> +            /* Initialize a UNICODE string from the callers buffer */
>>                RtlInitEmptyUnicodeString(&WValue, ValueBuffer,
> ValueBufferLength);
>>    
>> -            /* Convert the result to unicode */
>> +            /* Convert the result to UNICODE */
>>                Status = RtlAnsiStringToUnicodeString(&WValue, &AValue,
> FALSE);
>>    
>>                if (ReturnLength != NULL)
>> @@ -305,8 +301,7 @@
>>            Status = STATUS_UNSUCCESSFUL;
>>        }
>>    
>> -    /* Cleanup allocated resources. */
>> -    RtlFreeAnsiString(&AName);
>> +    /* Free the allocated ANSI value buffer */
>>        ExFreePoolWithTag(AnsiValueBuffer, 'pmeT');
>>    
>>        return Status;
>>
>>
>>
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
>