New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix crash when loading external plug-ins into Windows executables that statically link HTSlib #966
Conversation
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 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. |
One other thing, I had to remove |
libcurl with http works; https and ftp don't for some reason. |
Autoconf now checks that the compiler understands |
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 Now that the configure script checks for |
It turns out that The TODO comment was indeed left to signal that we should really work out which platforms really need |
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 |
By popular request, I've restricted this work-around to Windows builds. |
d87fa0c
to
89ef0d2
Compare
plugin.c
Outdated
#if defined(_WIN32) || defined(__MSYS__) | ||
const char *colon = strchr(itr->pathdir, ';'); | ||
#else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
htslib/hts_defs.h
Outdated
#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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' };
I've moved the |
if (!schemes) { | ||
if (try_exe_add_scheme_handler(scheme, handler) != 0) { | ||
hts_log_warning("Couldn't register scheme handler for %s", scheme); | ||
} | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 findhfile_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:
ctype
functions withchar
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