Open thekid opened 5 years ago
Everything works fine if I remove the TestReceiverDestroy test and move the cleanup code before the c.Destroy()
inside TestReceiverDefine:
$ git diff
diff --git a/receiver_test.go b/receiver_test.go
index 8c662b0..fa3130c 100644
--- a/receiver_test.go
+++ b/receiver_test.go
@@ -149,13 +149,6 @@ func TestReceiverDefine(t *testing.T) {
}
}
- c.Destroy()
-}
-
-func TestReceiverDestroy(t *testing.T) {
- c, _ := e.NewContext()
- defer c.Destroy()
-
r := e.receivers["TestReceiver"]
if r == nil {
t.Fatalf("Receiver.Destroy(): Could not find defined receiver")
@@ -166,8 +159,7 @@ func TestReceiverDestroy(t *testing.T) {
t.Errorf("Receiver.Destroy(): Did not set internal fields to `nil`")
}
- // Attempting to destroy a receiver twice should be a no-op.
- r.Destroy()
+ c.Destroy()
}
func TestReceiverEnd(t *testing.T) {
GDB isn't super helpful as I'm not using a PHP7 library with debug enabled, but here it is anyway:
Thread 1 "tests" received signal SIGSEGV, Segmentation fault.
tcache_get (tc_idx=1) at malloc.c:2943
2943 malloc.c: No such file or directory.
(gdb) bt
#0 tcache_get (tc_idx=1) at malloc.c:2943
#1 __GI___libc_malloc (bytes=32) at malloc.c:3050
#2 0x00007ffffefba139 in __zend_malloc () from /usr/lib/libphp7.so
#3 0x00007fffff01202a in zend_interned_strings_init () from /usr/lib/libphp7.so
#4 0x00007ffffefe5771 in ?? () from /usr/lib/libphp7.so
#5 0x00007ffffef80d1a in php_module_startup () from /usr/lib/libphp7.so
#6 0x00000000006a0d38 in engine_init () at engine.c:123
#7 0x00000000006a0358 in _cgo_16684e298d85_C2func_engine_init (v=0xc420031720) at cgo-gcc-prolog:61
#8 0x000000000045c520 in runtime.asmcgocall () at /usr/lib/go-1.10/src/runtime/asm_amd64.s:688
#9 0x0000000000000002 in ?? ()
#10 0x0000000000000000 in ?? ()
Looks like php_request_shutdown()
, which is called from Context.Destroy, is causing some parts of what Engine.Define is creating to be removed again, see https://github.com/php/php-src/blob/master/main/main.c#L1952-L1956 (which even includes a cautionary comment 😜)
/* 15. Free Willy (here be crashes) */
zend_interned_strings_deactivate();
My educated guess is that https://github.com/php/php-src/blob/master/Zend/zend_API.c#L2726, which is what is called from do_register_internal_class()
, is causing this:
lowercase_name = zend_new_interned_string(lowercase_name);
Maybe this part of the PHP / Zend API wasn't mean to be used that way?
Another observation: Calling Engine.Define before a context has been created also causes a segfault:
package main
import (
"fmt"
php "github.com/deuill/go-php"
"os"
)
type Watcher struct {
Path string
}
func NewWatcher(args []interface{}) interface{} {
if len(args) > 0 {
if path, ok := args[0].(string); ok {
return &Watcher{path}
}
}
return nil
}
func main() {
engine, err := php.New()
if err != nil {
fmt.Printf("Could not create a new engine: %v\n", err)
return
}
defer engine.Destroy()
// Move the next two lines *below* the context creation to make it work
engine.Define("Watcher", NewWatcher)
fmt.Printf("Engine %+v\n", engine)
context, err := engine.NewContext()
if err != nil {
fmt.Printf("Could not create a new context: %v\n", err)
return
}
defer context.Destroy()
context.Output = os.Stdout
context.Eval("var_dump(new Watcher('Test'));")
}
GDB:
Thread 1 "create" received signal SIGSEGV, Segmentation fault.
0x00007fffff011e5a in ?? () from /usr/lib/libphp7.so
(gdb) bt
#0 0x00007fffff011e5a in ?? () from /usr/lib/libphp7.so
#1 0x0000000000648932 in receiver_define (name=0xc23ed0 "Watcher") at receiver.c:163
#2 0x0000000000457bc0 in runtime.asmcgocall () at /usr/lib/go-1.10/src/runtime/asm_amd64.s:688
#3 0x00007ffffffde600 in ?? ()
#4 0x0000000000455821 in runtime.exitsyscallfast.func1 () at /usr/lib/go-1.10/src/runtime/proc.go:3039
#5 0x00000000004563e9 in runtime.systemstack () at /usr/lib/go-1.10/src/runtime/asm_amd64.s:409
#6 0x0000000000431d60 in ?? () at /usr/lib/go-1.10/src/runtime/proc.go:1092
#7 0x0000000000000000 in ?? ()
The code at receiver.c:163 is:
INIT_CLASS_ENTRY_EX(tmp, name, strlen(name), NULL);
...which expands to https://github.com/php/php-src/blob/master/Zend/zend_API.h#L188-L193:
#define INIT_CLASS_ENTRY_EX(class_container, class_name, class_name_len, functions) \
{ \
memset(&class_container, 0, sizeof(zend_class_entry)); \
class_container.name = zend_string_init_interned(class_name, class_name_len, 1); \
class_container.info.internal.builtin_functions = functions; \
}
Looks like class entries are not meant to be created before php_request_startup(); and not meant to be used after php_request_shutdown()
From my point of view, this leaves us with the following options:
Define()
to Context; make Engine.Define raise a deprecation error. This will result in quite a bit of code being shifted around.Good insight, thanks!
I'll delve a bit deeper in these crashes, since the code around method receiver bindings is somewhat terrible. From what I can tell, php_request_shutdown
destroys the interned_strings
compiler global (shudder):
ZEND_API void zend_interned_strings_deactivate(void)
{
zend_hash_destroy(&CG(interned_strings));
}
Initializing a class defines a couple of interned strings, including the class name itself:
#define INIT_OVERLOADED_CLASS_ENTRY_EX(class_container, class_name, class_name_len, functions, handle_fcall, handle_propget, handle_propset, handle_propunset, handle_propisset) \
{ \
zend_string *cl_name; \
cl_name = zend_string_init(class_name, class_name_len, 1); \
class_container.name = zend_new_interned_string(cl_name); \
INIT_CLASS_ENTRY_INIT_METHODS(class_container, functions, handle_fcall, handle_propget, handle_propset, handle_propunset, handle_propisset) \
}
As well as, as you mentioned, the name used for registering the class against the compiler globals:
static zend_class_entry *do_register_internal_class(zend_class_entry *orig_class_entry, uint32_t ce_flags) /* {{{ */
{
zend_class_entry *class_entry = malloc(sizeof(zend_class_entry));
zend_string *lowercase_name = zend_string_alloc(ZSTR_LEN(orig_class_entry->name), 1);
...
zend_str_tolower_copy(ZSTR_VAL(lowercase_name), ZSTR_VAL(orig_class_entry->name), ZSTR_LEN(class_entry->name));
lowercase_name = zend_new_interned_string(lowercase_name);
zend_hash_update_ptr(CG(class_table), lowercase_name, class_entry);
zend_string_release(lowercase_name);
return class_entry;
}
The, zend_new_interned_string
defaults to setting strings into a separate HashTable
:
ZEND_API void zend_interned_strings_init(void)
{
...
zend_init_interned_strings_ht(&interned_strings_permanent, 1);
zend_new_interned_string = zend_new_interned_string_permanent;
...
}
However, the handler switches during php_module_startup
to use a different function:
ZEND_API void zend_interned_strings_switch_storage(void)
{
if (interned_string_copy_storage) {
interned_string_copy_storage();
}
zend_new_interned_string = interned_string_request_handler;
}
static zend_string *zend_new_interned_string_request(zend_string *str)
{
zend_string *ret;
if (ZSTR_IS_INTERNED(str)) {
return str;
}
/* Check for permanent strings, the table is readonly at this point. */
ret = zend_interned_string_ht_lookup(str, &interned_strings_permanent);
if (ret) {
zend_string_release(str);
return ret;
}
ret = zend_interned_string_ht_lookup(str, &CG(interned_strings));
if (ret) {
zend_string_release(str);
return ret;
}
/* Create a short living interned, freed after the request. */
ret = zend_add_interned_string(str, &CG(interned_strings), 0);
return ret;
}
Sigh...
The Define
function is defined at the engine
level, because class entries are set at the compiler globals level, and would imply that contexts would be able to override one another's class entries, which would make moving Define
to context
misleading.
However, I now believe this may be due to my misunderstanding on how PHP's process model operates when not built with thread/ZTS support:
So most things operate at the RINIT level anyways, since different requests aren't supposed to run concurrently.
I'll run some tests and see about perhaps moving Define
(which is a somewhat terrible name) to Context
if need be.
This happens when running tests:
Some research shows that is has to do with accessing receivers after a context is destroyed.