Segfaut in test suite

[remicollet]

Probably since recent changes in libvips

Using PHP 7.4, 8.0 with libvips 8.11.3

TEST 12/33 [tests/012.phpt]
========DIFF========
002+ Termsig=11
========DONE========
FAIL new_from_buffer works [tests/012.phpt] 

TEST 33/33 [tests/042.phpt]
========DIFF========
004+ 
005+ Termsig=11
========DONE========
FAIL can set metadata [tests/042.phpt] 
========================================

[remicollet]

(gdb) bt
#0  0x00007ffff7fa0ec0 in  ()
#1  0x00007ffff04327de in vips_area_free () at /lib64/libvips.so.42
#2  0x00007ffff0439226 in vips_area_unref () at /lib64/libvips.so.42
#3  0x00007ffff7571ed8 in g_value_unset () at /lib64/libgobject-2.0.so.0
#4  0x00007ffff0448584 in meta_free () at /lib64/libvips.so.42
#5  0x00007ffff7444442 in g_hash_table_remove_all_nodes.part () at /lib64/libglib-2.0.so.0
#6  0x00007ffff74450a3 in g_hash_table_remove_all () at /lib64/libglib-2.0.so.0
#7  0x00007ffff7448822 in g_hash_table_destroy () at /lib64/libglib-2.0.so.0
#8  0x00007ffff0448ad9 in vips.meta_destroy () at /lib64/libvips.so.42
#9  0x00007ffff0441900 in vips_image_finalize () at /lib64/libvips.so.42
#10 0x00007ffff7558a70 in g_object_unref () at /lib64/libgobject-2.0.so.0
#11 0x00007ffff043aaf8 in vips.object_set_member () at /lib64/libvips.so.42
#12 0x00007ffff043adb0 in vips_object_set_property () at /lib64/libvips.so.42
#13 0x00007ffff755a5b6 in object_set_property () at /lib64/libgobject-2.0.so.0
#14 0x00007ffff755c789 in g_object_set_valist () at /lib64/libgobject-2.0.so.0
#15 0x00007ffff755ca44 in g_object_set () at /lib64/libgobject-2.0.so.0
#16 0x00007ffff0431b85 in vips_object_dispose_argument () at /lib64/libvips.so.42
#17 0x00007ffff04350a3 in vips_argument_map () at /lib64/libvips.so.42
#18 0x00007ffff043512b in vips_object_dispose () at /lib64/libvips.so.42
#19 0x00007ffff75589e8 in g_object_unref () at /lib64/libgobject-2.0.so.0
#20 0x00007ffff044017f in vips_cache_remove () at /lib64/libvips.so.42
#21 0x00007ffff044627d in vips_cache_drop_all () at /lib64/libvips.so.42
#22 0x00007ffff046091a in vips_shutdown () at /lib64/libvips.so.42
#23 0x00007ffff7692247 in __run_exit_handlers () at /lib64/libc.so.6
#24 0x00007ffff76923f0 in on_exit () at /lib64/libc.so.6
#25 0x000055555563c261 in main (argc=68, argv=0x555555e21230) at /usr/src/debug/php-7.4.23-1.fc33.remi.x86_64/sapi/cli/php_cli.c:1398

[kleisauke]

I was able to reproduce this. It only seems to happen when one or more loadable modules are installed. See for example:

$ sudo dnf --setopt=install_weak_deps=False install vips-devel
$ make test > test.log 2>&1 && echo "Succeed" || echo "Failed"
Succeed
$ sudo dnf install vips-jxl vips-magick-im6 vips-openslide vips-poppler
$ make test > test.log 2>&1 && echo "Succeed" || echo "Failed"
Failed
$ grep "FAILED TEST SUMMARY" -B1 -A4 test.log
=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
new_from_buffer works [tests/012.phpt]
can set metadata [tests/042.phpt]
=====================================================================

(Tested on Fedora 34)

Perhaps the loadable module is somehow unloaded prior to vips_shutdown? Though, the g_module_make_resident call should ensure that it never gets unloaded.
https://github.com/libvips/libvips/blob/4d079f169103f45c7d866d49a54598be0278e7d6/libvips/module/jxl.c#L75-L77

Let me investigate this further.

[kleisauke]

It seems that the test suite passes when the ZEND_DONT_UNLOAD_MODULES=1 environment variable is set.

$ ZEND_DONT_UNLOAD_MODULES=1 make test

So, the PHP vips extension (vips.so) will probably need to link with -Wl,-z,nodelete. I also had a go with this patch for libvips:

z-nodelete.patch

diff --git a/configure.ac b/configure.ac
index 1111111..2222222 100644
--- a/configure.ac
+++ b/configure.ac
@@ -466,6 +466,20 @@ else
   fi
 fi
 
+SAVE_LDFLAGS="$LDFLAGS"
+LDFLAGS="$LDFLAGS -Wl,-z,nodelete"
+AC_MSG_CHECKING([whether linker understands -z nodelete])
+AC_LINK_IFELSE([AC_LANG_PROGRAM([], [])],
+  [
+    AC_MSG_RESULT([yes])
+    LDFLAGS_Z_NODELETE="-Wl,-z,nodelete"
+  ],[
+    AC_MSG_RESULT([no])
+    LDFLAGS_Z_NODELETE=""
+  ])
+LDFLAGS="$SAVE_LDFLAGS"
+AC_SUBST(LDFLAGS_Z_NODELETE)
+
 # check for gtk-doc
 GTK_DOC_CHECK([1.14],[--flavour no-tmpl])
 
diff --git a/libvips/Makefile.am b/libvips/Makefile.am
index 1111111..2222222 100644
--- a/libvips/Makefile.am
+++ b/libvips/Makefile.am
@@ -58,6 +58,7 @@ libvips_la_LIBADD = \
 	@VIPS_LIBS@
 
 libvips_la_LDFLAGS = \
+	$(LDFLAGS_Z_NODELETE) \
 	-no-undefined \
 	-version-info @LIBRARY_CURRENT@:@LIBRARY_REVISION@:@LIBRARY_AGE@ 
 
@@ -107,6 +108,7 @@ MODULE_CPPFLAGS = \
 	$(REQUIRED_CFLAGS)
 
 MODULE_LDFLAGS = \
+	$(LDFLAGS_Z_NODELETE) \
 	-no-undefined \
 	-shared \
 	-module \

But that resulted in the same segfault.

[kleisauke]

Linking the extension with -Wl,-z,nodelete could possibly also result that this code is no longer necessary:

php-vips-ext/vips.c

Lines 2010 to 2043 in 310b029

	if (strcmp(sapi_module.name, "apache2handler") == 0) {
	/* "apachectl graceful" can cause us terrible problems. What happens:
	*
	* - the main apache process unloads this extension, vips.so
	* - in turn, the C runtime will unload libvips.so, the vips library,
	* since vips.so is the only thing that references it
	* - libvips.so in turn uses glib.so, but this is often not unloaded,
	* since other parts of apache can be using it (glib could also
	* possibly be preventing unload itself, I'm not sure)
	* - the main apache process then reloads vips.so, which in turn will
	* reload libvips.so as it starts up
	* - vips.so tries to init libvips.so
	* - libvips.so tries to register its types (such as VipsImage) with
	* glib.so, but finds the types from the previous init still there
	* - everything breaks
	*
	* A simple fix that will always work is just to lock libvips in
	* memory and prevent unload. We intentionally leak refs to the shared
	* library.
	*
	* We include the binary API version number that this extension needs.
	* We can't just load .so, that's only installed with libvips-dev,
	* which may not be present at runtime.
	*/
	#ifdef VIPS_SONAME
	if (!dlopen(VIPS_SONAME, RTLD_LAZY | RTLD_NODELETE))
	#else /*!VIPS_SONAME*/
	if (!dlopen("libvips.so.42", RTLD_LAZY | RTLD_NODELETE))
	#endif /*VIPS_SONAME*/
	{
	sapi_module.sapi_error(E_WARNING, "php-vips-ext: unable to lock "
	"libvips -- graceful may be unreliable");
	}
	}

(untested)

[remicollet]

Above hack is only for mod_php, and the segfault occurs with cli sapi

Also notice that it may not work, as in various distribution, PHP use RTLD_NOW (build using --enable-rtld-now for security / safety reasons)

[remicollet]

I confirm, building the ext with -Wl,-z,nodelete avoid the segfault

[kleisauke]

Great, thanks for confirming. Commit kleisauke@f08dc82 should ensure that the extension is always linked with -Wl,-z,nodelete. I'll open a PR for that soon.

In that commit, I've also removed the hack mentioned above, as I think it's no longer needed. However, I couldn't reproduce issue libvips/php-vips#26 with version 1.0.2 of the extension, so I'm not sure if it's completely safe to remove that (maybe it was only needed for older Apache versions?).

[kleisauke]

BTW, does test 029.phpt still need to be investigated? I noticed that it is excluded in the spec file, see:
https://git.remirepo.net/cgit/rpms/php/pecl/php-pecl-vips.git/tree/php-pecl-vips.spec?id=e9d2156a981132b47f4f01c852182e54e058831f#n180

Perhaps the vips_error_buffer contained more errors than expected. If so, we may need to apply the following patch:

diff --git a/tests/029.phpt b/tests/029.phpt
index 1111111..2222222 100644
--- a/tests/029.phpt
+++ b/tests/029.phpt
@@ -14,7 +14,7 @@ can get error messages
   $msg = vips_error_buffer();
 
   if ($err == -1 &&
-    $msg == "add: not one band or 3 bands\n") {
+    strpos($msg, "add: not one band or 3 bands\n") !== false) {
     echo "pass";
   }
 ?>

[kleisauke]

A (draft-)pull request has been made for this at #44.