[Arm64] System.Threading.Thread.Tests stack overflow

[sdmaclea]

xUnit.net console test runner (64-bit .NET Core)
Copyright (C) 2014 Outercurve Foundation.

Discovering: System.Threading.Thread.Tests
Discovered:  System.Threading.Thread.Tests
Starting:    System.Threading.Thread.Tests
   System.Threading.Threads.Tests.ThreadTests.ApartmentState_NoAttributePresent_DefaultState_Nano [SKIP]
      Condition(s) not met: \"IsWindowsNanoServer\"
Process is terminating due to StackOverflowException.
Command terminated by signal 6

[sdmaclea]

The stack overflow occurs during the
System.Threading.Threads.Tests.ThreadTests.ConstructorTest test case.

All other test cases pass.

[sdmaclea]

On my linux-arm64 and my linux-x64 Ubuntu 16.04 boxes.

ulimit -a returns stack size (kbytes, -s) 8192

This test tries to allocate 16MB on the stack. Legitimately causing a stack overflow.

This looks like a test bug to me. It is also not clear why this is not failing on linux-x64

[sdmaclea]

@kouvel I assert this is a corefx test bug

[kouvel]

I suspect pthread_create is not failing when 16 MB of stack size is requested and it only allocates 8 MB. On linux-x64 it seems to actually allocate the requested stack size despite the configured value, and this:

https://linux.die.net/man/3/pthread_create

Under the NPTL threading implementation, if the RLIMIT_STACK soft resource limit at the time the program started has any value other than "unlimited", then it determines the default stack size of new threads. Using pthread_attr_setstacksize(3), the stack size attribute can be explicitly set in the attr argument used to create a thread, in order to obtain a stack size other than the default.

Seems to suggest that the configured value is intended to be the default stack size and not the max, I could be missing something.

@sdmaclea could you please run the following on that machine and let me know the output?

#include <unistd.h>
#include <errno.h>

#include <pthread.h>

#include <iostream>
using namespace std;

void PrintError(int error)
{
    const char *errorName = "?";
    switch(error)
    {
        case ENOMEM:
            errorName = "ENOMEM";
            break;
        case EINVAL:
            errorName = "EINVAL";
            break;
        case EAGAIN:
            errorName = "EAGAIN";
            break;
        case EPERM:
            errorName = "EPERM";
            break;
        default:
            break;
    }
    cout << errorName << " (" << error << ')';
}

bool CheckAndPrintErrorOnNonzero(const char *op, int error)
{
    if(error == 0)
        return true;
    cout << op << " failed: ";
    PrintError(error);
    cout << '\n';
    return false;
}

void *ThreadStart(void *)
{
    return nullptr;
}

int main()
{
    pthread_attr_t attr;
    int error = pthread_attr_init(&attr);
    if(!CheckAndPrintErrorOnNonzero("attr_init", error))
        return 0;

    error = pthread_attr_setstacksize(&attr, (size_t)16 << 20);
    if(!CheckAndPrintErrorOnNonzero("attr_setstacksize", error))
        return 0;

    pthread_t thread;
    error = pthread_create(&thread, &attr, &ThreadStart, nullptr);
    if(!CheckAndPrintErrorOnNonzero("create", error))
        return 0;

    error = pthread_attr_destroy(&attr);
    if(!CheckAndPrintErrorOnNonzero("attr_destroy", error))
        return 0;

    error = pthread_getattr_np(thread, &attr);
    if(!CheckAndPrintErrorOnNonzero("getattr", error))
        return 0;

    size_t stackSize;
    error = pthread_attr_getstacksize(&attr, &stackSize);
    if(!CheckAndPrintErrorOnNonzero("attr_getstacksize", error))
        return 0;

    cout << "Actual stack size: " << stackSize;

    error = pthread_attr_destroy(&attr);
    if(!CheckAndPrintErrorOnNonzero("attr_destroy", error))
        return 0;

    return 0;
}

On linux-x64 I'm seeing that there's no error even for (size_t)6 << 30 (6 GB) and it's reserving the requested size, and I'm seeing a pthread_create error (EAGAIN) when I request 7 GB. Wondering if there's a difference in behavior somewhere here on linux-arm64.

[sdmaclea]

I'll try your test.

Also note, while debuigging this I changed the request to 7MB from 16MB and the test passed.

[sdmaclea]

Your program as written reports
Actual stack size: 16777216 by default on linux-arm64
Even with a hard limit ulimit -Hs 8192 reports the same.

[sdmaclea]

void *ThreadStart(void *) {
  char stackalloc[(16 << 20) - (64 << 10)];

  memset(stackalloc, 0, sizeof(stackalloc));
  return 0;
}

If I do an allocation in the stack, like is done in this test case, I see a SEGV in the debugger. (At least with the hard limit)

[kouvel]

Also note, while debuigging this I changed the request to 7MB from 16MB and the test passed.

I guess it only commits the necessary pages, I should have figured. I'll fix.

If I do an allocation in the stack, like is done in this test case, I see a SEGV in the debugger.

Interesting. I'm not sure what we can do about that from the product side. Any ideas? I'll go ahead and fix the test to use at most 2 MB for 32-bit platforms, that should be safe.

[kouvel]

I guess it only commits the necessary pages, I should have figured. I'll fix.

Actually that still seems odd, I guess it would skip the guard page and happens to work. It's not typical stack usage anyway, I'll make sure to go through the guard page.

[sdmaclea]

This is a 16GB 64 bit platform

[kouvel]

I just mean 2 GB should be a safe test for 32-bit and 64-bit

[sdmaclea]

It might just be that because this is a 64K page platform arm64 platform, the 16M-64K does not leave enough space for stack address randomization

In gdb, I can see the stack from

print $sp
$2 = (void *) 0xffffb7b9e9d0

(gdb) print *(char*)0xffffb6c8e9d0
$19 = 0 '\000'
(gdb) print *(char*)0xffffb6b8e9d0
Cannot access memory at address 0xffffb6b8e9d0

[janvorli]

Linux maps only a small portion of the stack space in the beginning and adds mapping more as necessary. But it keeps the virtual range reserved based on the ulimit for the primary thread and based on pthread internal limit or the limit asked for during pthread creation. So going over that limit should result in sigsegv.
There are some more subtle details though. This mechanism expects that the stack grows continuously as SP moves. But if you try to touch address that's smaller the currently mapped stack range lowest address and the SP is larger than the address you touch, at least on amd64, the behavior depends on the kernel version. On older kernels, it still extends the stack. On newer kernels, it rather passes sigsegv to the process, as touching stack below SP is considered illegal (to be precise, there is a 64kB safeguard zone where it still works on amd64, but that seems to be amd64 specific).

[kouvel]

I guess something would need to ensure that stack is allocated in continuous fashion. For a local array it is similar to direct addressing so it could potentially skip the guard page and read/write other threads' stacks if stack address randomization is not enabled (or by chance). The compiler may do something for functions with larger stacks (though it doesn't seem to be happening here). alloca may do something as well. In any case I guess it is inherently unsafe and similar would apply to C# stackalloc which has to be in unsafe block anyway.

[janvorli]

The JIT inserts stack probes for all functions with frame larger than memory page. However, about a week ago or so, the stack probing was reverted back to a way where the SP doesn't move as the probe continues (dotnet/coreclr#17112). It was causing issues on Windows. But it needs to go back for Unix.

[kouvel]

I see, makes sense

[kouvel]

The main issue here seems to be that pthread_create allocates less stack space than requested without failing. Filed issue https://sourceware.org/bugzilla/show_bug.cgi?id=23018, not sure if that's the right place but let's see. @sdmaclea it would be great if you could provide any info requested there (probably I guess they will ask for glibc version).

[kouvel]

@sdmaclea:

It might just be that because this is a 64K page platform arm64 platform, the 16M-64K does not leave enough space for stack address randomization

I think you are right, the 64 KB buffer was not enough. Based on the response from the Bugzilla bug above, I think 4 pages worth of gap should be more than enough. @sdmaclea, could you please try the test below (slightly modified from above to fix some issues, changed 64 KB buffer to 256 KB) and see if it works? If it works I'll go ahead and change the test back to what it was before and increase the buffer.

#include <unistd.h>
#include <errno.h>
#include <string.h>

#include <pthread.h>

#include <iostream>
using namespace std;

void PrintError(int error)
{
    const char *errorName = "?";
    switch(error)
    {
        case ENOMEM:
            errorName = "ENOMEM";
            break;
        case EINVAL:
            errorName = "EINVAL";
            break;
        case EAGAIN:
            errorName = "EAGAIN";
            break;
        case EPERM:
            errorName = "EPERM";
            break;
        default:
            break;
    }
    cout << errorName << " (" << error << ')';
}

bool CheckAndPrintErrorOnNonzero(const char *op, int error)
{
    if(error == 0)
        return true;
    cout << op << " failed: ";
    PrintError(error);
    cout << '\n';
    return false;
}

size_t requestedStackSize = 16 << 20;

void *ThreadStart(void *)
{
    char a[requestedStackSize - (256 << 10)];
    memset(a, 0, sizeof(a));
    cout << "success\n";
    return nullptr;
}

int main()
{
    pthread_attr_t attr;
    int error = pthread_attr_init(&attr);
    if(!CheckAndPrintErrorOnNonzero("attr_init", error))
        return 0;

    error = pthread_attr_setstacksize(&attr, requestedStackSize);
    if(!CheckAndPrintErrorOnNonzero("attr_setstacksize", error))
        return 0;

    pthread_t thread;
    error = pthread_create(&thread, &attr, &ThreadStart, nullptr);
    if(!CheckAndPrintErrorOnNonzero("create", error))
        return 0;

    error = pthread_attr_destroy(&attr);
    if(!CheckAndPrintErrorOnNonzero("attr_destroy", error))
        return 0;

    error = pthread_getattr_np(thread, &attr);
    if(!CheckAndPrintErrorOnNonzero("getattr", error))
        return 0;

    error = pthread_attr_destroy(&attr);
    if(!CheckAndPrintErrorOnNonzero("attr_destroy", error))
        return 0;

    error = pthread_join(thread, nullptr);
    if(!CheckAndPrintErrorOnNonzero("attr_destroy", error))
        return 0;

    return 0;
}

[sdmaclea]

@kouvel Latest test passes with "success"

[kouvel]

Ok great, thanks!