Ensure extension is linked with -Wl,-z,nodelete

[kleisauke]

As discussed in #43, this ensures that the extension is linked with -Wl,-z,nodelete. An advantage of doing this is that the apachectl graceful workaround introduced in commit d5e34e8 and ed9f424 is probably no longer needed.

The removal of the apachectl graceful workaround was verified with this Dockerfile (since I couldn't reproduce that behavior locally with version 1.0.2 of the extension).

Details

FROM ubuntu:bionic

ENV DEBIAN_FRONTEND=noninteractive
ENV NO_INTERACTION=1

ENV APACHE_RUN_USER=www-data
ENV APACHE_RUN_GROUP=www-data
ENV APACHE_LOG_DIR=/var/log/apache2

RUN apt-get update \
    && apt-get upgrade -y \
    && apt-get install -y \
        git \
        build-essential \
        unzip \
        wget \
        pkg-config

# stuff we need to build our own libvips
# glib and expat are the only required ones, the others are optional and
# enable features like jpeg load etc.
# you'll probably want to custiomise this list
RUN apt-get install -y \
    libexif-dev \
    libexpat-dev \
    libglib2.0-dev \
    libjpeg-dev \
    libtiff-dev \
    liblcms2-dev \
    liborc-dev \
    libpng-dev \
    libpoppler-glib-dev \
    librsvg2-dev

WORKDIR /usr/local/src

ARG VIPS_URL=https://github.com/libvips/libvips/releases/download
ARG VIPS_VERSION=8.11.3

RUN wget $VIPS_URL/v$VIPS_VERSION/vips-$VIPS_VERSION.tar.gz \
    && tar xf vips-$VIPS_VERSION.tar.gz \
    && cd vips-$VIPS_VERSION \
    && ./configure --prefix=/usr/local --disable-static \
    && make V=0 \
    && make install

# php layers
RUN apt-get install -y \
    php-dev \
    php-pear

# web stuff
RUN apt-get install -y \
    apache2 \
    libapache2-mod-php

# install the latest version of the php vips extension
# (to verify `apachectl graceful` is functional)
#RUN pecl install vips

# install version 1.0.2 of the php vips extension
# (to verify `apachectl graceful` is broken)
#RUN pecl install vips-1.0.2

# install the php vips extension from source
# (to verify `apachectl graceful` is still functional)
RUN git clone https://github.com/kleisauke/php-vips-ext.git --branch z_nodelete --single-branch \
    && cd php-vips-ext \
    && phpize \
    && ./configure \
    && make \
    && make test \
    && make install

# enable the vips.so extension for cli
RUN echo "extension=vips.so" > /etc/php/7.2/mods-available/vips.ini \
    && ln -s /etc/php/7.2/mods-available/vips.ini \
        /etc/php/7.2/cli/conf.d/20-vips.ini

# enable the vips.so extension for apache
RUN ln -s /etc/php/7.2/mods-available/vips.ini \
    /etc/php/7.2/apache2/conf.d/20-vips.ini

EXPOSE 80

WORKDIR /var/www/html

CMD ["/usr/sbin/apache2ctl", "-D", "FOREGROUND"]

$ docker pull ubuntu:bionic
$ docker build -t php-vips-ext .
$ docker run -d -p 8080:80 --name=php-vips-ext-apache2 php-vips-ext
$ docker exec -it php-vips-ext-apache2 /bin/bash
$ wget https://images.weserv.nl/zebra.jpg
$ cat <<'EOF' > index.php
<?php

    echo "Hello world!<br>\n";

    $filename = "zebra.jpg";

    $x = vips_image_new_from_file($filename)["out"];
    if(!$x) {
        echo "failed to open image<br>\n";
        echo "vips error log is " . vips_error_buffer() . "\n<br>";
    }
    else {
        $width = vips_image_get($x, "width")["out"];

        echo "image " . $filename . " is " . $width . " pixels across\n";
    }
?>
EOF
$ rm index.html
# Visit http://localhost:8080/index.php
# (should print: `image zebra.jpg is 4120 pixels across`)

$ cat /var/log/apache2/error.log
$ apachectl graceful
$ cat /var/log/apache2/error.log
$ ps aux | grep apache

# Visit http://localhost:8080/index.php again

Note that I'm not sure if linking with -Wl,-z,nodelete may cause undesirable side effects (it's probably fine since the whole of GLib is linked with this flag).

Marked this PR as a draft due to these TODO-items:

• Was the apachectl graceful workaround only for Linux? If not, we may need to verify whether this PR also works on macOS.
• Should we re-introduce the call to vips_shutdown in PHP_MSHUTDOWN_FUNCTION?

[jcupitt]

Wow I'd never heard of nodelete. Sure, this sounds like a good solution.

This comment talks about the scenario which caused graceful to fail:

libvips/php-vips#26 (comment)

So it sounds like locking libvips in memory ought to fix it, since it will no longer restart without restarting glib. However, I've no idea what platforms like BSD and macOS do with nodelete.

Perhaps the problem is that libvips and the vips.so extension are not using the same linker flags as glib? It's because they differ that we are seeing one library restarting and not the other. Maybe glib-2.0.pc needs some mechanism to get link flags that match glib's?

I'd think vips_shutdown() wasn't necessary since we can just rely on process termination.

[kleisauke]

However, I've no idea what platforms like BSD and macOS do with nodelete.

I did a quick search, but it seems that on macOS nodelete isn't supported. This PR matches GLib behavior with adding this linker flag only if it's supported, so hopefully GLib will just do a proper unload and reload on systems where nodelete isn't supported at all. It might be wise to check this, but I don't have a macOS system handy.

Perhaps the problem is that libvips and the vips.so extension are not using the same linker flags as glib? It's because they differ that we are seeing one library restarting and not the other. Maybe glib-2.0.pc needs some mechanism to get link flags that match glib's?

I think the non-matching linker flags could be the underlying issue. Unfortunally, the -Wl,-z,nodelete linker flag is used internally in GLib, and not populated in any of the pkg-config files. If, for example, glib-2.0.pc would include this flag, this issue would probably not exist (but that would also imply that it would be exposed to all GLib dependencies, which might not be desired).

[jcupitt]

I have a macOS machine for testing, I'll test this.

Yes, it's complex issue. You'd think other libraries which use glib would have been bitten by this. I'll ask the glib maintainers if they have an opinion.

[jcupitt]

I opened an issue on glib: https://gitlab.gnome.org/GNOME/glib/-/issues/2483

[jcupitt]

... moved to discourse https://discourse.gnome.org/t/libraries-that-use-glib-sometimes-need-to-use-glibs-link-flags-whats-the-best-way-to-achieve-this/7487

[remicollet]

What is strange (to me) is that the discussion is about the mod_php case.

But the segfault also happens in CLI (running the test suite), with a single process...

$ /usr/bin/php  -n -c '/work/GIT/pecl-and-ext/vips/tmp-php.ini'  -d "output_handler=" -d "open_basedir=" -d "disable_functions=" -d "output_buffering=Off" -d "error_reporting=32767" -d "display_errors=1" -d "display_startup_errors=1" -d "log_errors=0" -d "html_errors=0" -d "track_errors=0" -d "report_memleaks=1" -d "report_zend_debug=0" -d "docref_root=" -d "docref_ext=.html" -d "error_prepend_string=" -d "error_append_string=" -d "auto_prepend_file=" -d "auto_append_file=" -d "ignore_repeated_errors=0" -d "precision=14" -d "memory_limit=128M" -d "log_errors_max_len=0" -d "opcache.fast_shutdown=0" -d "opcache.file_update_protection=0" -d "opcache.revalidate_freq=0" -d "zend.assertions=1" -d "zend.exception_ignore_args=0" -d "extension_dir=/work/GIT/pecl-and-ext/vips/modules/" -d "extension=vips.so" -d "session.auto_start=0" -d "zlib.output_compression=Off" -f "/work/GIT/pecl-and-ext/vips/tests/042.php"
pass set_metadata
pass reload
pass get_metadata
Erreur de segmentation (core dumped)

[remicollet]

Should we re-introduce the call to vips_shutdown in PHP_MSHUTDOWN_FUNCTION?

Yes, this solves the issue in CLI mode (cleanup occurs before the library is unloaded)

[remicollet]

I think the issue is related to atexit( vips_shutdown ); call (which raise the segfault), this happens when the library is unloaded, and nothing ensure unload order...

(gdb) bt
...
#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

[jcupitt]

I suppose we also need to link libvips.so itself with nodelete, is that right?

The conclusion from the glib devs seems to be that we should copy-paste the glib linker logic from their meson.build.

I think the atexit() crash will go if we call vips_shutdown() from PHP_MSHUTDOWN_FUNCTION.

[kleisauke]

The curious thing is that I saw the same segfault when libvips.so was linked with nodelete, see for example the patch mentioned in #43 (comment).

Perhaps I didn't test carefully, let me check it again.

[kleisauke]

I can confirm that (on the master branch of php-vips-ext):

• When linking libvips.so with nodelete (using the above mentioned patch) the segfault is still present.
• Re-introducing the call to vips_shutdown in PHP_MSHUTDOWN_FUNCTION no longer produces a segfault.

I also confirm that with this PR the segfault is no longer present with or without applying the above changes.

[remicollet]

I suppose we also need to link libvips.so itself with nodelete, is that right?

No

I think the atexit() crash will go if we call vips_shutdown() from PHP_MSHUTDOWN_FUNCTION.

Yes

[kleisauke]

This workaround isn't necessary on macOS, and specific to ELF libraries only. Although VIPS_SONAME is set correctly on macOS.

$ podman create --arch=amd64 --os=darwin -ti --name dummy ghcr.io/homebrew/core/vips:8.12.0 sh
$ podman export dummy > homebrew-vips.tar
$ podman rm -f dummy
$ tar -xf homebrew-vips.tar vips/8.12.0/include/vips/soname.h --strip-components 4
$ cat soname.h
/* This file is autogenerated, do not edit. */
#define VIPS_SONAME "libvips.42.dylib"

RTLD_NODELETE and the -Wl,-z,nodelete linker flag (which is added only if it's supported) is no-op there. Moreover, the Objective-C runtime also prevents it from being unloaded by default, see: https://stackoverflow.com/a/8793766.

This PR is ready for reviewing now. Note that issue #43 is already resolved within libvips 8.12, since we no longer call vips_shutdown during atexit, see commit libvips/libvips@af61d37.

[jcupitt]

OK, LGTM. Good job everyone for investigating and fixing this.