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

BlockImpTest failures with Clang 8 on RHEL8 #147

Closed
woachk opened this issue Feb 11, 2020 · 4 comments · Fixed by #148
Closed

BlockImpTest failures with Clang 8 on RHEL8 #147

woachk opened this issue Feb 11, 2020 · 4 comments · Fixed by #148

Comments

@woachk
Copy link

woachk commented Feb 11, 2020

On latest master:

`98% tests passed, 4 tests failed out of 178

Total Test time (real) = 29.15 sec

The following tests FAILED:
25 - BlockImpTest (SEGFAULT)
26 - BlockImpTest_optimised (SEGFAULT)
27 - BlockImpTest_legacy (SEGFAULT)
28 - BlockImpTest_legacy_optimised (SEGFAULT)`

`$ lldb Test/BlockImpTest
(lldb) target create "Test/BlockImpTest"
Current executable set to 'Test/BlockImpTest' (x86_64).
(lldb) run
Process 4784 launched: '/home/ares/labs2/libobjc2/Build/Test/BlockImpTest' (x86_64)
Process 4784 stopped

  • thread 1.7.1 release? #1, name = 'BlockImpTest', stop reason = signal SIGSEGV: address access protected (fault address: 0x652000)
    frame #0: 0x0000000000652000
    -> 0x652000: movq -0x1007(%rip), %rsi
    0x652007: xchgq %rsi, %rdi
    0x65200a: jmpq *-0x1008(%rip)
    0x652010: movq -0x1007(%rip), %rsi
    (lldb) bt
  • thread 1.7.1 release? #1, name = 'BlockImpTest', stop reason = signal SIGSEGV: address access protected (fault address: 0x652000)
@davidchisnall
Copy link
Member

Do you have any unusual SELinux settings enabled? The block-to-imp code has been tested (in CI) on x86-64 Linux, but it may be that you have a setting that prevents changing permission of pages from read-write to read-execute.

@woachk
Copy link
Author

woachk commented Feb 12, 2020

Hello,

I use the default SELinux policy for CentOS 8. I can however confirm that all the tests pass with SELinux off. (tested it on another machine using the same OS)

For reference, Clang version is:
$ clang -v
clang version 8.0.1 (Red Hat 8.0.1-1.module_el8.1.0+215+a01033fb)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Found candidate GCC installation: /usr/bin/../lib/gcc/x86_64-redhat-linux/8
Found candidate GCC installation: /usr/lib/gcc/x86_64-redhat-linux/8
Selected GCC installation: /usr/bin/../lib/gcc/x86_64-redhat-linux/8
Candidate multilib: .;@m64
Candidate multilib: 32;@m32
Selected multilib: .;@m64

Thanks,

@ngrewe
Copy link
Member

ngrewe commented Feb 12, 2020

A quick check reveals that using mmap() with MAP_ANONYMOUS makes things work under CentOS 8's SELinux policy. As far as I'm aware, MAP_ANONYMOUS is not portable, though.

diff --git a/block_to_imp.c b/block_to_imp.c
index 655489a..8377623 100644
--- a/block_to_imp.c
+++ b/block_to_imp.c
@@ -163,7 +163,7 @@ static struct trampoline_set *alloc_trampolines(char *start, char *end)
 #if _WIN32
        metadata->buffers = VirtualAlloc(NULL, sizeof(struct trampoline_buffers), MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
 #else
-       posix_memalign((void **)&metadata->buffers, getpagesize(), sizeof(struct trampoline_buffers));
+       metadata->buffers = mmap(NULL, sizeof(struct trampoline_buffers), PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
 #endif
        for (int i=0 ; i<HEADERS_PER_PAGE ; i++)
        {

@davidchisnall
Copy link
Member

Interesting. I'm not sure why we're using posix_memalign here. Please can you send a PR that replaces it with mmap? There's no reason why it can't use mmap, the Windows code path uses the Windows equivalent.

ngrewe added a commit that referenced this issue Feb 12, 2020
Should fix #147 (SELinux related segfault)
ngrewe added a commit that referenced this issue Feb 12, 2020
Should fix #147 (SELinux related segfault)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants