Fix SegFault on serialization of objects

SwenVanZanten commented 4 years ago

I've encountered a problem with upgrading a codebase to PHP 7.4 using the latest build of this extension.

In this situation we have some objects which contain a GearmanJob object. When serializing and unserializing a bunch of them you'll get a Segmentation Fault.

I'm not familiar with writing code in C and I'm not sure this is the proper solution to the problem. So correct me if I'm wrong here! However it solves my problem and doesn't introduce any new once... according to the testsuite 😜

Trainmaster commented 4 years ago

@SwenVanZanten I think that was already fixed in master, see https://github.com/php/pecl-networking-gearman/commit/89db5655df81b1a2123733e65c03d74ea23064f7.

SwenVanZanten commented 4 years ago

It did fix running in PHP 7.4, however it doesn't work for the test scenario's I've added in the PR. If you run those on master they'll fail.

SwenVanZanten commented 4 years ago

@Trainmaster hi any chance to check this PR out?

oleg-st commented 4 years ago

@SwenVanZanten I can confirm the problem. I think better solution is to remove zend_object_std_dtor call in destructors. Because it is called in free_obj by default.

C4RoCKeT commented 4 years ago

I tried your fixes, and it completes all the tests, but I'm still getting segmentation faults using PHP 7.4.3.

SwenVanZanten commented 4 years ago

I tried your fixes, and it completes all the tests, but I'm still getting segmentation faults using PHP 7.4.3.

With serialization & unserialization or another use case?

Trainmaster commented 4 years ago

Maybe @nikic can help us out at this point (https://github.com/php/php-src/commit/1cfbb21790ff6dd4931223c5bdc18a0cebf3ffd4). Because I would say that

It is recommended to initialize object handler structures by copying the std object handlers and only overwriting those you want to change.

was implemented by https://github.com/php/pecl-networking-gearman/commit/afd2ed880c67455fbf28097dfde7bd3e368c183e.

nikic commented 4 years ago

@Trainmaster Presumably there was a reason why the object destruction was previously skipped -- most likely, it was to work around a refcounting bug somewhere else.

From a quick look, I'd assume that this kind of code is the root problem: https://github.com/php/pecl-networking-gearman/blob/a787fa0ca5ff5ab4abc30d440ddfc77eea55b825/php_gearman_task.c#L130-L143 That code should be inside the free_obj handler, not in destruct(). Putting it in destruct() is trivially unsound if you consider that the method may be manually invoked multiple times.

C4RoCKeT commented 4 years ago

I tried your fixes, and it completes all the tests, but I'm still getting segmentation faults using PHP 7.4.3.

With serialization & unserialization or another use case?

Actually, just using $client = new GearmanClient(); will cause one.

SwenVanZanten commented 4 years ago

@Trainmaster Presumably there was a reason why the object destruction was previously skipped -- most likely, it was to work around a refcounting bug somewhere else.

From a quick look, I'd assume that this kind of code is the root problem:

https://github.com/php/pecl-networking-gearman/blob/a787fa0ca5ff5ab4abc30d440ddfc77eea55b825/php_gearman_task.c#L130-L143

That code should be inside the free_obj handler, not in destruct(). Putting it in destruct() is trivially unsound if you consider that the method may be manually invoked multiple times.

Thnx @nikic !

I'll give it another go tomorrow

SwenVanZanten commented 4 years ago

Added extra test cases for GearmanTask and GearmanWorker