Skip to content

PHP: fix zend_hash_str_del() segfault#20249

Merged
stanley-cheung merged 2 commits intogrpc:masterfrom
HannahShiSFB:enable-debug-19970-19248
Sep 24, 2019
Merged

PHP: fix zend_hash_str_del() segfault#20249
stanley-cheung merged 2 commits intogrpc:masterfrom
HannahShiSFB:enable-debug-19970-19248

Conversation

@HannahShiSFB
Copy link
Copy Markdown
Collaborator

@HannahShiSFB HannahShiSFB commented Sep 13, 2019

avoid calling php_grpc_zend_hash_del() at all in the constructor.
keys ("credentials", "force_new", "grpc_target_persist_bound") cannot del from hash table since php refers them somewhere else.
in php_grpc_read_args_array(), ignore 3 special keys, when copy info to args.
using args, instead of args_array, to construct a sha1 hash key

ps: #19970, #19248 can be reproduced using unit tests in any os+enable debug

@HannahShiSFB HannahShiSFB changed the title fix zend_hash_str_del: Assertion failed: ((ht)->gc.refcount == 1) with enable debug, 19248, 19970 PHP: fix zend_hash_str_del: Assertion failed: ((ht)->gc.refcount == 1) with enable debug, 19248, 19970 Sep 13, 2019
@stanley-cheung stanley-cheung self-requested a review September 13, 2019 20:13
@stanley-cheung stanley-cheung added lang/php release notes: no Indicates if PR should not be in release notes labels Sep 13, 2019
@stanley-cheung stanley-cheung changed the title PHP: fix zend_hash_str_del: Assertion failed: ((ht)->gc.refcount == 1) with enable debug, 19248, 19970 PHP: fix zend_hash_str_del() segfault Sep 13, 2019
@stanley-cheung stanley-cheung self-assigned this Sep 13, 2019
@HannahShiSFB HannahShiSFB reopened this Sep 13, 2019
@stanley-cheung stanley-cheung added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels Sep 24, 2019
@stanley-cheung stanley-cheung merged commit a9533e7 into grpc:master Sep 24, 2019
Copy link
Copy Markdown
Contributor

@ZhouyihaiDing ZhouyihaiDing left a comment

Choose a reason for hiding this comment

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

LGTM. Approval.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lang/php release notes: yes Indicates if PR needs to be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants