Hide libmagic symbols by default on supported platforms

[orlitzky]

Trying my hand at a very old problem (PHP bug 66095, Gentoo bug 471682, FreeBSD bug 195562)

The bundled libmagic for the fileinfo extension has some visible symbols:

$ objdump -TC /usr/lib/php8.5/apache2/libphp8.so | grep '\smagic_'
00000000002b3f34 g    DF .text	0000000000000034  Base        magic_compile
00000000002b3dde g    DF .text	0000000000000024  Base        magic_open
00000000002b3fd0 g    DF .text	0000000000000034  Base        magic_descriptor
00000000002b3f00 g    DF .text	0000000000000034  Base        magic_load
00000000002b43fa g    DF .text	0000000000000032  Base        magic_setflags
00000000002b3f9c g    DF .text	0000000000000034  Base        magic_list
00000000002b4442 g    DF .text	0000000000000206  Base        magic_setparam
00000000002b43d4 g    DF .text	0000000000000026  Base        magic_getflags
00000000002b439e g    DF .text	0000000000000036  Base        magic_errno
00000000002b4648 g    DF .text	00000000000001f4  Base        magic_getparam
00000000002b42f4 g    DF .text	000000000000006e  Base        magic_buffer
00000000002b4362 g    DF .text	000000000000003c  Base        magic_error
00000000002b3f68 g    DF .text	0000000000000034  Base        magic_check
00000000002b442c g    DF .text	0000000000000016  Base        magic_version
00000000002b3eda g    DF .text	0000000000000026  Base        magic_close
00000000002b4038 g    DF .text	0000000000000034  Base        magic_stream
00000000002b4004 g    DF .text	0000000000000034  Base        magic_file

This works fine until you load that shared library into a process that links against the system libmagic, providing (in many cases, different versions of) the same symbols. I hit this myself several years ago when using another apache module that linked against libmagic. I can't cite any bug reports for the embed SAPI, but there is every reason to suspect that it suffers from the same issue.

As pointed out by @iluuu1994 below, hiding these symbols on platforms that support the visibility attribute is straightforward.

The strcasestr() commit is unrelated to an extent. I noticed (grep) that the only use of strcasestr() in libmagic is patched out by PHP, so there is no need for it on any platform. The -DHAVE_STRCASESTR=1 simply ensures that libmagic does not try to prototype it.

[orlitzky]

Linking of the embed SAPI looks borked on master, but if I hack these patches onto the PHP-8.5 branch, pointlessly linking the embed SAPI into a small libmagic example induces the problem:

/* main.c */
#include <magic.h>
#include <stdio.h>

int main(int argc, char **argv)
{
  char *actual_file = "example.pdf";
  const char *magic_full;
  magic_t magic_cookie;

  magic_cookie = magic_open(MAGIC_MIME);
  if (magic_cookie == NULL) {
    printf("unable to initialize magic library\n");
    return 1;
  }

  printf("Loading default magic database...\n");
  if (magic_load(magic_cookie, "/usr/share/misc/magic.mgc") != 0) {
    printf("cannot load magic database - %s\n", magic_error(magic_cookie));
    magic_close(magic_cookie);
    return 1;
  }

  magic_full = magic_file(magic_cookie, actual_file);
  printf("libmagic answer: %s\n", magic_full);
  magic_close(magic_cookie);
  return 0;
}

Before the PR (my system copy of php-8.5):

$ gcc $(/usr/lib/php8.5/bin/php-config --includes) main.c -L/usr/lib/php8.5/lib -lphp -lmagic
$ LD_LIBRARY_PATH=/usr/lib/php8.5/lib ./a.out 
Segmentation fault         LD_LIBRARY_PATH=/usr/lib/php8.5/lib ./a.out

And the one I just built and installed to ~/tmp/php:

$ gcc $(/home/mjo/tmp/php/bin/php-config --includes) main.c -L/home/mjo/tmp/php/lib -lphp -lmagic
$ LD_LIBRARY_PATH=/home/mjo/tmp/php/lib ./a.out 
Loading default magic database...
libmagic answer: application/pdf; charset=binary

[orlitzky]

(example.pdf is a random PDF on my machine)

[iluuu1994]

Would something like this not be enough?

diff --git a/ext/fileinfo/libmagic/file.h b/ext/fileinfo/libmagic/file.h
index f8fe5889500..443ca686319 100644
--- a/ext/fileinfo/libmagic/file.h
+++ b/ext/fileinfo/libmagic/file.h
@@ -102,12 +102,12 @@
 
 #if HAVE_VISIBILITY
 # if defined(WIN32)
-#  define file_public  __declspec(dllexport)
+#  define file_public
 #  ifndef file_protected
 #   define file_protected
 #  endif
 # else
-#  define file_public  __attribute__((__visibility__("default")))
+#  define file_public  __attribute__((__visibility__("hidden")))
 #  ifndef file_protected
 #   define file_protected __attribute__((__visibility__("hidden")))
 #  endif

FWIU these symbols don't need dynamic linkage.

[orlitzky]

Would something like this not be enough?

...

FWIU these symbols don't need dynamic linkage.

This approach does work but it's not quite that simple. We're not using libmagic's build system, so HAVE_VISIBILITY will be undefined. The PHP ./configure script already checks for -fvisibility=hidden support though -- apparently it was me who added it, ten years ago in 086f9adc4 -- so we could easily define the constant.

Afterwards, the prototypes in libmagic/magic.h need to be modified because they don't use the file_public macro, probably to avoid pulling file.h into the public API. It shouldn't be a problem to include it in the bundled copy though. Then we would just need to stick file_public in front of each magic_foo() function.

Around this point is where I stopped and tried the find/sed approach in the PR. It's arguably simpler (doesn't require configure.ac changes or hand edits) and works on systems without -fvisibility=hidden, but it is a little less clean since the php_magic_* symbols still wind up in libphp.so, even on systems where they could be hidden.

[orlitzky]

This approach does work but it's not quite that simple. We're not using libmagic's build system, so HAVE_VISIBILITY will be undefined. The PHP ./configure script already checks for -fvisibility=hidden support though...

Now that I think about it, the compiler accepting -fvisibility=hidden doesn't necessarily mean that it will support the inline attribute. Not too hard to check for in any case.

[iluuu1994]

We're not using libmagic's build system, so HAVE_VISIBILITY will be undefined.

Don't we already take care of this here?

https://github.com/php/php-src/blob/master/ext/fileinfo/libmagic/config.h

I did test this with a static ext-fileinfo build, and nm -D sapi/cli/php | grep magic_open disappears with the patch above.

[orlitzky]

We're not using libmagic's build system, so HAVE_VISIBILITY will be undefined.

Don't we already take care of this here?

https://github.com/php/php-src/blob/master/ext/fileinfo/libmagic/config.h

I did test this with a static ext-fileinfo build, and nm -D sapi/cli/php | grep magic_open disappears with the patch above.

You're right, I completely missed that. I think that tips the scale in favor of your approach. I'll redo it and force-push.

[petk]

Good find. Here would be then also good to remove the source file from Windows:

--- a/ext/fileinfo/config.w32
+++ b/ext/fileinfo/config.w32
@@ -8,7 +8,7 @@ if (PHP_FILEINFO != 'no') {
                        encoding.c fsmagic.c funcs.c \
                        is_json.c is_tar.c is_simh.c magic.c print.c \
                        readcdf.c softmagic.c der.c \
-                       strcasestr.c buffer.c is_csv.c";
+                       buffer.c is_csv.c";
 
        EXTENSION('fileinfo', 'fileinfo.c php_libmagic.c', true, "/I" + configure_module_dirname + "/libmagic /I" + configure_module_dirname);
        ADD_EXTENSION_DEP('fileinfo', 'pcre');

And can we then update the patch file instead of adding the HAVE_STRCASESTR to build configuration, so it's safer to not be redefined in any case.