Re: [ros-dev] [ros-diffs] [tkreuzer] 56442: [PSEH] Fix a serious bug in PSEH that could lead to stack corruption and was causing CSRSS to crash, when large amounts of text were output on the console. Background: PSEH used _...

[Alex Ionescu]

Runner-up to Arty's patch of the year.

Best regards,
Alex Ionescu


On Sun, Apr 29, 2012 at 3:00 AM,  <tkreuzer(a)svn.reactos.org> wrote:
>   Author: tkreuzer
>  Date: Sat Apr 28 20:00:09 2012
>  New Revision: 56442
>
>  URL: http://svn.reactos.org/svn/reactos?rev=56442&view=rev
>  Log:
>  [PSEH]
>  Fix a serious bug in PSEH that could lead to stack corruption and was causing CSRSS to
> crash, when large amounts of text were output on the console.
>  Background: PSEH used __builtin_alloca to allocate an SEH registration frame on the stack
> on demand, ie only for the first try level. But there are some nasty things with
> __builtin_alloca: First it DOES NOT - as one might think - free the allocated memory, once
> the allocation "goes out of scope", like with local variables, but only on
> function exit. Therefore it cannot normally be used inside a loop. The trick that PSEH
> used to "fix" this problem, was to save the stack pointer and reset it back at
> the end. This is quite problematic, since the rest of the code might assume that the stack
> pointer is still where it was left off after the allocation. The other thing is that
> __builtin_alloca() can allocate the memory whenever it likes to. It can allocate
> everything on function entry or not allocate anything at all, when other stack variables
> that went out of scope have left enough space to be reused. In csrss it now happened that
> the allocation was done before the stack pointer was saved,
>   so the memory could not be freed by restoring the old stack pointer value. That lead to
> slowly eating up the stack since the code was inside a loop.
>  The allocation is now replaced with a variable sized array. The variable stays in scope
> until the _SEH2_END and will be automaticall cleaned up by the compiler. This also makes
> saving and restoring the stack pointer obsolete.
>  Reliability++
>
>  Modified:
>     trunk/reactos/include/reactos/libs/pseh/pseh2.h
>
>  Modified: trunk/reactos/include/reactos/libs/pseh/pseh2.h
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/include/reactos/libs/pseh/…
>  ==============================================================================
>  --- trunk/reactos/include/reactos/libs/pseh/pseh2.h [iso-8859-1] (original)
>  +++ trunk/reactos/include/reactos/libs/pseh/pseh2.h [iso-8859-1] Sat Apr 28 20:00:09
> 2012
>  @@ -192,7 +192,6 @@
>         if(_SEHTopTryLevel) \
>         { \
>                 _SEH2LeaveFrame(); \
>  -               __asm__ __volatile__("mov %0, %%esp" : : "g"
> (_SEHStackPointer)); \
>         }
>
>   #define __SEH_END_SCOPE_CHAIN \
>  @@ -219,16 +218,14 @@
>                         static const int _SEH2ScopeKind = 0; \
>                         volatile _SEH2TryLevel_t _SEHTryLevel; \
>                         volatile _SEH2HandleTryLevel_t _SEHHandleTryLevel; \
>  -                       void * _SEHStackPointer; \
>  +                       _SEH2Frame_t _SEH2Frame[_SEHTopTryLevel ? 1 : 0]; \
>                         volatile _SEH2TryLevel_t * _SEH2TryLevelP; \
>                         _SEH2Frame_t * const _SEH2FrameP = _SEHTopTryLevel ? \
>  -                               ({ __asm__ __volatile__("mov %%esp, %0" :
> "=g" (_SEHStackPointer)); __builtin_alloca(sizeof(_SEH2Frame_t)); }) : \
>  -                               _SEHCurFrameP; \
>  +                               _SEH2Frame : _SEHCurFrameP; \
>   \
>                         (void)_SEH2ScopeKind; \
>                         (void)_SEHTryLevel; \
>                         (void)_SEHHandleTryLevel; \
>  -                       (void)_SEHStackPointer; \
>                         (void)_SEH2FrameP; \
>                         (void)_SEH2TryLevelP; \
>   \
>
>