Fix SegFault on serialization of objects

[SwenVanZanten]

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]

@SwenVanZanten I think that was already fixed in master, see 89db565.

[SwenVanZanten]

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]

@Trainmaster hi any chance to check this PR out?

[oleg-st]

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

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

[SwenVanZanten]

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]

Maybe @nikic can help us out at this point (php/php-src@1cfbb21). 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 afd2ed8.

[nikic]

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

pecl-networking-gearman/php_gearman_task.c

Lines 130 to 143 in a787fa0

	/* {{{ proto object GearmanTask::__destruct()
	Destroys a task object */
	PHP_METHOD(GearmanTask, __destruct) {
	gearman_task_obj *intern = Z_GEARMAN_TASK_P(getThis());
	if (!intern) {
	return;
	}
	zval_dtor(&intern->zworkload);
	zval_dtor(&intern->zdata);
	zval_dtor(&intern->zclient);
	zend_object_std_dtor(&intern->std);
	}

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]

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]

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

pecl-networking-gearman/php_gearman_task.c

Lines 130 to 143 in a787fa0

	/* {{{ proto object GearmanTask::__destruct()
	Destroys a task object */
	PHP_METHOD(GearmanTask, __destruct) {
	gearman_task_obj *intern = Z_GEARMAN_TASK_P(getThis());
	if (!intern) {
	return;
	}
	zval_dtor(&intern->zworkload);
	zval_dtor(&intern->zdata);
	zval_dtor(&intern->zclient);
	zend_object_std_dtor(&intern->std);
	}

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]

Added extra test cases for GearmanTask and GearmanWorker

[ekosogin]

If it's stable please push tag with new version

[Trainmaster]

@rlerdorf removed the Note: This is no longer under active development.. Maybe he can tell us who's in charge of this repository now.

[mmoll]

I can confirm, this PR fixes segfaults seen with PHP 7.4. 👍🏿

[NoiseEee]

Hi folks, when can we see all these pull requests finally merged??

[ekosogin]

I also confirm it's stable