GNOME Bugzilla – Bug 788388
TIFF loader doesn't work on Windows with Win32-IO libtiff
Last modified: 2018-02-02 10:32:08 UTC
When using libtiff with Win32-IO enabled (which is the default), TIFF loader fails to load anything, because it passes a C runtime FD to TIFFFdOpen(), while libtiff expects a HANDLE.
Created attachment 360713 [details] [review] io-tiff: Correctly work with WIN32-IO version of libtiff If libtiff is compiled with WIN32-IO (which is the default, unless it's built for Cygwin), it works with HANDLEs, not C runtime FDs.
Review of attachment 360713 [details] [review]: ::: gdk-pixbuf/io-tiff.c @@ +362,1 @@ if (!tiff) { "tiff" might be used unset here, if DuplicateHandle() fails.
(In reply to Bastien Nocera from comment #2) > Review of attachment 360713 [details] [review] [review]: > > ::: gdk-pixbuf/io-tiff.c > @@ +362,1 @@ > if (!tiff) { > > "tiff" might be used unset here, if DuplicateHandle() fails. I don't follow. Please elaborate.
(In reply to LRN from comment #3) > (In reply to Bastien Nocera from comment #2) > > Review of attachment 360713 [details] [review] [review] [review]: > > > > ::: gdk-pixbuf/io-tiff.c > > @@ +362,1 @@ > > if (!tiff) { > > > > "tiff" might be used unset here, if DuplicateHandle() fails. > > I don't follow. Please elaborate. Click on the review, and look at it in context. The "tiff" variable might be unset when you use it where my comment is because of your new code.
(In reply to Bastien Nocera from comment #4) > (In reply to LRN from comment #3) > > (In reply to Bastien Nocera from comment #2) > > > Review of attachment 360713 [details] [review] [review] [review] [review]: > > > > > > ::: gdk-pixbuf/io-tiff.c > > > @@ +362,1 @@ > > > if (!tiff) { > > > > > > "tiff" might be used unset here, if DuplicateHandle() fails. > > > > I don't follow. Please elaborate. > > Click on the review, and look at it in context. I know *that*. What i do not follow is how this can happen. Looking at the code, the "tiff" variable is never left unset.
(In reply to LRN from comment #5) > (In reply to Bastien Nocera from comment #4) > > (In reply to LRN from comment #3) > > > (In reply to Bastien Nocera from comment #2) > > > > Review of attachment 360713 [details] [review] [review] [review] [review] [review]: > > > > > > > > ::: gdk-pixbuf/io-tiff.c > > > > @@ +362,1 @@ > > > > if (!tiff) { > > > > > > > > "tiff" might be used unset here, if DuplicateHandle() fails. > > > > > > I don't follow. Please elaborate. > > > > Click on the review, and look at it in context. > > I know *that*. What i do not follow is how this can happen. Looking at the > code, the "tiff" variable is never left unset. Sorry, I read the patch incorrectly because of the missing braces. If we use braces in one branch of a conditional, we always use it for the other(s). And we always use braces for multi-line conditionals, and multi-line branches. So: if (foo_function (arg1, arg2)) { do_something(); } else { do_something_else(); } I'll fix it up before committing.
Created attachment 364622 [details] [review] io-tiff: Correctly work with WIN32-IO version of libtiff If libtiff is compiled with WIN32-IO (which is the default, unless it's built for Cygwin), it works with HANDLEs, not C runtime FDs.
Can you please test this? The changes are minimal, but I want to be sure it doesn't break anything.
The patch applies, the code compiles.
Attachment 364622 [details] pushed as ce52cef - io-tiff: Correctly work with WIN32-IO version of libtiff
libtiff casts those handles to int everywhere, so I guess this is still broken on 64bit?
uh, 2008 :( http://bugzilla.maptools.org/show_bug.cgi?id=1941
How exactly do you think casting handles to int breaks this on 64-bit Windowses?
HANDLE is 8 byte, int is 4
It looks like the unix backend, using FDs, got Windows support as well. Maybe we can depend on that instead? Another thing, when compiling with win32 the size of the thandle_t is different (see USE_WIN32_FILEIO) than what you see in the header (int vs void*), so all the API using thandle_t is broken on win64 as well, afaics.
ah, ignore the last part, thandle_t is a pointer in all cases.
HADNLE is 8 bytes, but only the lowest 4 bytes are significant (this is guaranteed by the OS), so as long as the cast is done correctly, it's fine. So the question is: is the cast done correctly? Can't say anything about the unix backend. IME, backends that use W32 API directly generally work better, as they do not make any unreasonable assumptions about C99-ishness and POSIX-ishness of the API they are using.
Ah, I didn't know that, it looks OK then. Sorry for the noise :( Just for future reference, I've rebuild libtiff with "--disable-win32-io" and without this patch, and things seem to work fine as well.