Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSX-Arm64 pthread_jit_write_protect_np(bool) does not have a sane initial value #41991

Closed
sdmaclea opened this issue Sep 8, 2020 · 10 comments · Fixed by #51135
Closed

OSX-Arm64 pthread_jit_write_protect_np(bool) does not have a sane initial value #41991

sdmaclea opened this issue Sep 8, 2020 · 10 comments · Fixed by #51135
Assignees
Labels
arch-arm64 area-VM-coreclr os-macos-bigsur (macOS11) tracking-external-issue The issue is caused by external problem (e.g. OS) - nothing we can do to fix it directly
Milestone

Comments

@sdmaclea
Copy link
Contributor

sdmaclea commented Sep 8, 2020

Discovered while debugging the Exceptions/ForeignThread/ForeignThreadExceptions/ForeignThreadExceptions.dll test.

The test creates a new thread which invokes JIT managed code.

int st = pthread_attr_init(&attr);
AbortIfFail(st);
// set stack size to 1.5MB
st = pthread_attr_setstacksize(&attr, 0x180000);
AbortIfFail(st);
pthread_t t;
st = pthread_create(&t, &attr, InvokeCallbackUnix, (void*)callback);
AbortIfFail(st);
st = pthread_join(t, NULL);

(lldb) bt
* thread #1, queue = 'com.apple.main-thread'
  * frame #0: 0x00000001d51db3d0 libsystem_kernel.dylib`__ulock_wait + 8
    frame #1: 0x00000001d528ec3c libsystem_pthread.dylib`_pthread_join + 452
    frame #2: 0x0000000102377ed8 libForeignThreadExceptionsNative.dylib`::InvokeCallbackOnNewThread(callback=(0x000000028003c210)) at ForeignThreadExceptionsNative.cpp:60:10
    frame #3: 0x000000028137ce34
    frame #4: 0x000000028137cbac
    frame #5: 0x000000028137c914
    frame #6: 0x000000010163f40c libcoreclr.dylib`CallDescrWorkerInternal at calldescrworkerarm64.S:71

The new thread is failing when it tries to execute the managed code.

* thread #9, stop reason = EXC_BAD_ACCESS (code=2, address=0x28003c210)
    frame #0: 0x000000028003c210
  * frame #1: 0x0000000102377e38 libForeignThreadExceptionsNative.dylib`InvokeCallbackUnix(callback=0x000000028003c210) at ForeignThreadExceptionsNative.cpp:31:5
    frame #2: 0x00000001d528d0f8 libsystem_pthread.dylib`_pthread_start + 320
(lldb) mem region 0x000000028003c210
[0x000000028003c000-0x0000000280040000) rwx

This is apparently occurring because the new thread does not default to pthread_jit_write_protect_np(true).

I believe we should try to have Apple set a default which allows new threads to execute JIT code.

/cc @janvorli

@sdmaclea sdmaclea added arch-arm64 tracking-external-issue The issue is caused by external problem (e.g. OS) - nothing we can do to fix it directly os-macos-bigsur (macOS11) labels Sep 8, 2020
@sdmaclea sdmaclea added this to the 6.0.0 milestone Sep 8, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Sep 8, 2020
@janvorli
Copy link
Member

janvorli commented Sep 8, 2020

I think we can handle that ourselves by calling the pthread_jit_write_protect_np in TheUMEntryPrestubWorker or maybe CreateThreadBlockThrow to establish the initial state.

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Sep 8, 2020

I don't see how that can happen on the clients new thread which gets a pointer to our managed code.

InvokeCallbackOnNewThread(callback=(0x000000028003c210))

Unless all delegates get that as a wrapper and the wrapper is not in JIT generated code

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Sep 8, 2020

I did wonder if we could hijack the exception and set the JIT write enable state in this case. I suspect we can, but it may introduce unnecessary security risk. A sane default seems safer.

@janvorli
Copy link
Member

janvorli commented Sep 8, 2020

All the callbacks go through the asm TheUMEntryPrestub that then calls the c++ TheUMEntryPrestubWorker.

@jkoritzinsky
Copy link
Member

UnmanagedCallersOnly methods don't go through TheUMEntryPrestub. They instead go through PreStubWorker_Preemptive IIRC.

@sdmaclea sdmaclea self-assigned this Sep 9, 2020
@sdmaclea sdmaclea removed the untriaged New issue has not been triaged by the area owner label Sep 9, 2020
@janvorli
Copy link
Member

janvorli commented Sep 9, 2020

Right, I was referring to regular callbacks.

@sdmaclea sdmaclea removed the tracking-external-issue The issue is caused by external problem (e.g. OS) - nothing we can do to fix it directly label Sep 9, 2020
@sdmaclea
Copy link
Contributor Author

sdmaclea commented Sep 9, 2020

I didn't find a test which failed due to PreStubWorker_Preemptive call from a foreign thread, perhaps there is no test coverage?

@sdmaclea sdmaclea closed this as completed Sep 9, 2020
@jkotas
Copy link
Member

jkotas commented Sep 9, 2020

Did you run src\tests\Interop\UnmanagedCallersOnly ?

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Sep 9, 2020

I think it was run.... I just reran it now too. Looks like there should be coverage for the new thread case.

The entry path is a little different, so perhaps it was getting JIT execute enabled someplace else....

@sdmaclea
Copy link
Contributor Author

sdmaclea commented Sep 9, 2020

Re-opened based on comments here #40435 (comment)

@BruceForstall BruceForstall added area-VM-coreclr and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Oct 9, 2020
@sdmaclea sdmaclea added the tracking-external-issue The issue is caused by external problem (e.g. OS) - nothing we can do to fix it directly label Jan 12, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 12, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 area-VM-coreclr os-macos-bigsur (macOS11) tracking-external-issue The issue is caused by external problem (e.g. OS) - nothing we can do to fix it directly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants