[PATCH] use canary at end of stack to indicate overruns at oops time

[PATCH] use canary at end of stack to indicate overruns at oops time

[Eric Sandeen]

Use a canary at the end of the stack to clearly indicate
at oops time whether the stack has ever overflowed.

This is a very simple implementation with a couple of
drawbacks:

1) a thread may legitimately use exactly up to the last 
   word on the stack

 -- but the chances of doing this and then oopsing later seem slim

2) it's possible that the stack usage isn't dense enough
   that the canary location could get skipped over

 -- but the worst that happens is that we don't flag the overrun

With the code in place, an intentionally-bloated stack oops does:

BUG: unable to handle kernel paging request at ffff8103f84cc680
IP: [<ffffffff810253df>] update_curr+0x9a/0xa8
PGD 8063 PUD 0 
Thread overran stack or stack corrupted
Oops: 0000 [1] SMP 
CPU 0 
...


Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---


Index: linux-2.6.25/arch/x86/mm/fault.c
===================================================================
--- linux-2.6.25.orig/arch/x86/mm/fault.c	2008-04-20 22:30:36.000000000 -0500
+++ linux-2.6.25/arch/x86/mm/fault.c	2008-04-21 16:58:18.913885442 -0500
@@ -25,6 +25,7 @@
 #include <linux/kprobes.h>
 #include <linux/uaccess.h>
 #include <linux/kdebug.h>
+#include <linux/magic.h>
 
 #include <asm/system.h>
 #include <asm/desc.h>
@@ -581,6 +582,8 @@ void __kprobes do_page_fault(struct pt_r
 	unsigned long address;
 	int write, si_code;
 	int fault;
+	unsigned long *stackend;
+
 #ifdef CONFIG_X86_64
 	unsigned long flags;
 #endif
@@ -850,6 +853,10 @@ no_context:
 
 	show_fault_oops(regs, error_code, address);
 
+	stackend = end_of_stack(tsk);
+	if (*stackend != STACK_END_MAGIC)
+		printk(KERN_ALERT "Thread overran stack or stack corrupted\n");
+
 	tsk->thread.cr2 = address;
 	tsk->thread.trap_no = 14;
 	tsk->thread.error_code = error_code;
Index: linux-2.6.25/include/linux/magic.h
===================================================================
--- linux-2.6.25.orig/include/linux/magic.h	2008-01-24 16:58:37.000000000 -0600
+++ linux-2.6.25/include/linux/magic.h	2008-04-21 16:53:47.838406992 -0500
@@ -42,4 +42,5 @@
 #define FUTEXFS_SUPER_MAGIC	0xBAD1DEA
 #define INOTIFYFS_SUPER_MAGIC	0x2BAD1DEA
 
+#define STACK_END_MAGIC		0x57AC6E9D
 #endif /* __LINUX_MAGIC_H__ */
Index: linux-2.6.25/kernel/fork.c
===================================================================
--- linux-2.6.25.orig/kernel/fork.c	2008-04-21 16:49:49.000000000 -0500
+++ linux-2.6.25/kernel/fork.c	2008-04-21 16:54:22.039406916 -0500
@@ -53,6 +53,7 @@
 #include <linux/tty.h>
 #include <linux/proc_fs.h>
 #include <linux/blkdev.h>
+#include <linux/magic.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -167,6 +168,8 @@ static struct task_struct *dup_task_stru
 {
 	struct task_struct *tsk;
 	struct thread_info *ti;
+	unsigned long *stackend;
+
 	int err;
 
 	prepare_to_copy(orig);
@@ -192,6 +195,8 @@ static struct task_struct *dup_task_stru
 	}
 
 	setup_thread_stack(tsk, orig);
+	stackend = end_of_stack(tsk);
+	*stackend = STACK_END_MAGIC;	/* for overflow detection */
 
 #ifdef CONFIG_CC_STACKPROTECTOR
 	tsk->stack_canary = get_random_int();

Re: [PATCH] use canary at end of stack to indicate overruns at oops time

[Arjan van de Ven]

On Mon, 21 Apr 2008 22:44:39 -0500
Eric Sandeen <sandeen@redhat.com> wrote:

> Use a canary at the end of the stack to clearly indicate
> at oops time whether the stack has ever overflowed.
> 
> This is a very simple implementation with a couple of
> drawbacks:
> 
> 1) a thread may legitimately use exactly up to the last 
>    word on the stack
> 
>  -- but the chances of doing this and then oopsing later seem slim
> 
> 2) it's possible that the stack usage isn't dense enough
>    that the canary location could get skipped over
> 
>  -- but the worst that happens is that we don't flag the overrun
> 
> With the code in place, an intentionally-bloated stack oops does:
> 
> BUG: unable to handle kernel paging request at ffff8103f84cc680
> IP: [<ffffffff810253df>] update_curr+0x9a/0xa8
> PGD 8063 PUD 0 
> Thread overran stack or stack corrupted
> Oops: 0000 [1] SMP 
> CPU 0 

I like this but I wonder if it's useful to do this for the irq stacks too;

either way

Acked-by: Arjan van de Ven <arjan@linux.intel.com>

(I'll also add detection of your printk to the kerneloops.org website)

Re: [PATCH] use canary at end of stack to indicate overruns at oops time

[Ingo Molnar]

* Eric Sandeen <sandeen@redhat.com> wrote:

> Use a canary at the end of the stack to clearly indicate at oops time 
> whether the stack has ever overflowed.
> 
> This is a very simple implementation with a couple of drawbacks:
> 
> 1) a thread may legitimately use exactly up to the last
>    word on the stack
> 
>  -- but the chances of doing this and then oopsing later seem slim

even that narrow case is a bug - an NMI might arrive at exactly that 
point and overflow the stack for real.

> 2) it's possible that the stack usage isn't dense enough
>    that the canary location could get skipped over
> 
>  -- but the worst that happens is that we don't flag the overrun

yeah.

> With the code in place, an intentionally-bloated stack oops does:
> 
> BUG: unable to handle kernel paging request at ffff8103f84cc680
> IP: [<ffffffff810253df>] update_curr+0x9a/0xa8
> PGD 8063 PUD 0 
> Thread overran stack or stack corrupted
> Oops: 0000 [1] SMP 
> CPU 0 
> ...

excellent. I've queued this up, it's definitely an improvement in 
debuggability.

we used to have something comparable in ancient kernels (it was called 
the stack red zone IIRC) - but it was not printed in oopses and we lost 
the feature somewhere anyway.

	Ingo

Re: [PATCH] use canary at end of stack to indicate overruns at oops time

[Eric Sandeen]

Ingo Molnar wrote:
> * Eric Sandeen <sandeen@redhat.com> wrote:

>> With the code in place, an intentionally-bloated stack oops does:
>>
>> BUG: unable to handle kernel paging request at ffff8103f84cc680
>> IP: [<ffffffff810253df>] update_curr+0x9a/0xa8
>> PGD 8063 PUD 0 
>> Thread overran stack or stack corrupted
>> Oops: 0000 [1] SMP 
>> CPU 0 
>> ...
> 
> excellent. I've queued this up, it's definitely an improvement in 
> debuggability.

Crud, just realized this probably doesn't play well with
CONFIG_DEBUG_STACK_USAGE.  I think it will need something like:

Index: linux-2.6.25-rc7/kernel/exit.c
===================================================================
--- linux-2.6.25-rc7.orig/kernel/exit.c	2008-04-20 22:34:16.000000000 -0500
+++ linux-2.6.25-rc7/kernel/exit.c	2008-04-22 11:38:05.769412824 -0500
@@ -826,6 +826,8 @@ static void check_stack_usage(void)
 	unsigned long *n = end_of_stack(current);
 	unsigned long free;

+	n++;	/* skip over canary at end */
+
 	while (*n == 0)
 		n++;
 	free = (unsigned long)n - (unsigned long)end_of_stack(current);


Testing now... want me to resend the whole patch, Ingo, or you want to
just fix it up?  (I'll follow up with the testing results)

Thanks,
-Eric

[PATCH] Fix max-stack calculators to skip canary

[Eric Sandeen]

With the canary on the end of the stack, anything which
looks for 0 to mean unused when calculating max stack
excursions must skip over this non-zero magic number.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Index: linux-2.6.25/kernel/exit.c
===================================================================
--- linux-2.6.25.orig/kernel/exit.c	2008-04-20 22:34:16.000000000 -0500
+++ linux-2.6.25/kernel/exit.c	2008-04-22 11:38:05.769412824 -0500
@@ -826,6 +826,8 @@ static void check_stack_usage(void)
 	unsigned long *n = end_of_stack(current);
 	unsigned long free;
 
+	n++;	/* skip over canary at end */
+
 	while (*n == 0)
 		n++;
 	free = (unsigned long)n - (unsigned long)end_of_stack(current);
Index: linux-2.6.25/kernel/sched.c
===================================================================
--- linux-2.6.25.orig/kernel/sched.c	2008-04-20 22:34:19.000000000 -0500
+++ linux-2.6.25/kernel/sched.c	2008-04-22 11:48:06.975407495 -0500
@@ -5190,6 +5190,8 @@ void sched_show_task(struct task_struct 
 #ifdef CONFIG_DEBUG_STACK_USAGE
 	{
 		unsigned long *n = end_of_stack(p);
+
+		n++;	/* skip over canary at end */
 		while (!*n)
 			n++;
 		free = (unsigned long)n - (unsigned long)end_of_stack(p);

Re: [PATCH] Fix max-stack calculators to skip canary

[Joe Perches]

On Tue, 2008-04-22 at 12:18 -0500, Eric Sandeen wrote:
> Index: linux-2.6.25/kernel/exit.c
> ===================================================================
> --- linux-2.6.25.orig/kernel/exit.c	2008-04-20 22:34:16.000000000 -0500
> +++ linux-2.6.25/kernel/exit.c	2008-04-22 11:38:05.769412824 -0500
> @@ -826,6 +826,8 @@ static void check_stack_usage(void)
>  	unsigned long *n = end_of_stack(current);
>  	unsigned long free;
>  
> +	n++;	/* skip over canary at end */
> +
>  	while (*n == 0)
>  		n++;
>  	free = (unsigned long)n - (unsigned long)end_of_stack(current);
> Index: linux-2.6.25/kernel/sched.c
> ===================================================================
> --- linux-2.6.25.orig/kernel/sched.c	2008-04-20 22:34:19.000000000 -0500
> +++ linux-2.6.25/kernel/sched.c	2008-04-22 11:48:06.975407495 -0500
> @@ -5190,6 +5190,8 @@ void sched_show_task(struct task_struct 
>  #ifdef CONFIG_DEBUG_STACK_USAGE
>  	{
>  		unsigned long *n = end_of_stack(p);
> +
> +		n++;	/* skip over canary at end */
>  		while (!*n)
>  			n++;
>  		free = (unsigned long)n - (unsigned long)end_of_stack(p);
> 

Wouldn't it be better to have exactly the same code?
How about using a statement expression macro?

#define DEBUG_STACK_FREE(process) \
({ \
	unsigned long *n = end_of_stack(process); \
	do { 	/* Skip over canary */\
		n++; \
	} while (!*n); \
	(unsigned long)n - (unsigned long)end_of_stack(process); \
})

Re: [PATCH] Fix max-stack calculators to skip canary

[Eric Sandeen]

Joe Perches wrote:

> Wouldn't it be better to have exactly the same code?
> How about using a statement expression macro?
> 
> #define DEBUG_STACK_FREE(process) \
> ({ \
> 	unsigned long *n = end_of_stack(process); \
> 	do { 	/* Skip over canary */\
> 		n++; \
> 	} while (!*n); \
> 	(unsigned long)n - (unsigned long)end_of_stack(process); \
> })
> 

That'd probably be good although I don't think we like macros anymore :)

-Eric

[PATCH V2] use canary at end of stack to indicate overruns at oops time

[Eric Sandeen]

(Updated with a common max-stack-used checker that knows about
the canary, as suggested by Joe Perches)

Use a canary at the end of the stack to clearly indicate
at oops time whether the stack has ever overflowed.

This is a very simple implementation with a couple of
drawbacks:

1) a thread may legitimately use exactly up to the last 
   word on the stack

 -- but the chances of doing this and then oopsing later seem slim

2) it's possible that the stack usage isn't dense enough
   that the canary location could get skipped over

 -- but the worst that happens is that we don't flag the overrun
 -- though this happens fairly often in my testing :(

With the code in place, an intentionally-bloated stack oops might
do:

BUG: unable to handle kernel paging request at ffff8103f84cc680
IP: [<ffffffff810253df>] update_curr+0x9a/0xa8
PGD 8063 PUD 0 
Thread overran stack or stack corrupted
Oops: 0000 [1] SMP 
CPU 0 
...

... unless the stack overrun is so bad that it corrupts some other
thread.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Index: linux-2.6.25/arch/x86/mm/fault.c
===================================================================
--- linux-2.6.25.orig/arch/x86/mm/fault.c
+++ linux-2.6.25/arch/x86/mm/fault.c
@@ -25,6 +25,7 @@
 #include <linux/kprobes.h>
 #include <linux/uaccess.h>
 #include <linux/kdebug.h>
+#include <linux/magic.h>
 
 #include <asm/system.h>
 #include <asm/desc.h>
@@ -581,6 +582,8 @@ void __kprobes do_page_fault(struct pt_r
 	unsigned long address;
 	int write, si_code;
 	int fault;
+	unsigned long *stackend;
+
 #ifdef CONFIG_X86_64
 	unsigned long flags;
 #endif
@@ -850,6 +853,10 @@ no_context:
 
 	show_fault_oops(regs, error_code, address);
 
+ 	stackend = end_of_stack(tsk);
+	if (*stackend != STACK_END_MAGIC)
+		printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
+
 	tsk->thread.cr2 = address;
 	tsk->thread.trap_no = 14;
 	tsk->thread.error_code = error_code;
Index: linux-2.6.25/include/linux/magic.h
===================================================================
--- linux-2.6.25.orig/include/linux/magic.h
+++ linux-2.6.25/include/linux/magic.h
@@ -42,4 +42,5 @@
 #define FUTEXFS_SUPER_MAGIC	0xBAD1DEA
 #define INOTIFYFS_SUPER_MAGIC	0x2BAD1DEA
 
+#define STACK_END_MAGIC		0x57AC6E9D
 #endif /* __LINUX_MAGIC_H__ */
Index: linux-2.6.25/kernel/fork.c
===================================================================
--- linux-2.6.25.orig/kernel/fork.c
+++ linux-2.6.25/kernel/fork.c
@@ -53,6 +53,7 @@
 #include <linux/tty.h>
 #include <linux/proc_fs.h>
 #include <linux/blkdev.h>
+#include <linux/magic.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -167,6 +168,8 @@ static struct task_struct *dup_task_stru
 {
 	struct task_struct *tsk;
 	struct thread_info *ti;
+	unsigned long *stackend;
+
 	int err;
 
 	prepare_to_copy(orig);
@@ -192,6 +195,8 @@ static struct task_struct *dup_task_stru
 	}
 
 	setup_thread_stack(tsk, orig);
+	stackend = end_of_stack(tsk);
+	*stackend = STACK_END_MAGIC;	/* for overflow detection */
 
 #ifdef CONFIG_CC_STACKPROTECTOR
 	tsk->stack_canary = get_random_int();
Index: linux-2.6.25/kernel/exit.c
===================================================================
--- linux-2.6.25.orig/kernel/exit.c
+++ linux-2.6.25/kernel/exit.c
@@ -823,12 +823,9 @@ static void check_stack_usage(void)
 {
 	static DEFINE_SPINLOCK(low_water_lock);
 	static int lowest_to_date = THREAD_SIZE;
-	unsigned long *n = end_of_stack(current);
 	unsigned long free;
 
-	while (*n == 0)
-		n++;
-	free = (unsigned long)n - (unsigned long)end_of_stack(current);
+	free = stack_not_used(current);
 
 	if (free >= lowest_to_date)
 		return;
Index: linux-2.6.25/kernel/sched.c
===================================================================
--- linux-2.6.25.orig/kernel/sched.c
+++ linux-2.6.25/kernel/sched.c
@@ -5188,12 +5188,7 @@ void sched_show_task(struct task_struct 
 		printk(KERN_CONT " %016lx ", thread_saved_pc(p));
 #endif
 #ifdef CONFIG_DEBUG_STACK_USAGE
-	{
-		unsigned long *n = end_of_stack(p);
-		while (!*n)
-			n++;
-		free = (unsigned long)n - (unsigned long)end_of_stack(p);
-	}
+	free = stack_not_used(p);
 #endif
 	printk(KERN_CONT "%5lu %5d %6d\n", free,
 		task_pid_nr(p), task_pid_nr(p->real_parent));
Index: linux-2.6.25/include/linux/sched.h
===================================================================
--- linux-2.6.25.orig/include/linux/sched.h
+++ linux-2.6.25/include/linux/sched.h
@@ -1893,6 +1893,19 @@ static inline unsigned long *end_of_stac
 
 #endif
 
+#ifdef CONFIG_DEBUG_STACK_USAGE
+static inline unsigned long stack_not_used(struct task_struct *p)
+{
+	unsigned long *n = end_of_stack(p);
+
+	do { 	/* Skip over canary */
+		n++;
+	} while (!*n);
+
+	return (unsigned long)n - (unsigned long)end_of_stack(p);
+}
+#endif
+
 /* set thread flags in other task's structures
  * - see asm/thread_info.h for TIF_xxxx flags available
  */

Re: [PATCH V2] use canary at end of stack to indicate overruns at oops time

[Harvey Harrison]

On Tue, 2008-04-22 at 16:38 -0500, Eric Sandeen wrote:

> ... unless the stack overrun is so bad that it corrupts some other
> thread.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> Index: linux-2.6.25/arch/x86/mm/fault.c
> ===================================================================
> --- linux-2.6.25.orig/arch/x86/mm/fault.c
> +++ linux-2.6.25/arch/x86/mm/fault.c
> @@ -25,6 +25,7 @@
>  #include <linux/kprobes.h>
>  #include <linux/uaccess.h>
>  #include <linux/kdebug.h>
> +#include <linux/magic.h>
>  
>  #include <asm/system.h>
>  #include <asm/desc.h>
> @@ -581,6 +582,8 @@ void __kprobes do_page_fault(struct pt_r
>  	unsigned long address;
>  	int write, si_code;
>  	int fault;
> +	unsigned long *stackend;
> +
>  #ifdef CONFIG_X86_64
>  	unsigned long flags;
>  #endif
> @@ -850,6 +853,10 @@ no_context:
>  
>  	show_fault_oops(regs, error_code, address);
>  
> + 	stackend = end_of_stack(tsk);
> +	if (*stackend != STACK_END_MAGIC)
> +		printk(KERN_ALERT "Thread overran stack, or stack corrupted\n");
> +
>  	tsk->thread.cr2 = address;
>  	tsk->thread.trap_no = 14;
>  	tsk->thread.error_code = error_code;
> Index: linux-2.6.25/include/linux/magic.h
> ===================================================================
> --- linux-2.6.25.orig/include/linux/magic.h
> +++ linux-2.6.25/include/linux/magic.h
> @@ -42,4 +42,5 @@
>  #define FUTEXFS_SUPER_MAGIC	0xBAD1DEA
>  #define INOTIFYFS_SUPER_MAGIC	0x2BAD1DEA
>  
> +#define STACK_END_MAGIC		0x57AC6E9D
>  #endif /* __LINUX_MAGIC_H__ */
> Index: linux-2.6.25/kernel/fork.c
> ===================================================================
> --- linux-2.6.25.orig/kernel/fork.c
> +++ linux-2.6.25/kernel/fork.c
> @@ -53,6 +53,7 @@
>  #include <linux/tty.h>
>  #include <linux/proc_fs.h>
>  #include <linux/blkdev.h>
> +#include <linux/magic.h>
>  
>  #include <asm/pgtable.h>
>  #include <asm/pgalloc.h>
> @@ -167,6 +168,8 @@ static struct task_struct *dup_task_stru
>  {
>  	struct task_struct *tsk;
>  	struct thread_info *ti;
> +	unsigned long *stackend;
> +
>  	int err;
>  
>  	prepare_to_copy(orig);
> @@ -192,6 +195,8 @@ static struct task_struct *dup_task_stru
>  	}
>  
>  	setup_thread_stack(tsk, orig);
> +	stackend = end_of_stack(tsk);
> +	*stackend = STACK_END_MAGIC;	/* for overflow detection */
>  
>  #ifdef CONFIG_CC_STACKPROTECTOR
>  	tsk->stack_canary = get_random_int();
> Index: linux-2.6.25/kernel/exit.c
> ===================================================================
> --- linux-2.6.25.orig/kernel/exit.c
> +++ linux-2.6.25/kernel/exit.c
> @@ -823,12 +823,9 @@ static void check_stack_usage(void)
>  {
>  	static DEFINE_SPINLOCK(low_water_lock);
>  	static int lowest_to_date = THREAD_SIZE;
> -	unsigned long *n = end_of_stack(current);
>  	unsigned long free;
>  
> -	while (*n == 0)
> -		n++;
> -	free = (unsigned long)n - (unsigned long)end_of_stack(current);
> +	free = stack_not_used(current);
>  
>  	if (free >= lowest_to_date)
>  		return;
> Index: linux-2.6.25/kernel/sched.c
> ===================================================================
> --- linux-2.6.25.orig/kernel/sched.c
> +++ linux-2.6.25/kernel/sched.c
> @@ -5188,12 +5188,7 @@ void sched_show_task(struct task_struct 
>  		printk(KERN_CONT " %016lx ", thread_saved_pc(p));
>  #endif
>  #ifdef CONFIG_DEBUG_STACK_USAGE
> -	{
> -		unsigned long *n = end_of_stack(p);
> -		while (!*n)
> -			n++;
> -		free = (unsigned long)n - (unsigned long)end_of_stack(p);
> -	}
> +	free = stack_not_used(p);
>  #endif

Maybe remove the #ifdef CONFIG_DEBUG_STACK_USAGE block and move it into
stack_not_used...call it debug_stack_not_used.

>  	printk(KERN_CONT "%5lu %5d %6d\n", free,
>  		task_pid_nr(p), task_pid_nr(p->real_parent));
> Index: linux-2.6.25/include/linux/sched.h
> ===================================================================
> --- linux-2.6.25.orig/include/linux/sched.h
> +++ linux-2.6.25/include/linux/sched.h
> @@ -1893,6 +1893,19 @@ static inline unsigned long *end_of_stac
>  
>  #endif
>  
> +#ifdef CONFIG_DEBUG_STACK_USAGE
> +static inline unsigned long stack_not_used(struct task_struct *p)
> +{
> +	unsigned long *n = end_of_stack(p);
> +
> +	do { 	/* Skip over canary */
> +		n++;
> +	} while (!*n);
> +
> +	return (unsigned long)n - (unsigned long)end_of_stack(p);
> +}
> +#endif
> +

static inline unsigned long debug_stack_not_used(struct task_struct *p)
{
#ifdef CONFIG_DEBUG_STACK_USAGE
	unsigned long *n = end_of_stack(p);

	do { 	/* Skip over canary */
		n++;
	} while (!*n);

	return (unsigned long)n - (unsigned long)end_of_stack(p);
#else
	return $(large_value)....maybe or just some known value.
#endif
}

Also, do you expect this to ever be used outside of sched.c?  Maybe just
leave it as a static function there rather than an inline in the header.

Harvey

Re: [PATCH V2] use canary at end of stack to indicate overruns at oops time

[Eric Sandeen]

Harvey Harrison wrote:

> Also, do you expect this to ever be used outside of sched.c?  Maybe just
> leave it as a static function there rather than an inline in the header.

the patch uses it in exit.c....

-Eric

Re: [PATCH] Fix max-stack calculators to skip canary

[Ingo Molnar]

* Eric Sandeen <sandeen@sandeen.net> wrote:

> With the canary on the end of the stack, anything which looks for 0 to 
> mean unused when calculating max stack excursions must skip over this 
> non-zero magic number.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

thanks Eric, applied.

	Ingo

Re: [PATCH V2] use canary at end of stack to indicate overruns at oops time

[Ingo Molnar]

* Eric Sandeen <sandeen@sandeen.net> wrote:

> (Updated with a common max-stack-used checker that knows about the 
> canary, as suggested by Joe Perches)

thanks - picked up this updated version.

	Ingo