You need to log in before you can comment on or make changes to this bug.
There is a Win64 problem in tif_win32.c's TIFFOpen(). It calls CreateFile() which returns a variable of type HANDLE, which is a 64-bit value on Win64. It then casts this down to a 32-bit value that is passed into TIFFFdOpen(), and eventually into TIFFClientOpen() as client data (without the upper 32-bits). Then when the various platform specific routines are called (ie. _tiffReadProc, _tiffWriteProc, etc), it is passed the truncated 32-bit value as the HANDLE to use. This patch was created as a result of the discussions from this thread, http://www.asmail.be/msg0054557922.html which details some other failed attempts at fixing this. The current approach is to change the client data to be a real "fd" (ie. that returned by the C library's open()) instead of a HANDLE. After creating the HANDLE, we create an entry in the C runtime library's table of fd to HANDLE mappings via _open_osfhandle(), giving us an fd that we then pass to TIFFFdOpen() to be used as client data. Thereafter, the C runtime library has ownership of the HANDLE. In our callback functions, we convert the fd back into a HANDLE by looking up the fd to HANDLE table via _get_osfhandle(). The pro of this approach is that other than fixing the bug described, the functions TIFFFdOpen(), TIFFFileno(), and TIFFSetFileno() now all have the same semantics on Windows as it is on Unix. This in turn simplifies the client code as evidenced in the fax2ps.c and fax2tiff.c changes in the patch. The con of this approach is that it fundementally changes the compatibility with Windows client applications that expect the client data (ie. those using TIFFClientdata(), TIFFSetClientdata(), TIFFSetFileno(), TIFFFileno()) to be an HANDLE instead of an fd. The client application will need to convert between HANDLEs and fds as appropriate when using these functions. I've done code path testing of the patch using a small RGBA image. For large (3 GB) image testing, I scaled the small image into a large image, verified it's tiffdump and tiffinfo output, scaled it back down to a small file, and then examined that the data is still correct.
I am rather nervous about this approach, and I wonder if an interim step might be to distribute a tif_win64.c using this approach. Folks building on win64 could use that instead of tif_win32.c.
In order to work by default with existing build systems (outside of libtiff) and decrease clutter, it is useful if Windows "native" I/O is handled by just one source module. The main concern I have with the approach is that someone needs to test to verify that the APIs used do work on whatever the oldest version of Windows (or perhaps the oldest MSVC runtime) that libtiff desires to support. For example, are these functions provided (and work) with Visual C++ 6.0? If things work with Visual C++ 6.0, then I think we are ok.
AFAICT, the following links indicate that _get_osfhandle() and _open_osfhandle() exist in Visual Studio C++ 6.0, http://msdn.microsoft.com/en-us/library/aa297958(VS.60).aspx http://msdn.microsoft.com/en-us/library/aa298518(VS.60).aspx Of course, I don't have a Visual Studio C++ 6.0 compiler to test these changes. :) I also don't think it matters even if _get_osfhandle() returns a long in VC6 since it can't compile 64-bit programs. Is libtiff 4.0 expected to be source code compatible? I guess this is a yes which is why we're so concerned? If that is the case, then I think a tif_win64.c type of approach will be needed. People moving to the new Win64 semantics will compile with tif_win64.c instead of tif_win32.c. If we go this route, then we have some choices: 1. Move these changes to a new tif_win64.c and use tif_win32.c by default for source code compatibility. This means we then need to add yet another define so that the tools can build with either tif_win32.c or tif_win64.c. 2. Compile with tif_win64.c by default, leaving only tif_win32.c for source code compatibility. In this case, we forego adding another define since someone building the tools can just use tif_win64.c. They will only care about building their own libtiff using tif_win32.c for their client application. Note that I'm betting that it's fairly rare right now that an application will be built on Win64 but NOT on Win32 as well with the current Vista x64 driver situation (ie. forcing lots of people to stay on Win32 while they can). Having spend this much work on it, I guess one should ask whether we care about fixing this. :) In practice, HANDLE values only use the lower 32-bits most of the time. However, I think that this is the *right* change and there's no really good transition plan for people if we agree this is something that should be fixed for correctness. If we're going to do this change, then there's no point in postponing it to a later libtiff version, say 5.0. Since fd's are generally smaller values than HANDLEs, it might good enough to just bite the bullet and let client apps crash if they didn't notice the source code incompatibility. Then they can choose to switch to the new semantics or use the old tif_win32.c if they care about compiling against older versions of libtiff. I guess I'm advocating for choice #2.
PS. Regarding Bob's comment about the naming of tif_win32 vs tif_win64, we could choose to keep the old tif_win32.c as tif_win32_old.c around while updating tif_win32.c to the new semantics.
Here's another idea. In tiffio.h where we used to do all that stuff with the USE_WIN32_FILEIO macro, we do something this instead: #if defined(USE_WIN32_FILEIO) #error USE_WIN32_FILEIO is no longer supported, TIFFFdOpen() now uses file descriptors instead of HANDLEs. #endif We can also add more documentation about the change in various places as well.
From the mailing list today: On 4/12/2011 1:05 PM, Lee Howard wrote: >> 1. http://bugzilla.maptools.org/show_bug.cgi?id=1941 > > In reviewing that bug report there is nothing concluded for committal. > If an agreeable patch appears there it would be committed to CVS. It's agreeable by me. :) Some of the objections were/are: 1. It changes API semantics for tif_win32.c. I think this fear is unfounded given than we've defaulted to tif_unix.c for at least years now (CVS change was made in Mar 2005). Changing the default to tif_win32.c without the patch is more dangerous to me now because a lot of client apps have been building with tif_unix.c all along. 2. It was feared that older build systems like Visual C++ 6.0 might not work. I noted that this issue should not be a problem in theory because of the MSDN documentation. This point is probably now moot anyhow because people have enough trouble even finding MSVC 2005 these days. 3. In terms of source code compatibility, I offered an approach that should be sufficient to alert library users when compiling libtiff 4 by issuing a #error when USE_WIN32_FILEIO is defined. This is already a lot better than the change to TIFFFindFieldInfo() which silently does nothing. Best Regards, -Edward
Year 2013. Still no accepted solution to the bug. Do something, please.
Strange, what happened to the patch? I don't seem to see it attached anymore?
Does anyone have the patch?
Sorry, the best I've found is here: http://www.asmail.be/msg0054685155.html However, I no longer recall if there were any local improvements to the patch anymore.
Sorry, the corrected link with the patch here: http://www.asmail.be/msg0054557922.html
To finally get this working under 32-bit and 64-bit automatically, my suggestion is to use 'thandle_t' instead of 'int' everywhere: - tiffiop.h: struct tiff { ... thandle_t tif_fd; [old: int tif_fd;] - tiffio.h: extern thandle_t TIFFFileno (TIFF*); [old: extern int TIFFFileno (TIFF*);] extern thandle_t TIFFSetFileno (TIFF*, thandle_t); [old: extern int TIFFSetFileno (TIFF*, int );] extern TIFF* TIFFFdOpen (thandle_t, const char*, const char*); [old: extern TIFF* TIFFFdOpen (int, const char*, const char*); - tif_open.c: Fix TIFFFileno and TIFFSetFileno accordingly, remove all explicit conversions. - tif_win32.c: Fix TIFFFdOpen and TIFFOpen and TiffOpenW accordingly, remove all explicit conversions. Thanks!
I'd like to return to this but from a rather different direction: I don't think there is any real problem here at all. HANDLEs use 32 bits even under Win64, as documented in https://docs.microsoft.com/en-us/windows/desktop/WinProg64/interprocess-communication for example, which I'll quote here just in case the URL changes: 64-bit versions of Windows use 32-bit handles for interoperability. When sharing a handle between 32-bit and 64-bit applications, only the lower 32 bits are significant, so it is safe to truncate the handle (when passing it from 64-bit to 32-bit) or sign-extend the handle (when passing it from 32-bit to 64-bit). Handles that can be shared include handles to user objects such as windows (HWND), handles to GDI objects such as pens and brushes (HBRUSH and HPEN), and handles to named objects such as mutexes, semaphores, and file handles. And this really makes sense. HANDLEs are system-wide and there can be both 32 and 64 bit applications running on the same system and it would be pretty disastrous if they used incompatible HANDLEs. But before you just close this bug, let me say that there is still a small issue here: that of warnings generated by at least recent versions of gcc, which give one -Wint-to-pointer-cast and two -Wpointer-to-int-cast warnings in src/tiff/libtiff/tif_win32.c and I'd like to fix those because they're annoying, even if harmless. My "fix" would just suppress the warnings, however, and not really fix at all -- but this is fine because there is nothing to fix, see above. If there are no objections to this, I can make a PR on GitLab suppressing the warnings. Would there be any interest in this?
That sounds good to me as long as we carefully comment the heck out of it! In the intervening years, I've learned that these warnings can be overcome by explicitly adding intptr_t casts for Visual C++.
I think that adding an explicit mask of the value so the passed value can only contain the lower 32-bits, along with some suitable casts to eliminate warnings should be sufficient. A comment describing why this approach is sane would be useful.
Bugzilla is no longer used for tracking libtiff issues. Remaining open tickets, such as this one, have been migrated to the libtiff GitLab instance at https://gitlab.com/libtiff/libtiff/issues . The migrated tickets have their summary prefixed with [BZ#XXXX] where XXXX is the initial Bugzilla issue number.
