Re: [ros-dev] [ros-diffs] [tkreuzer] 40963: MmGrowKernelStack: Don't assert, but fail, when the kernel stack can't grow any more. Fixes a crash with recursive user calls. See issue #4060 for more details.

[Alex Ionescu]

The code SHOULD assert.
This is a hack.

Best regards,
Alex Ionescu


On Sun, May 17, 2009 at 11:51 PM, <tkreuzer(a)svn.reactos.org> wrote:

>   Author: tkreuzer
>  Date: Mon May 18 01:51:31 2009
>  New Revision: 40963
>
>  URL: http://svn.reactos.org/svn/reactos?rev=40963&view=rev
>  Log:
>  MmGrowKernelStack: Don't assert, but fail, when the kernel stack can't grow
>  any more. Fixes a crash with recursive user calls.
>  See issue #4060 for more details.
>
>  Modified:
>     trunk/reactos/ntoskrnl/mm/procsup.c
>
>  Modified: trunk/reactos/ntoskrnl/mm/procsup.c
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/procsup.c?rev=…
>
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/mm/procsup.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/mm/procsup.c [iso-8859-1] Mon May 18 01:51:31
>  2009
>  @@ -259,8 +259,11 @@
>      PETHREAD Thread = PsGetCurrentThread();
>
>      /* Make sure we have reserved space for our grow */
>  -    ASSERT(((PCHAR)Thread->Tcb.StackBase - (PCHAR)Thread->Tcb.StackLimit)
>  <=
>  -           (KERNEL_LARGE_STACK_SIZE + PAGE_SIZE));
>  +    if (((PCHAR)Thread->Tcb.StackBase - (PCHAR)Thread->Tcb.StackLimit) >
>  +           (KERNEL_LARGE_STACK_SIZE + PAGE_SIZE))
>  +    {
>  +        return STATUS_NO_MEMORY;
>  +    }
>
>      /*
>       * We'll give you three more pages.
>
>

[Timo Kreuzer]

Ok, I'm sure you know what you are talking about.

I was misled by the comment (/* Make sure we have reserved space for our
grow */) which should probably be /* Make sure the stack didn't overflow */

The behavious on Windows is described here:
http://news.jrsoftware.org/news/toolbar2000/msg07779.html

So on windows KiCallUserModecalls calls MmGrowStack and when that fails
with STATUS_STACK_OVERFLOW (yes I used a wrong status), KiCallUserMode
fails.
It does not state though what the exact check is or whether
KiCallUserMode would also check the Size before, but the latter seemes
to be redundand to me. So I there seems to be a different check.

Would you agree with this?
---
    /* Make sure the stack did not overflow */
    ASSERT(((PCHAR)Thread->Tcb.StackBase - (PCHAR)Thread->Tcb.StackLimit) <=
           (KERNEL_LARGE_STACK_SIZE + PAGE_SIZE));

    /* Check if we have reserved space for our grow */
    if (Thread->Tcb.StackBase - Thread->Tcb.StackLimit +
KERNEL_STACK_SIZE >
        KERNEL_LARGE_STACK_SIZE)
    {
        return STATUS_STACK_OVERFLOW;
    }
---

Regards,
Timo

Alex Ionescu schrieb:
>   The code SHOULD assert.
>  This is a hack.
>
>  Best regards,
>  Alex Ionescu
>
>
>  On Sun, May 17, 2009 at 11:51 PM, &lt;tkreuzer(a)svn.reactos.org&gt; wrote:
>
>    
> >   Author: tkreuzer
> >  Date: Mon May 18 01:51:31 2009
> >  New Revision: 40963
> >
> >  URL: http://svn.reactos.org/svn/reactos?rev=40963&view=rev
> >  Log:
> >  MmGrowKernelStack: Don't assert, but fail, when the kernel stack can't grow
> >  any more. Fixes a crash with recursive user calls.
> >  See issue #4060 for more details.
> >
> >  Modified:
> >     trunk/reactos/ntoskrnl/mm/procsup.c
> >
> >  Modified: trunk/reactos/ntoskrnl/mm/procsup.c
> >  URL:
> >
> > http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/procsup.c?rev=…
> >
> >  ==============================================================================
> >  --- trunk/reactos/ntoskrnl/mm/procsup.c [iso-8859-1] (original)
> >  +++ trunk/reactos/ntoskrnl/mm/procsup.c [iso-8859-1] Mon May 18 01:51:31
> >  2009
> >  @@ -259,8 +259,11 @@
> >      PETHREAD Thread = PsGetCurrentThread();
> >
> >      /* Make sure we have reserved space for our grow */
> >  -    ASSERT(((PCHAR)Thread->Tcb.StackBase - (PCHAR)Thread->Tcb.StackLimit)
> >  <=
> >  -           (KERNEL_LARGE_STACK_SIZE + PAGE_SIZE));
> >  +    if (((PCHAR)Thread->Tcb.StackBase - (PCHAR)Thread->Tcb.StackLimit) >
> >  +           (KERNEL_LARGE_STACK_SIZE + PAGE_SIZE))
> >  +    {
> >  +        return STATUS_NO_MEMORY;
> >  +    }
> >
> >      /*
> >       * We'll give you three more pages.
> >
> >
> >       
>    
>  ------------------------------------------------------------------------
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Aleksey Bragin]

Exactly what I was telling yesterday in IRC. This assert is caused by  
recursive user call, not vice versa!!

Revert?


WBR,
Aleksey Bragin.

On May 18, 2009, at 10:29 AM, Alex Ionescu wrote:

>   The code SHOULD assert.
>
>  This is a hack.
>
>  Best regards,
>  Alex Ionescu
>
>
>  On Sun, May 17, 2009 at 11:51 PM, <tkreuzer(a)svn.reactos.org> wrote:
>  Author: tkreuzer
>  Date: Mon May 18 01:51:31 2009
>  New Revision: 40963
>
>  URL: http://svn.reactos.org/svn/reactos?rev=40963&view=rev
>  Log:
>  MmGrowKernelStack: Don't assert, but fail, when the kernel stack  
>  can't grow any more. Fixes a crash with recursive user calls.
>  See issue #4060 for more details.
>
>  Modified:
>     trunk/reactos/ntoskrnl/mm/procsup.c
>
>  Modified: trunk/reactos/ntoskrnl/mm/procsup.c
>  URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ 
>  procsup.c?rev=40963&r1=40962&r2=40963&view=diff
>  ====================================================================== 
>  ========
>  --- trunk/reactos/ntoskrnl/mm/procsup.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/mm/procsup.c [iso-8859-1] Mon May 18  
>  01:51:31 2009
>  @@ -259,8 +259,11 @@
>      PETHREAD Thread = PsGetCurrentThread();
>
>      /* Make sure we have reserved space for our grow */
>  -    ASSERT(((PCHAR)Thread->Tcb.StackBase - (PCHAR)Thread- 
> >  Tcb.StackLimit) <= 
>  -          
> (KERNEL_LARGE_STACK_SIZE + PAGE_SIZE));
>  +    if (((PCHAR)Thread->Tcb.StackBase - (PCHAR)Thread- 
> >  Tcb.StackLimit) > 
>  +          
> (KERNEL_LARGE_STACK_SIZE + PAGE_SIZE))
>  +    {
>  +        return STATUS_NO_MEMORY;
>  +    }
>
>      /*
>       * We'll give you three more pages.
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Timo Kreuzer]

Aleksey Bragin wrote:
>   Exactly what I was telling yesterday in IRC.  
I
can only remember that you asked, if it would really fix something. ;)
And yes it does. Just not the fully correct way.

>   This assert is caused by recursive user call, not vice
> versa!! 
True. But the recursive usercall is valid. Please take a look at the
article I linked. It should simply end at some point when no more kernel
stack is available.

>  
>  Revert? 
What about the updated code I send?

[Aleksey Bragin]

On May 18, 2009, at 6:43 PM, Timo Kreuzer wrote:

>  
>  Aleksey Bragin wrote:
> >   Exactly what I was telling yesterday in IRC.
>  I can only remember that you asked, if it would really fix  
>  something. ;)
>  And yes it does. Just not the fully correct way. 
Ah yes, my bad!

> >   This assert is
> > caused by recursive user call, not vice versa!! 
>  True. But the recursive usercall is
> valid. Please take a look at the
>  article I linked. It should simply end at some point when no more  
>  kernel
>  stack is available. 
Yes, I mixed those two different bugs earlier too, hence the  
confusion, sorry.

> >  
> >  Revert? 
>  What about the updated code I send? 
For me it looks correct, maybe
Alex has comments.


WBR,
Aleksey Bragin.

[Benny Gaechter]

Hi there,

i started today testing and reviewing the code of reactos because i want 
to contribute (and i am really fascinated).
Now i looked at the hostname function in 
ReactOS/base/applications/cmdutils/hostname/.

If argc is more than 1 (so if there is given a parameter) the output is

/Print the current host's name.\n\nhostname\n

/But maybe it could be changed in:
/
Usage: hostname
use this application without any parameter./

}else{
        if (0 == strcmp(argv[1],"-s"))
        {
            fprintf(stderr,"%s: -s not supported.\n",argv[0]);
            return EXIT_FAILURE;
        }else{
            printf("\nUsage: hostname\n\nuse this application without 
any parameter\n"); //here i had to change.
        }
    }

Or would this be a bad idea?

Kind regards

hostname behaves like the following on XP (polish localized copy, 
messages may differ):

if(no parameter line given) {
	print hostname()
} else {
	if(parameter line begins with "-") {
		if(parameter == "-s")
			print hostname() . "\n"
		else
			print "Invalid option. \n\n Prints current host name. \n\n hostname \n"
	}
	else
		print "sethostname: Use Control panel applet to set hostname. \n 
hostname -s is not supported. \n"
}

~d0g



Benny Gaechter pisze:
>   If argc is more than 1 (so if there is given a
> parameter) the output is
>  
>  /Print the current host's name.\n\nhostname\n
>  
>  /But maybe it could be changed in:
>  /
>  Usage: hostname
>  use this application without any parameter./
>  
>  }else{
>          if (0 == strcmp(argv[1],"-s"))
>          {
>              fprintf(stderr,"%s: -s not supported.\n",argv[0]);
>              return EXIT_FAILURE;
>          }else{
>              printf("\nUsage: hostname\n\nuse this application without 
>  any parameter\n"); //here i had to change.
>          }
>      }
>