Re: [PATCH 1/2] ntdll: Restore stack guard and prevent stack from shrinking

[Alexandre Julliard]

Piotr Caban  writes:
> @@ -1617,9 +1617,18 @@ BOOL virtual_handle_stack_fault( void *addr )
>          BYTE vprot = view->prot[((const char *)page - (const char *)view->base) >> page_shift];
>          if (vprot & VPROT_GUARD)
>          {
> -
>        struct _TEB *teb = NtCurrentTeb();
>        VIRTUAL_SetProt( view, page, page_size, vprot & ~VPROT_GUARD );
> -
>        if ((char *)page + page_size == NtCurrentTeb()->Tib.StackLimit)
> -
>            NtCurrentTeb()->Tib.StackLimit = page;
> -
>        if ((char *)page - page_size == teb->DeallocationStack)
> -
>            teb->Tib.StackLimit = page;
> -
>        else if ((char*)addr > (char*)teb->Tib.StackLimit + page_size &&
> -
>                teb->Tib.StackLimit == (char*)teb->DeallocationStack + page_size &&
> -
>                (view = VIRTUAL_FindView( (char*)teb->Tib.StackLimit, 0)))
> -
>        {
> -
>            vprot = view->prot[((char*)teb->Tib.StackLimit - (char*)view->base) >> page_shift];
> -
>            VIRTUAL_SetProt( view, teb->Tib.StackLimit, page_size, vprot | VPROT_GUARD);
> -
>            teb->Tib.StackLimit = (char*)teb->Tib.StackLimit + page_size;
> -
>        }

Why do you want to lookup the view again?

[PATCH 1/2] ntdll: Restore stack guard and prevent stack from shrinking

[Piotr Caban]

On 04/07/11 10:24, Alexandre Julliard wrote:
> Piotr Caban  writes:
> > @@ -1617,9 +1617,18 @@ BOOL virtual_handle_stack_fault( void *addr )
> >           BYTE vprot = view->prot[((const char *)page - (const char *)view->base)>>  page_shift];
> >           if (vprot&  VPROT_GUARD)
> >           {
> > -
> >        struct _TEB *teb = NtCurrentTeb();
> >         VIRTUAL_SetProt( view, page, page_size, vprot&  ~VPROT_GUARD );
> > -
> >        if ((char *)page + page_size == NtCurrentTeb()->Tib.StackLimit)
> > -
> >            NtCurrentTeb()->Tib.StackLimit = page;
> > -
> >        if ((char *)page - page_size == teb->DeallocationStack)
> > -
> >            teb->Tib.StackLimit = page;
> > -
> >        else if ((char*)addr>  (char*)teb->Tib.StackLimit + page_size&&
> > -
> >                teb->Tib.StackLimit == (char*)teb->DeallocationStack + page_size&&
> > -
> >                (view = VIRTUAL_FindView( (char*)teb->Tib.StackLimit, 0)))
> > -
> >        {
> > -
> >            vprot = view->prot[((char*)teb->Tib.StackLimit - (char*)view->base)>>  page_shift];
> > -
> >            VIRTUAL_SetProt( view, teb->Tib.StackLimit, page_size, vprot | VPROT_GUARD);
> > -
> >            teb->Tib.StackLimit = (char*)teb->Tib.StackLimit + page_size;
> > -
> >        }
>
> Why do you want to lookup the view again?

It's not needed (I thought it may be not valid for whole stack). I'll 
send fixed version.

There also should be "growing" instead of "shrinking" in commit message. 
It was meant to point that it's possible to change StackLimit more then 
once without this patch.

[PATCH 1/2] ntdll: Restore stack guard and prevent stack from shrinking

[Alexandre Julliard]

Piotr Caban  writes:
> It's not needed (I thought it may be not valid for whole stack). I'll
> send fixed version.
>
> There also should be "growing" instead of "shrinking" in commit
> message. It was meant to point that it's possible to change StackLimit
> more then once without this patch.

I'm not sure what you mean, making it grow is the reason for that
function.

[PATCH 1/2] ntdll: Restore stack guard and prevent stack from shrinking

[Piotr Caban]

On 04/07/11 11:00, Alexandre Julliard wrote:
> Piotr Caban  writes:
> > It's not needed (I thought it may be not valid for whole stack). I'll
> > send fixed version.
> >
> > There also should be "growing" instead of "shrinking" in commit
> > message. It was meant to point that it's possible to change StackLimit
> > more then once without this patch.
>
> I'm not sure what you mean, making it grow is the reason for that
> function.

Without this patch there's following condition for stack growing:
if ((char *)page + page_size == NtCurrentTeb()->Tib.StackLimit)
    NtCurrentTeb()->Tib.StackLimit = page;
If after growing the stack application protects the memory between 
StackLimit and StackLimit-page_size, StackLimit will be changed again.

[PATCH 1/2] ntdll: Restore stack guard and prevent stack from shrinking

[Alexandre Julliard]

Piotr Caban  writes:
> Without this patch there's following condition for stack growing:
> if ((char *)page + page_size == NtCurrentTeb()->Tib.StackLimit)
>    NtCurrentTeb()->Tib.StackLimit = page;
> If after growing the stack application protects the memory between
> StackLimit and StackLimit-page_size, StackLimit will be changed again.

StackLimit is supposed to be the last address that is unprotected, so
changing it in that case would be correct. The last page should really
to remain protected though, which application is modifying it?

[PATCH 1/2] ntdll: Restore stack guard and prevent stack from shrinking

[Piotr Caban]

On 04/07/11 11:22, Alexandre Julliard wrote:
> Piotr Caban  writes:
> > Without this patch there's following condition for stack growing:
> > if ((char *)page + page_size == NtCurrentTeb()->Tib.StackLimit)
> >    NtCurrentTeb()->Tib.StackLimit = page;
> > If after growing the stack application protects the memory between
> > StackLimit and StackLimit-page_size, StackLimit will be changed again.
>
> StackLimit is supposed to be the last address that is unprotected, so
> changing it in that case would be correct. The last page should really
> to remain protected though, which application is modifying it?

It happens when native _resetstkoflw is called when there's little 
memory left on the stack. I don't know if there's any real application 
that is calling it in that case. I thought that it's incorrect to grow 
the stack size above stack limit anyway.

[PATCH 1/2] ntdll: Restore stack guard and prevent stack from shrinking

[Alexandre Julliard]

Piotr Caban  writes:
> It happens when native _resetstkoflw is called when there's little
> memory left on the stack. I don't know if there's any real application
> that is calling it in that case. I thought that it's incorrect to grow
> the stack size above stack limit anyway.

No, the stack limit indicates the limit of the current growth. If the
stack grows the limit is updated.