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

Segfault when using references in thread #306

Closed
realFlowControl opened this issue Jun 16, 2024 · 4 comments
Closed

Segfault when using references in thread #306

realFlowControl opened this issue Jun 16, 2024 · 4 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@realFlowControl
Copy link
Collaborator

realFlowControl commented Jun 16, 2024

Running code that uses a reference to an element in an array in a thread segfaults with the following backtrace:

    frame #0: 0x000000018f386a60 libsystem_kernel.dylib`__pthread_kill + 8
    frame #1: 0x000000018f3bec20 libsystem_pthread.dylib`pthread_kill + 288
    frame #2: 0x000000018f2cba30 libsystem_c.dylib`abort + 180
    frame #3: 0x000000010036c32c php`zend_mm_panic + 52
    frame #4: 0x000000010036e930 php`_efree_32 + 120
    frame #5: 0x00000001003ea2f0 php`ZEND_ASSIGN_REF_SPEC_CV_VAR_HANDLER + 288
    frame #6: 0x00000001003cdc38 php`execute_ex + 172
    frame #7: 0x000000010104d03c parallel.so`php_parallel_scheduler_run(runtime=<unavailable>, frame=0x0000000102413020) at scheduler.c:331:13 [opt]
    frame #8: 0x000000010104c62c parallel.so`php_parallel_thread(arg=0x000000010125f3c0) at scheduler.c:501:9 [opt]
    frame #9: 0x000000018f3bef94 libsystem_pthread.dylib`_pthread_start + 136

You can reproduce using https://github.com/realFlowControl/1brc/blob/main/calculateAverage.php, the opline points to line 71 (where the array is accessed). If I comment the entire if-block, it segfaults in another line.

It started with 32cd9c3 and the change that leads to this is this line:

GC_SET_REFCOUNT(ht, 1);

Setting this back to 2 will fix the segfault, but lead to ASAN tests failing ;-)

I sadly have no idea what is going on :-(

Edit (2024-10-02): if OPcache is enabled, this works like a charm, without any issues. As soon as I disable OPcache, it crashes

@hschimpf
Copy link

hschimpf commented Jul 9, 2024

Hi @realFlowControl!

Does this happen on any PHP version?

@realFlowControl
Copy link
Collaborator Author

So far this issue is in the develop branch and the changes that lead to that are not yet released (that's why i'm withholding new minor releases). So it should not be related to hschimpf/parallel-sdk#26

@realFlowControl
Copy link
Collaborator Author

realFlowControl commented Oct 2, 2024

Interestingly another way to workaround in case no OPcache is around is to declare the $stations array static in

https://github.com/realFlowControl/1brc/blob/57cc8737e9f2593c907ce6e845e682fadc1ccf31/calculateAverage.php#L59

@arnaud-lb
Copy link

arnaud-lb commented Oct 15, 2024

Smaller reproducer:

<?php

$run = function() {
    $a = [];
    for ($i = 0; $i < 100; $i++) {
        $b = &$a[$i];
    }
    return 0;
};

for ($i = 0; $i < 10; $i++) {
    $futures[$i] = \parallel\run($run, []);
}

for ($i = 0; $i < 10; $i++) {
    var_dump($futures[$i]->value());
}

What is happening is that an array literal is copied to $a in $a = [];, but its refcount is not incremented because the IS_TYPE_REFCOUNTED flag is not set. Multiple threads will have the same array literal in $a, with a refcount of 1, and will modify it, which breaks semantics and also causes corruption and crash.

A refcount of 2 prevented this by forcing the VM to separate the array before modifying it.

Reverting two lines here and here would fix the issue, but this is inconsistent with other code in copy.c.

Changing this line to Z_TYPE_FLAGS_P(slot) &= ~IS_TYPE_COLLECTABLE; (so the IS_TYPE_REFCOUNTED is not removed) would also fix the problem, but this may be unsafe as ADDREF/DELREF are not thread safe.

Edit: When OPcache is enabled, parallel relies on it to manage literals. In this case, literals have a refcount of 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants