GNOME Bugzilla – Bug 664043
Add-data-based-content-type-guessing-for-win32.patch
Last modified: 2018-05-24 13:31:11 UTC
The windows port of glib currently lacks content type guessing from file data. This patch will add it. It does not link in urlmon.dll directly because the mingw32 compiler does not provide an urlmon library to link against afaik. Instead it uses LoadLibrary() for dynamic loading.
Created attachment 201373 [details] [review] 0001-Add-data-based-content-type-guessing-for-win32.patch
The initialization (the LoadLibrary stuff) is not threadsafe, it needs some g_once action. Also, why are you copying the data before passing it to FindMimeFromData?
Created attachment 201853 [details] [review] 0001-Add-data-based-content-type-guessing-for-win32.patch (In reply to comment #2) > The initialization (the LoadLibrary stuff) is not threadsafe, it needs some > g_once action. Is this new patch better? > Also, why are you copying the data before passing it to FindMimeFromData? Because FindMimeFromData needs a (void *) data, not a (const void *) or (const gchar *) data. I do not know why it needs this data to be non-const, but just passing in a (const gchar *) causes a segfault.
Created attachment 201928 [details] [review] 0001-Add-data-based-content-type-guessing-for-win32.patch Updated patch to check for NULL return value.
Review of attachment 201928 [details] [review]: ::: gio/gcontenttype.c @@ +355,3 @@ + { + static gsize initialized = FALSE; + static t_FindMimeFromData p_FindMimeFromData = NULL; You can't use a separate static that is not the one used in the g_once.., then there is nothing that protects it. Use the gsize (an int that fits a pointer) to store the actual function pointer, and use a separate value, like e.g. 1 to mark that initialization failed (so it won't be run again). @@ +364,3 @@ + if (urlmon != NULL) + p_FindMimeFromData = (t_FindMimeFromData) GetProcAddress ( + urlmon, "FindMimeFromData"); There is some possibility to allow glib to run arbitrary code here, by placing a urlmon.dll in the PATH. We should probably do something similar to: http://git.gnome.org/browse/gtk+/commit?id=88f54ea47d4a55bbbf9e34a7a0502f365eb69ae5 to avoid this. @@ +374,3 @@ + + LPVOID pBuffer = (LPVOID) g_malloc (data_size); + memcpy (pBuffer, data, data_size); This really should not be necessary. (void *) or (const void *) are identical in the resulting binary, its purely a language construct. If it did crash we need to figure out why, not just work around it by adding inefficiencies.
Created attachment 202988 [details] [review] 0001-Add-data-based-content-type-guessing-for-win32.patch New patch taking into account Alex's remarks. (In reply to comment #5) > Review of attachment 201928 [details] [review]: > > ::: gio/gcontenttype.c > @@ +355,3 @@ > + { > + static gsize initialized = FALSE; > + static t_FindMimeFromData p_FindMimeFromData = NULL; > > You can't use a separate static that is not the one used in the g_once.., then > there is nothing that protects it. Use the gsize (an int that fits a pointer) > to store the actual function pointer, and use a separate value, like e.g. 1 to > mark that initialization failed (so it won't be run again). In the new patch I use g_once () instead of g_once_init_enter () in the hope that this will work out right in a multi-threaded setting. As I am very new to g_once... and multi-threading, please review carefully again. > @@ +364,3 @@ > + if (urlmon != NULL) > + p_FindMimeFromData = (t_FindMimeFromData) GetProcAddress ( > + urlmon, "FindMimeFromData"); > > There is some possibility to allow glib to run arbitrary code here, by placing > a urlmon.dll in the PATH. We should probably do something similar to: > http://git.gnome.org/browse/gtk+/commit?id=88f54ea47d4a55bbbf9e34a7a0502f365eb69ae5 > to avoid this. I created a new function _g_win32_load_system_library_W () for this. Maybe this fuction could be placed in a more general location so that other code can benefit from it as well? > > @@ +374,3 @@ > + > + LPVOID pBuffer = (LPVOID) g_malloc (data_size); > + memcpy (pBuffer, data, data_size); > > This really should not be necessary. (void *) or (const void *) are identical > in the resulting binary, its purely a language construct. If it did crash we > need to figure out why, not just work around it by adding inefficiencies. According to (a user comment on) http://msdn.microsoft.com/en-us/library/ms775107%28v=vs.85%29.aspx the pBuffer must be writable or else FindMimeFromData() will fail/crash. That is exactly what I experienced in testing. So really, it seems to be necessary. But the same user comment also states that only a maximum of 256 bytes will be used from pBuffer, so I adjusted the code to only copy the first 256 bytes.
Created attachment 235298 [details] [review] 0001-Add-data-based-content-type-guessing-for-win32.patch Patch updated to current master (2.35.7)
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/477.