issue-#25 Fixing a bug in stack overflow handling

[Josef Templ]

The next issue refers to number 8 in CPC1.7 rc5.
It is about a bug in the handling of stack overflows reported by OberonCore.
The first stack overflow is handled well, but subsequent stack overflows crash the BB process.

Please see http://redmine.blackboxframework.org/issues/25.
A proposal to fix the bug exists from luowy.
I have put it into the branch issue-#25.

@luowy: can you please comment on the bug fix. What is going on here in detail.
Why is the page guard required at all? What effect has it to increase the size?
Did you find this solution by try-and-error or by fully understanding the
working of the trap handler?

For all:
Here is the test program:

Code: Select all

MODULE TestCrash;
    IMPORT Log := StdLog;

    CONST n = 8000;

    PROCEDURE Do* ;
        VAR a: ARRAY n, n OF REAL;
    BEGIN
        Log.Int(LEN(a, 0)); Log.Ln;
    END Do;

END TestCrash.

TestCrash.Do

First click the Do command, BB will give a trap dialog,
close the trap dialog, and click the command again,
then BB will crash.

- Josef

[luowy]

Did you find this solution by try-and-error or by fully understanding the
working of the trap handler?

by try-and-error, not fully understanding why it need two page guard.
only know that the compiler allocate the stack page by page, the one page guard cant
catch the trap,need two page to guard it.


luowy

[Josef Templ]

Thanks luowy. Do you know why we need a page guard at all?
How is the very first stack overflow detected?
Is this also by some page guard? But where is it set up?

- Josef

[luowy]

Do you know why we need a page guard at all?
How is the very first stack overflow detected?
Is this also by some page guard? But where is it set up?

the CreateThread funtion setup the first page guard,
system use page guard to detected the memory boundary:clear the accessing page guard and set guard flag to next page.
when next page is the last page of the reserved memory,OS will not set guard flag on it,but raise a stackoverflow exception:
then the stack will have no guard page any more if you not reset a guard page by hand!

so we need reset a page guard on the stackoverflow exception for catch next stackoverflow exception.


the origin bb reset stack only guard one page,seem not enough!
see vc: crt\src\resetstk.c

Code: Select all

    if (RegionSize < MIN_STACK_REQ_WINNT * PageSize) {
        RegionSize = MIN_STACK_REQ_WINNT * PageSize;
    }

they gurad two page at least .

I have tried two guard:
1,

Code: Select all

res := WinApi.VirtualProtect(FPageWord(8), 1024+4096, {2, 8}, old);

2,

Code: Select all

res := WinApi.VirtualProtect(FPageWord(8)+4096, 1024, {2, 8}, old);

they all work well, actully, the second page of last is the point.
so it seem guard two pagesize is a good choice.

luowy

[Josef Templ]

Thanks luowy!

Since the trap handling is a non-trivial piece of code I have tried to
refactor it in order to make it more explicit what is going on at least as far as I understand it now.

Stack protection happens at three locations in TrapHandler. So I have factored this out into a separate
procedure ProtectStack with appropriate comments and a link into wikipedia
where the Windows NT thread information block is documented.
I have also used named constants from WinApi for the protection flags.
Since we need to protect two pages, I have used the system's page size * 2 for the
size of the protected block at the ceiling of the stack. This makes it explicit
what the size number really means and which size is used effectively.

The solution still works in exactly the same way as luowy's proposal,
but I think it makes the code more readable and maintainable in this form.

Please have a look at the diff in
http://redmine.blackboxframework.org/pr ... Kernel.odc
and post your comments.

- Josef

[luowy]

Yes,It is more mantainable than before,

I prefer:
1,ProtectStack --> ResetStack,or ResetStackGuard or ResetStackPageGuard,

2,just do a comment at

Code: Select all

  stackLimit := FPageWord(8);(* StackLimitOffset=8, http://en.wikipedia.org/wiki/Win32_Thread_Information_Block *)

more short and more readable than define the const.

3, size --> regionSize

luowy

[Josef Templ]

OK,

ProtectStack renamed to ResetStackGuard.
size renamed to regionSize.

> stackLimit := FPageWord(8);(* StackLimitOffset=8, http://en.wikipedia.org/wiki/Win32_Thre ... tion_Block *)

stackLimitOffset unchanged because:
1. the proposed variant does not fit on one line
2. the proposed variant mentions the constant '8' twice

see:
http://redmine.blackboxframework.org/pr ... Kernel.odc

- Josef

[Josef Templ]

I just found out that the thread informatin block (TIB) is defined in WinApi.
So there is no need for the reference to the wikipedia article any more and we can
write it in a single line very much as proposed by luowy.

Code: Select all

stackLimit := FPageWord(8); (* cf. WinApi.NT_TIB.StackLimit *)

From my point of view the issue is resolved and unless there are any objections, I will
create a voting tomorrow.

- Josef