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

[Timo Kreuzer]

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

[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 : [e-mail] [mailto:[e-mail]] De la
part de Timo Kreuzer
Envoyé : samedi 6 octobre 2012 22:21
À : [e-mail]
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 [e-mail]:
> 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
[e-mail]
http://www.reactos.org/mailman/listinfo/ros-dev

[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 : [e-mail] [mailto:[e-mail]] De la
> part de Timo Kreuzer
> Envoyé : samedi 6 octobre 2012 22:21
> À : [e-mail]
> 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 [e-mail]:
> > 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
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev