Skip to content

Fix GH-16727: Opcache bad signal 139 crash in ZTS bookworm (frankenphp) #16748

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

Closed
wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Nov 10, 2024

Reproducer: #16727 (comment), only happens when opcache is enabled. Reporter confirmed this fixes the problem for them and I also can't reproduce the issue anymore with this patch. I don't know how to write a test for this.

The root cause is a race condition between two different threads:

  1. We allocate a lower cased name for an anonymous class here:
    lcname = zend_string_tolower(name);
  2. This gets looked up as an interned string here:
    lcname = zend_new_interned_string(lcname);

    Assuming that there are uppercase symbols in the string and therefore
    lcname != name and that lcname is not yet in the interned string table,
    the pointer value of lcname won't change.
  3. Here we add the string into the interned string table:
    LITERAL_STR(opline->op1, lcname);

    However, in the meantime another thread could've added the string into the interned string table.
    This means that the following code will run, indirectly called via the LITERAL_STR macro,
    freeing lcname:
    if (ret) {
    zend_string_release(str);
    return ret;
    }
  4. In the reproducer we then access the freed lcname string here:
    if (!zend_hash_add_ptr(CG(class_table), lcname, ce)) {

This is solved in my patch by retrieving the interned string pointer and putting it in lcname.

…nphp)

Reproducer: php#16727 (comment)

The root cause is a data race between two different threads:

1) We allocate a lower cased name for an anonymous class here:
   https://github.com/php/php-src/blob/f97353f228e21dcc2db24d7edf08c1cb3678b0fd/Zend/zend_compile.c#L8109
2) This gets looked up as an interned string here:
   https://github.com/php/php-src/blob/f97353f228e21dcc2db24d7edf08c1cb3678b0fd/Zend/zend_compile.c#L8112
   Assuming that there are uppercase symbols in the string and therefore
   `lcname != name` and that `lcname` is not yet in the interned string table,
   the pointer value of `lcname` won't change.
3) Here we add the string into the interned string table:
   https://github.com/php/php-src/blob/f97353f228e21dcc2db24d7edf08c1cb3678b0fd/Zend/zend_compile.c#L8223
   However, in the meantime another thread could've added the string into the interned string table.
   This means that the following code will run, indirectly called via the `LITERAL_STR` macro,
   freeing `lcname`: https://github.com/php/php-src/blob/62e53e6f4965f37d379a3fd21f65a4210c5c86b5/ext/opcache/ZendAccelerator.c#L572-L575
4) In the reproducer we then access the freed `lcname` string here:
   https://github.com/php/php-src/blob/f97353f228e21dcc2db24d7edf08c1cb3678b0fd/Zend/zend_compile.c#L8229

This is solved in my patch by retrieving the interned string pointer
and putting it in `lcname`.
Copy link
Member

@dstogov dstogov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right.

@nielsdos nielsdos closed this in 02ee521 Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Opcache bad signal 139 crash in ZTS bookworm (frankenphp)
3 participants