Fix GH-10398: Fibers are broken on alpine x86 with clang

[nielsdos]

Fixes GH-10398, maybe fixes GH-9357 too.

[devnexen]

Note that the upstream project for those assembly codes come from here

[nielsdos]

Note that the upstream project for those assembly codes come from here

Unless I'm reading over it, it seems that the trampoline part isn't in the original?

[nielsdos]

Note that the upstream project for those assembly codes come from here

Unless I'm reading over it, it seems that the trampoline part isn't in the original?

Nvm I found it.

[nielsdos]

Just to be clear:
It also works if you reserve additional stack space, e.g. by adding subl $0x8, %esp at the start of the trampoline: code.
So the trampoline code was overwriting a part of the data passed to the function, or the C code actually does some kind of alignment thing in its machine code. By changing to pushes like I did in this PR, we don't risk overwriting part of the data.

Edit: I originally wrote addl above instead of subl, mistake when typing over the code

[devnexen]

Your alternative solution has my personal preference, but nevermind you can perfectly defend your case upstream in due time, I have only contributed once a while ago but they re pretty nice as far as I remember.

[trowski]

@nielsdos Could you please submit this fix upstream to boost/context first? I prefer to get their insight and approval before applying changes to the bundled assembly files here.

[nielsdos]

@trowski I spent some more time figuring out this issue, and it finally clicked. The patch in this PR only fixes the issue by accident. There is no overwrite issue, it's a stack alignment issue...
Upon entering trampoline, the stack isn't 16-byte aligned. This alignment is requires by the SYSV calling convention. The crash then occurs in zend_fiber_trampoline where a SSE instruction is used: movaps %xmm0, 0x10(%esp) but because the stack isn't aligned and SSE requires alignment, the CPU traps.

The fix is simple, this code:

php-src/Zend/asm/make_i386_sysv_elf_gas.S

Line 44 in 90b0e77

leal -0x28(%eax), %eax

Should actually become:

leal  -0x30(%eax), %eax

This works because 0x30 is a multiple of 16 and the stack was already 16-byte aligned at this point. Other assembly files for i386 may require a modification as well.

I'll make PR to the boost context repo soon-ish.

[nielsdos]

My fix got merged in the boost/context repo.
I can update this PR in one of two possible ways:
(1) We only update the i386 asm files
(2) We update all asm files
I guess we should do (2) as that will bring us up to date with all the changes of upstream?

[nielsdos]

I've gone for strategy (2) and updated all assembly files.

[trowski]

Merging in all the changes from boost/context has brought in the the changes made for shadow stack support in #9283, but this is fine I think because the flag isn't set in 8.1 and 8.2, so those sections are skipped anyway.

There's also changes adding a BOOST_CONTEXT_TLS_STACK_PROTECTOR flag from this PR boostorg/context#205. Do we have support for -fstack-protector? If so, some changes may be needed to add this flag (probably not in this PR, maybe only in master).

[nielsdos]

I don't see an option for stack protection in the configure script. I think that the stack protector is off in standard GCC/Clang, but some distros turn it on by default (and also the fortify options iirc). But yeah that can probably go in a separate PR.

[trowski]

👍 Let's merge this then and look at what needs to be done for stack protection and defining BOOST_CONTEXT_TLS_STACK_PROTECTOR in another PR.

[andypost]

FYI I've applied PR as patch and x86-CI no loner reports failed fiber tests https://gitlab.alpinelinux.org/alpine/aports/-/jobs/965476

[andypost]

btw uncommented 3 tests that failed on ppc64le https://gitlab.alpinelinux.org/alpine/aports/-/jobs/965496

Zend/tests/fibers/no-switch-force-close-finally.phpt
Zend/tests/fibers/suspend-in-force-close-fiber-after-shutdown.phpt
Zend/tests/fibers/throw-in-multiple-destroyed-fibers-after-shutdown.phpt

[andypost]

bit they are fails

TEST 2620/15985 [Zend/tests/fibers/no-switch-force-close-finally.phpt]
========DIFF========
     finally
002- 
003- Fatal error: Uncaught FiberError: Cannot switch fibers in current execution context in %sno-switch-force-close-finally.php:%d
004- Stack trace:
005- #0 %sno-switch-force-close-finally.php(%d): Fiber->start()
006- #1 [internal function]: {closure}()
007- #2 {main}
008-   thrown in %sno-switch-force-close-finally.php on line %d
========DONE========
FAIL Cannot start a new fiber in a finally block in a force-closed fiber [Zend/tests/fibers/no-switch-force-close-finally.phpt] 
...
TEST 2636/15985 [Zend/tests/fibers/suspend-in-force-close-fiber-after-shutdown.phpt]
========DIFF========
     done
002- 
003- Fatal error: Uncaught FiberError: Cannot suspend in a force-closed fiber in %ssuspend-in-force-close-fiber-after-shutdown.php:%d
004- Stack trace:
005- #0 %ssuspend-in-force-close-fiber-after-shutdown.php(%d): Fiber::suspend()
006- #1 [internal function]: {closure}()
007- #2 {main}
008-   thrown in %ssuspend-in-force-close-fiber-after-shutdown.php on line %d
========DONE========
FAIL Suspend in force-closed fiber after shutdown [Zend/tests/fibers/suspend-in-force-close-fiber-after-shutdown.phpt] 
...
TEST 2642/15985 [Zend/tests/fibers/throw-in-multiple-destroyed-fibers-after-shutdown.phpt]
========DIFF========
     done
002- 
003- Fatal error: Uncaught Exception: test1 in %sthrow-in-multiple-destroyed-fibers-after-shutdown.php:%d
004- Stack trace:
005- #0 [internal function]: {closure}()
006- #1 {main}
007- 
008- Next Exception: test2 in %sthrow-in-multiple-destroyed-fibers-after-shutdown.php:%d
009- Stack trace:
010- #0 [internal function]: {closure}()
011- #1 {main}
012-   thrown in %sthrow-in-multiple-destroyed-fibers-after-shutdown.php on line %d
========DONE========
FAIL Throw in multiple destroyed fibers after shutdown [Zend/tests/fibers/throw-in-multiple-destroyed-fibers-after-shutdown.phpt] 

[nielsdos]

Yeah I only spent time debugging and fixing x86, I might look at the PPC cases sometime later, but that's unrelated to this PR though.

[andypost]

Thank you!

[nielsdos]

Thank you!

My pleasure :)

@devnexen Thanks for merging. This also fixes GH-9357 as confirmed here: #10407 (comment). Could you please close that issue too? :)

[andypost]

Follow-up for fibers on ppc64le #10512