Skip to content

Conversation

xKerman
Copy link
Contributor

@xKerman xKerman commented May 14, 2018

@xKerman xKerman changed the title Fixed bug #76337 Fixed bug #76337: use interned string for class alias name May 15, 2018
@carusogabriel
Copy link
Contributor

@xKerman Maybe a test for new users understand what was wrong? :)

@xKerman
Copy link
Contributor Author

xKerman commented May 16, 2018

I see, I will explain what the cause is.

@xKerman
Copy link
Contributor Author

xKerman commented May 16, 2018

@carusogabriel added test for this fix using zend_test module, and commented what the cause is in https://bugs.php.net/bug.php?id=76337#1526488561 .

@weltling
Copy link
Contributor

@xKerman strangely the test code doesn't crash on my side without the patch. Perhaps load order or something else yet involved?

Thanks.

@xKerman
Copy link
Contributor Author

xKerman commented May 17, 2018

@weltling I can reproduce that with macOS + clang compiler.
On CentOS6 + gcc compiler, I could not reproduce.

It seems that, for some compiler optimization, null pointer check is skipped in accel_replace_string_by_process_permanent(str), if PHP is compiled without --enable-debug flag.
https://github.com/php/php-src/blob/php-7.2.6RC1/ext/opcache/ZendAccelerator.c#L633
So class alias name string (non interned string) is replaced with NULL pointer in https://github.com/php/php-src/blob/php-7.2.6RC1/ext/opcache/ZendAccelerator.c#L547-L549 .

NULL key in CG(class_table) does not destroyed in zend_hash_destroy(GLOBAL_CLASS_TABLE), so segmentaion fault does not occur.
https://github.com/php/php-src/blob/php-7.2.6RC1/Zend/zend.c#L911

(lldb) breakpoint set --name accel_replace_string_by_process_permanent --condition '(int)strcmp(str->val, "_zendtestclassalias") == 0'
Breakpoint 3: 18 locations.
(lldb) c
Process 5347 resuming
Process 5347 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 3.7
    frame #0: 0x0000000101b04758 opcache.so`accel_shutdown [inlined] accel_replace_string_by_process_permanent(str=0x00000001031f8a20) at ZendAccelerator.c:631
[opt]
   628
   629  static zend_string *accel_replace_string_by_process_permanent(zend_string *str)
   630  {
-> 631          zend_string *ret = zend_interned_string_find_permanent(str);
   632
   633          if (ret) {
   634                  zend_string_release(str);
Target 0: (php) stopped.
(lldb) p str
(zend_string *) $8 = 0x00000001031f8a20
(lldb) p str->val
(char [1]) $9 = "_zendtestclassalias"
(lldb) p compiler_globals->class_table->arData[157]->key
(zend_string *) $10 = 0x00000001031f8a20
  Fix-it applied, fixed expression was:
    compiler_globals.class_table->arData[157].key

(lldb) n
Process 5347 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x0000000101b04763 opcache.so`accel_shutdown [inlined] accel_replace_string_by_process_permanent(str=0x00000001031f8a20) at ZendAccelerator.c:634
[opt]
   631          zend_string *ret = zend_interned_string_find_permanent(str);
   632
   633          if (ret) {
-> 634                  zend_string_release(str);
   635                  return ret;
   636          }
   637          ZEND_ASSERT(0);
Target 0: (php) stopped.
(lldb) p ret
(zend_string *) $11 = 0x0000000000000000

(lldb) n
Process 5347 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = step over
    frame #0: 0x0000000101b04790 opcache.so`accel_shutdown at ZendAccelerator.c:
548 [opt]
   545                  ce = (zend_class_entry*)Z_PTR(p->val);
   546
   547                  if (p->key) {
-> 548                          p->key = new_interned_string(p->key);
   549                  }
   550
   551                  if (ce->name) {
Target 0: (php) stopped.
(lldb) n
(lldb) p compiler_globals->class_table->arData[157]->key
(zend_string *) $13 = 0x0000000000000000
  Fix-it applied, fixed expression was:
    compiler_globals.class_table->arData[157].key

So perhaps test code will crash if deleting ZEND_ASSERT(0); in https://github.com/php/php-src/blob/php-7.2.6RC1/ext/opcache/ZendAccelerator.c#L637 and then compile PHP with --enable-debug.

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM, AppVeyor failures are unrelated.

@weltling
Copy link
Contributor

@xKerman yeah, you're right. I didn't notice i was testing on the release build, too. Hitting the assert in the debug build is reliable anyway. Merged as 5681f65.

Thanks!

@weltling weltling closed this May 20, 2018
@xKerman
Copy link
Contributor Author

xKerman commented May 20, 2018

Thanks for comment, review and merge!

@xKerman xKerman deleted the fix/bug-76337 branch May 20, 2018 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants