Skip to content

Fixed bug #76337: use interned string for class alias name#3241

Closed
xKerman wants to merge 3 commits intophp:PHP-7.2from
xKerman:fix/bug-76337
Closed

Fixed bug #76337: use interned string for class alias name#3241
xKerman wants to merge 3 commits intophp:PHP-7.2from
xKerman:fix/bug-76337

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

carusogabriel commented May 16, 2018

@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

weltling commented May 17, 2018

@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.

nikic
nikic approved these changes May 19, 2018
View reviewed changes
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

weltling commented May 20, 2018

@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

Reviewers

@nikic nikic nikic approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xKerman @carusogabriel @weltling @nikic