Re: [ros-dev] [ros-diffs] [tfaber] 66365: [KERNEL32] - Make BaseSetLastNTError return the converted Win32 error code. This will determine the upper 24 bits of EAX in functions that return BOOLEAN FALSE right after calling B...

[Timo Kreuzer]

For me this looks like a hack. It might "do what Windows does", but the 
result is more based on luck / random compiler behaviour rather than 
deterministic behavior. We should think about returning a full ULONG in 
all functions that rely on this (like Wow64EnableWow64FsRedirection), 
containing the correct value, rather than relying on random stuff.


Am 20.02.2015 um 08:03 schrieb tfaber(a)svn.reactos.org:
>   Author: tfaber
>  Date: Fri Feb 20 07:03:00 2015
>  New Revision: 66365
>
>  URL: http://svn.reactos.org/svn/reactos?rev=66365&view=rev
>  Log:
>  [KERNEL32]
>  - Make BaseSetLastNTError return the converted Win32 error code. This will determine the
> upper 24 bits of EAX in functions that return BOOLEAN FALSE right after calling
> BaseSetLastNTError, e.g. Wow64EnableWow64FsRedirection. Fixes installers using WiX Toolset
> (e.g. VS2012 redist) on MSVC builds.
>  See http://wixtoolset.org/issues/4681/ for the WiX bug that causes this.
>  CORE-8010
>
>  Modified:
>       trunk/reactos/dll/win32/kernel32/client/except.c
>       trunk/reactos/dll/win32/kernel32/include/kernel32.h
>
>  Modified: trunk/reactos/dll/win32/kernel32/client/except.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/…
>  ==============================================================================
>  --- trunk/reactos/dll/win32/kernel32/client/except.c	[iso-8859-1] (original)
>  +++ trunk/reactos/dll/win32/kernel32/client/except.c	[iso-8859-1] Fri Feb 20 07:03:00
> 2015
>  @@ -682,12 +682,16 @@
>    /*
>     * @implemented
>     */
>  -VOID
>  +DWORD
>    WINAPI
>    BaseSetLastNTError(IN NTSTATUS Status)
>    {
>  +    DWORD dwErrCode;
>  +
>        /* Convert from NT to Win32, then set */
>  -    SetLastError(RtlNtStatusToDosError(Status));
>  +    dwErrCode = RtlNtStatusToDosError(Status);
>  +    SetLastError(dwErrCode);
>  +    return dwErrCode;
>    }
>    
>    /*
>
>  Modified: trunk/reactos/dll/win32/kernel32/include/kernel32.h
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/include…
>  ==============================================================================
>  --- trunk/reactos/dll/win32/kernel32/include/kernel32.h	[iso-8859-1] (original)
>  +++ trunk/reactos/dll/win32/kernel32/include/kernel32.h	[iso-8859-1] Fri Feb 20 07:03:00
> 2015
>  @@ -353,7 +353,7 @@
>    WINAPI
>    InitCommandLines(VOID);
>    
>  -VOID
>  +DWORD
>    WINAPI
>    BaseSetLastNTError(IN NTSTATUS Status);
>    
>
>
>

[Thomas Faber]

I agree and I'm not super happy with this. The two reasons I did it
this hackish way anyway are
* Changing the return types of these functions in our implementation is
  pretty painful and confusing, it'll create differences between
  headers and implementation and/or require redirects in the spec file
  etc.
* I did not want to go through the trouble of locating all such
  functions (maybe it's only the one, who knows), and am not
  particularly keen on having to keep in mind this strategy as new
  functions get added.

Basically, I just wanted to make the app work instead of developing a
well-designed scheme to properly handle this situation.
In fact it seems pretty hard to design one... looks like you'd have to
stop using BOOLEAN return completely and use BOOL instead, but then
there could be applications with the opposite bug where they _only_
work because they interpret the return value incorrectly.
I'd simply prefer not having to debug a second issue of this kind any
time soon because we missed something or our design was flawed. However
if you see a good solution that I'm missing, I'm be happy to go with it.


On 2015-02-21 00:17, Timo Kreuzer wrote:
>   
>  For me this looks like a hack. It might "do what Windows does", but the 
>  result is more based on luck / random compiler behaviour rather than 
>  deterministic behavior. We should think about returning a full ULONG in 
>  all functions that rely on this (like Wow64EnableWow64FsRedirection), 
>  containing the correct value, rather than relying on random stuff.
>  
>  
>  Am 20.02.2015 um 08:03 schrieb tfaber(a)svn.reactos.org:
> > Author: tfaber
> > Date: Fri Feb 20 07:03:00 2015
> > New Revision: 66365
> >
> > URL: http://svn.reactos.org/svn/reactos?rev=66365&view=rev
> > Log:
> > [KERNEL32]
> > - Make BaseSetLastNTError return the converted Win32 error code. This will determine
> the upper 24 bits of EAX in functions that return BOOLEAN FALSE right after calling
> BaseSetLastNTError, e.g. Wow64EnableWow64FsRedirection. Fixes installers using WiX Toolset
> (e.g. VS2012 redist) on MSVC builds.
> > See http://wixtoolset.org/issues/4681/ for the WiX bug that causes this.
> > CORE-8010
> >
> > Modified:
> >      trunk/reactos/dll/win32/kernel32/client/except.c
> >      trunk/reactos/dll/win32/kernel32/include/kernel32.h
> >
> > Modified: trunk/reactos/dll/win32/kernel32/client/except.c
> > URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/…
> > ==============================================================================
> > --- trunk/reactos/dll/win32/kernel32/client/except.c	[iso-8859-1] (original)
> > +++ trunk/reactos/dll/win32/kernel32/client/except.c	[iso-8859-1] Fri Feb 20 07:03:00
> 2015
> > @@ -682,12 +682,16 @@
> >   /*
> >    * @implemented
> >    */
> > -VOID
> > +DWORD
> >   WINAPI
> >   BaseSetLastNTError(IN NTSTATUS Status)
> >   {
> > +    DWORD dwErrCode;
> > +
> >       /* Convert from NT to Win32, then set */
> > -    SetLastError(RtlNtStatusToDosError(Status));
> > +    dwErrCode = RtlNtStatusToDosError(Status);
> > +    SetLastError(dwErrCode);
> > +    return dwErrCode;
> >   }
> >   
> >   /*
> >
> > Modified: trunk/reactos/dll/win32/kernel32/include/kernel32.h
> > URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/include…
> > ==============================================================================
> > --- trunk/reactos/dll/win32/kernel32/include/kernel32.h	[iso-8859-1] (original)
> > +++ trunk/reactos/dll/win32/kernel32/include/kernel32.h	[iso-8859-1] Fri Feb 20
> 07:03:00 2015
> > @@ -353,7 +353,7 @@
> >   WINAPI
> >   InitCommandLines(VOID);
> >   
> > -VOID
> > +DWORD
> >   WINAPI
> >   BaseSetLastNTError(IN NTSTATUS Status);
> >   
> >
> >
> >