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,  [e-mail] 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/p...
> ==============================================================================
> --- 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; \
>  \