php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #74011 Access Violation on ZTS Embed SAPI Shutdown
Submitted: 2017-01-29 21:57 UTC Modified: 2017-03-29 12:12 UTC
From: maroszek at gmx dot net Assigned: ab (profile)
Status: Closed Package: Reproducible crash
PHP Version: 7.1.1 OS: Windows 10
Private report: No CVE-ID: None
 [2017-01-29 21:57 UTC] maroszek at gmx dot net
Description:
------------
PHP 7.1.1 + This patch: https://gist.github.com/weltling/e0922914dca8c8ee1886 (See bug #71129)

You need the patch. Otherwise you won't get to the shutdown part because it will crash during parallel execution.

Build the test app and run it. It will crash on module shutdown.

Hints:
It will NOT crash if no script is started
It will NOT crash if scripts are started from main
It will NOT crash on MacOS/Linux

Test script:
---------------
#include <stdio.h>
#include <iostream>
#include <thread>
#include <list>

#define PHP_WIN32 1
#define ZEND_WIN32 1
#define ZTS 1
#define ZEND_DEBUG 0

//#define _USE_32BIT_TIME_T 0

#include <main/php.h>
#include <main/SAPI.h>
#include <main/php_main.h>
#include <main/php_variables.h>
#include <main/php_ini.h>
#include <zend_ini.h>

#if defined(PHP_WIN32) && defined(ZTS)
ZEND_TSRMLS_CACHE_DEFINE();
#endif

zend_module_entry ips_module_entry = {
	STANDARD_MODULE_HEADER,
	"XYZ",
	NULL,
	NULL,
	NULL,
	NULL,
	NULL,
	NULL,
	NO_VERSION_YET,
	STANDARD_MODULE_PROPERTIES
};

static size_t php_embed_read_post(char *str, size_t str_length)
{
	return 0;
}

static char* php_embed_read_cookies()
{
	return NULL;
}

static size_t php_embed_ub_write(const char *str, size_t str_length)
{
	std::cout << str;
	return str_length;
}

static void php_embed_flush(void *server_context)
{
	//
}

static void php_embed_send_header(sapi_header_struct *sapi_header, void *server_context)
{
	//
}

static void php_embed_log_message(char *message, int type)
{
	fprintf(stderr, "%s\n", message);
}

static void php_embed_register_variables(zval *track_vars_array)
{
	//zval ipsValue;
	//ZVAL_STRING(&ipsValue, "Blaaaaaaaa");
	//php_register_variable_ex("Blubber", &ipsValue, track_vars_array);
}

static int php_embed_startup(sapi_module_struct *sapi_module)
{
	if (php_module_startup(sapi_module, NULL, 0) == FAILURE) {
		return FAILURE;
	}
	return SUCCESS;
}

sapi_module_struct php_embed_module = {
	"XYZ",                       /* name */
	"PHP for XYZ",               /* pretty name */

	php_embed_startup,                 /* startup */
	php_module_shutdown_wrapper,       /* shutdown */

	NULL,                              /* activate */
	NULL,                              /* deactivate */

	php_embed_ub_write,                /* unbuffered write */
	php_embed_flush,                   /* flush */
	NULL,                              /* get uid */
	NULL,                              /* getenv */

	php_error,                         /* error handler */

	NULL,                              /* header handler */
	NULL,                              /* send headers handler */
	php_embed_send_header,             /* send header handler */

	php_embed_read_post,               /* read POST data */
	php_embed_read_cookies,            /* read Cookies */

	php_embed_register_variables,   /* register server variables */
	php_embed_log_message,          /* Log message */
	NULL,							/* Get request time */
	NULL,							/* Child terminate */

	STANDARD_SAPI_MODULE_PROPERTIES
};

unsigned __stdcall phpThread(void* params)
{
	ts_resource(0);
#if defined(PHP_WIN32) && defined(ZTS)
	ZEND_TSRMLS_CACHE_UPDATE();
#endif

	for (int i = 0; i < 10; i++) {
		zend_file_handle file_handle;
		file_handle.type = ZEND_HANDLE_FILENAME;
		file_handle.filename = "date.php";
		file_handle.handle.fp = NULL;
		file_handle.opened_path = NULL;
		file_handle.free_filename = 0;

		if (php_request_startup(TSRMLS_C) == FAILURE) {
			//std::cout << "ERR" << std::endl;
			php_request_shutdown(NULL);
			continue;
		}

		if (php_execute_script(&file_handle TSRMLS_CC) == FAILURE)
			break;

		//std::cout << "RUN" << std::endl;
		php_request_shutdown(NULL);
	}

	std::cout << "FINISHED" << std::endl;

	return 0;
}

int main(int argc, const char * argv[]) {

	tsrm_startup(128, 1, 0, NULL);

	zend_signal_startup();

	sapi_startup(&php_embed_module);

	php_embed_module.ini_entries = "date.timezone=Europe/Berlin\n\0";

	if (php_embed_module.startup(&php_embed_module) == FAILURE) {
		throw std::runtime_error("Could not start PHP!");
	}

	std::list<std::shared_ptr<std::thread>> threads;
	for (int i = 0; i < 1; i++) {
		threads.push_back(std::make_shared<std::thread>([]{phpThread(NULL);}));
	}

	for(auto& thread: threads) {
		thread->join();
	}

	php_embed_module.shutdown(&php_embed_module);
	sapi_shutdown();
	tsrm_shutdown();

	return 0;
}

Expected result:
----------------
Nice and clean shutdown

Actual result:
--------------
php7ts.dll!zend_llist_destroy(_zend_llist * l)
php7ts.dll!php_shutdown_ticks()
php7ts.dll!ts_free_id(int id)
php7ts.dll!php_module_shutdown()
php7ts.dll!php_module_shutdown_wrapper(_sapi_module_struct * sapi_globals)
PHPCrash.exe!main(int argc, const char * * argv)

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-01-30 11:44 UTC] krakjoe@php.net
-Assigned To: +Assigned To: ab
 [2017-01-30 11:44 UTC] krakjoe@php.net
Anatol, can you please have a look at this, you seem somewhat familiar with embed ?
 [2017-01-31 17:56 UTC] ab@php.net
-Status: Assigned +Status: Feedback
 [2017-01-31 17:56 UTC] ab@php.net
Yeah, Joe, lets re-evaluate this, past more than a year. So thanks for the ping :)

@maroszek, I had issues with the exact patch with Apache, that's why it was reverted in 7.0. I also see that the backtrace in this ticket is different from that in #71129. So what is essentially different in this ticket from the previous one?

Thanks.
 [2017-01-31 20:33 UTC] maroszek at gmx dot net
I think both problems are not directly related to each other. I was able to reproduce the crash from this report without the mentioned patch as well. Though it might crash with the other stacktrace, which we should handle in the other report.

The simple difference is:
a) This bug is about a segfault on module shutdown
b) The other bug is a segfault during runtime

If i can give you any more information i'll gladly do it. Thanks for looking into it!
 [2017-02-06 14:46 UTC] maroszek at gmx dot net
Hi!

Were you able to reproduce the issue? 
Can i give you any more feedback? 

I can provide the full project source + exe if you like.
 [2017-02-09 12:31 UTC] ab@php.net
-Status: Feedback +Status: Open
 [2017-02-09 12:31 UTC] ab@php.net
@maroszek, i'm investigating on the similar issues right now, which seem to belong to the same bundle. So far i don't see any quick solution to this :( But the work continues.

Thanks.
 [2017-02-20 11:18 UTC] maroszek at gmx dot net
@ab: Sorry for being so persistent. Do you have any updates? Is there anything i can do to help? Is there any summary of related issues and ideas for fixes i could potentially work on?

Thanks!
 [2017-02-22 01:11 UTC] ab@php.net
@maroszek, there is a work in progress branch dealing with interned strings https://github.com/php/php-src/pull/2390 , which are the cause of issues in TS builds currently. The patch linked in the description was doing just a local fix in this direction, the PR targets a robust implementation.

Regarding the code you've linked - I still couldn't come to it, but was checking the code itself. Also checked and read the old linked issue. The thing is, and that's what wondered me earlier as well - it doesn't look like you indeed use the embed SAPI, the provided init/shutdown routines are not used. It looks like you develop some more or less new SAPI, based on embed. And in this case, it is a completely different story, which should not be sold as a bug in the embed SAPI itself. I also read other users comments in the old ticket about this.

With the core SAPIs - yes, the bugs are targeted as bug reports. But with a custom approach, that reuses things partially - it is hard to say where it's a mistake in your own code or a real bug in the core. If you're curious, you might check the linked PR. Given also that we have TS issues indeed, maybe it'd make sense to keep this bug. But otherwise - i barely see such code as a core bug in principle. Usually you would first ask for help on mailing lists about a custom SAPI development. I'd frankly see more willingly and would find more simple a reproducer with an exact embed SAPI, without ary unnecessary customization.

Thanks.
 [2017-03-08 17:49 UTC] ab@php.net
@maroszek, could you please check the current master. The interned strings are now supported in thread safe builds, so the issues from #71115 sohuld be fixed as well.

Thanks.
 [2017-03-10 08:53 UTC] maroszek at gmx dot net
Thanks for fixing the issue! For Linux it works as is - for Windows i had to make a small change to my custom SAPI.

I was using this line, which was causing crashes on Windows.

    tsrm_startup(128, 1, 0, NULL);

After changing it to the way the embed library does it the crashes went away.

    tsrm_startup(1, 1, 0, NULL);

What i don't understand, is how a parameter that is named expected_threads can causes crashes, after setting it to a sane value.

Here is my reworked crash example which uses the embed library: https://gist.github.com/paresy/3cbd4c6a469511ac7479aa0e7c42fea7
I will try my best to post next reports based on the official embed implementation! :)
 [2017-03-29 12:12 UTC] ab@php.net
-Status: Assigned +Status: Closed
 [2017-03-29 12:12 UTC] ab@php.net
@maroszek many thanks for the checks. I've compiled the snippet from the description and your latest one based on pure sapi/embed. With the embed snippet it seems hard to reproduce the crash even with 7.0, however with the original snippet i can confirm there are no crashes with master anymore. It was expected, however. Thus, closing this one and the predecessor ticket. Well, as an issue is an issue, that's fine with a custom SAPI, just it was a bit misleading to have it linked to embed where it was a custom SAPI indeed. Looking forward to 7.2 anyway, with the huge stability increase in this regard :)

Please, lets investigate the issue with tsrm_startup you've mentioned in the separate ticket, would need some backtrace and the usual stuff. Please file a ticket, even you could assign it to me right away.

Thanks.
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Sat Dec 05 23:01:23 2020 UTC