Fix crash when loading external plug-ins into Windows executables that statically link HTSlib by daviesrob · Pull Request #966 · samtools/htslib

daviesrob · 2019-11-06T17:55:38Z

Works around the plug-in's insistence on linking against and using the shared hts.dll by spotting when this causes a problem and using dlopen() / dlsym() to find hfile_add_scheme_handler() in the executable and call that instead. This ensures the plug-in is registered in the correct place. It's not elegant but it works.

Additional bonus fixes:

A missing dependencies on hts.dll.a which could result in the plug-in dlls trying to build before the shared library was available.
Some char-subscripts warnings due to use of ctype functions with char types. These don't appear to trigger warnings in our CI builds (including Appveyor) but did when I made a MinGW build by hand.

Fixes #964

jkbonfield · 2019-11-07T11:57:06Z

I can confirm using mingw64 that schemes is still NULL when trying to add the handler, causing the crash. However I can also confirm that this does not fix it.

Infact it makes things worse. With PLUGIN_PATH set to an inappropriate location it can read ftp:// (falling back to knet). With PLUGIN_PATH set correctly it can't read ftp. It no longer just crashes as before, but it also cannot read the files.

Edit: debugging this further, it finds the libcurl plugin handler for the ftp URI, but ultimately fails because the select in wait_perform fails. It returns with val < 0, but doesn't set errno. The reason is unknown.

IMO we should make the release and just ignore windows plugins this time. It's not as if they worked in the previous umpteen releases either so support this can come later.

jkbonfield · 2019-11-07T12:12:17Z

One other thing, I had to remove -rdynamic from the configure script to get --with-plugins to build on Windows. So that's also something to add on the to-fix list.

daviesrob · 2019-11-07T12:37:20Z

libcurl with http works; https and ftp don't for some reason.

daviesrob · 2019-11-07T22:56:30Z

Autoconf now checks that the compiler understands -rdynamic.

jmarshall · 2019-11-08T10:14:16Z

Not sure why the answer isn't “tell Windows people to build their executables in a consistent way” 😛 (and am curious why it didn't work for James) but if this wart is for the benefit of Windows only maybe it could be wrapped in #ifdef _WIN32.

Now that the configure script checks for -rdynamic support (yay), the TODO comment can go away. Or maybe you left it to signal further scope for tweaking this stuff..?

daviesrob · 2019-11-08T10:19:57Z

It turns out that #ifdef _WIN32 is only defined for mingw-w64-x86_64 builds. The MSYS one doesn't define it but still needs the hack to get the right symbol. As it should never do anything on UNIX-like systems it just seems easier to leave the guards out.

The TODO comment was indeed left to signal that we should really work out which platforms really need -rdynamic - but at least it's less likely to break things now.

jmarshall · 2019-11-08T10:52:46Z

I'm sure MSYS defines something, and it could all be encapsulated at the top of this file with its other platform warts:

--- a/hfile.c
+++ b/hfile.c
@@ -501,12 +501,16 @@ void hclose_abruptly(hFILE *fp)
 #ifndef _WIN32
 #include <sys/socket.h>
 #include <sys/stat.h>
 #define HAVE_STRUCT_STAT_ST_BLKSIZE
+#if defined __CYGWIN__ || defined __MSYS__
+#define NEED_DYNAMIC_LINKING_WORKAROUND
+#endif
 #else
 #include <winsock2.h>
 #define HAVE_CLOSESOCKET
 #define HAVE_SETMODE
+#define NEED_DYNAMIC_LINKING_WORKAROUND
 #endif
 #include <fcntl.h>
 #include <unistd.h>

and then #if defined ENABLE_PLUGINS && defined NEED_DYNAMIC_LINKING_WORKAROUND…
(or HAVE_BROKEN_DLLS if you prefer the HAVE_… pattern 😄)

daviesrob · 2019-11-08T15:18:43Z

By popular request, I've restricted this work-around to Windows builds.

jmarshall · 2019-11-13T15:23:05Z

plugin.c

+#if defined(_WIN32) || defined(__MSYS__)
+        const char *colon = strchr(itr->pathdir, ';');
+#else


Did you consider defining PATH_SEPARATOR as ':' or ';' as appropriate instead of sprinkling this guano everywhere and duplicating code? (Yes, you'll need PATH_SEPARATOR_STRING or other tricks as well.)

jmarshall · 2019-11-14T11:22:44Z

htslib/hts_defs.h

+#if defined(_WIN32) || defined(__MSYS__)
+#define HTS_PATH_SEPARATOR_CHAR ';'
+#define HTS_PATH_SEPARATOR_STR  ";"
+#else
+#define HTS_PATH_SEPARATOR_CHAR ':'
+#define HTS_PATH_SEPARATOR_STR  ":"
+#endif
+


This could be private to HTSlib, alongside the hts_path_itr stuff in hts_internal.h.

Or if it's decided to make it public, it would be better in hts_os.h or perhaps hts.h — but not really hts_defs.h which is about compiler capabilities.

The “other tricks” BTW is the following (though if made public it's probably best to provide both of them anyway rather than making users needing a string use this trick too):

static const char path_sep_string[] = { HTS_PATH_SEPARATOR_CHAR, '\0' };

daviesrob · 2019-11-15T10:42:01Z

I've moved the #defines to htslib/hts.h. I think it's useful to make them public, in case anything wants to find out easily which path separator is in use.

jmarshall · 2019-11-15T10:52:52Z

hfile.c

+    if (!schemes) {
+        if (try_exe_add_scheme_handler(scheme, handler) != 0) {
+            hts_log_warning("Couldn't register scheme handler for %s", scheme);
+        }
+        return;
+    }


Putting this inside #ifdef USING_WINDOWS_PLUGIN_DLLS too indicates that this test is irrelevant on Unix. (And then you don't need the #else and stub above.)

I thought of doing that, but decided that it would be better that in the (very unlikely) event of getting here with schemes == NULL on UNIX, it prints the message and bails out gracefully.

Change to pattern rules when building Windows plug-in dlls and add prerequisite for hts.dll.a / libhts.dll.a to MinGW / Cygwin rules. Ensures the shared library gets built before the plug-ins, in particular when doing parallel builds.

Windows dlls resolve symbols at link time (unlike UNIX shared objects which resolve at run time). This means Windows plug-in dlls are linked against and use the HTSlib shared library even if the executable that loads them statically links libhts.a. This caused plug-ins loaded by statically linked executables to crash when they called hfile_add_scheme_handler as the schemes khash in the dll had not been set up correctly (and even if it had been, the executable would still not be aware of the scheme handler). Try to fix by spotting when schemes is NULL, and if so try to find (via dlopen, dlsym) the executable's copy of hfile_add_scheme_handler and call that instead. If that doesn't work, print a warning and carry on, which is better than outright crashing.

Use the wrappers around ctype.h functions in textutils_internal.h Add a couple of extra wrappers (ispunct_c and isxdigit_c)

Fixes builds with --enable-plugins using the MSYS2 mingw-w64-x86_64 tool chain.

valeriuo force-pushed the win-fixes branch 2 times, most recently from d87fa0c to 89ef0d2 Compare November 13, 2019 15:14

jmarshall reviewed Nov 13, 2019

View reviewed changes

valeriuo force-pushed the win-fixes branch from 89ef0d2 to 2692b73 Compare November 14, 2019 10:51

jmarshall reviewed Nov 14, 2019

View reviewed changes

daviesrob force-pushed the win-fixes branch from 2692b73 to eb8b425 Compare November 15, 2019 10:39

jmarshall reviewed Nov 15, 2019

View reviewed changes

daviesrob and others added 5 commits November 19, 2019 12:29

Fix plug-in dll dependencies on Windows

6a73744

Change to pattern rules when building Windows plug-in dlls and add prerequisite for hts.dll.a / libhts.dll.a to MinGW / Cygwin rules. Ensures the shared library gets built before the plug-ins, in particular when doing parallel builds.

Fix char-subscripts warnings on MinGW build

016c09c

Use the wrappers around ctype.h functions in textutils_internal.h Add a couple of extra wrappers (ispunct_c and isxdigit_c)

In configure.ac, check compiler supports -rdynamic

618e13d

Fixes builds with --enable-plugins using the MSYS2 mingw-w64-x86_64 tool chain.

Allow for Windows style paths.

c673947

daviesrob force-pushed the win-fixes branch from eb8b425 to c673947 Compare November 19, 2019 12:30

whitwham merged commit c673947 into samtools:develop Nov 19, 2019

daviesrob deleted the win-fixes branch November 19, 2019 13:42

Conversation

daviesrob commented Nov 6, 2019

Uh oh!

jkbonfield commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkbonfield commented Nov 7, 2019

Uh oh!

daviesrob commented Nov 7, 2019

Uh oh!

daviesrob commented Nov 7, 2019

Uh oh!

jmarshall commented Nov 8, 2019

Uh oh!

daviesrob commented Nov 8, 2019

Uh oh!

jmarshall commented Nov 8, 2019

Uh oh!

daviesrob commented Nov 8, 2019

Uh oh!

jmarshall Nov 13, 2019

Choose a reason for hiding this comment

Uh oh!

jmarshall Nov 14, 2019

Choose a reason for hiding this comment

Uh oh!

jmarshall Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviesrob commented Nov 15, 2019

Uh oh!

jmarshall Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

daviesrob Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jkbonfield commented Nov 7, 2019 •

edited

Loading

jmarshall Nov 14, 2019 •

edited

Loading