Global variables in LibTIFF

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Global variables in LibTIFF

Paavo Helde

I upgraded LibTIFF to current last stable version 4.0.6 and saw it still
relies on global variables. In particular I am concerned about error and
warning handlers. From tif_error.c :

TIFFErrorHandlerExt _TIFFerrorHandlerExt = NULL;

TIFFErrorHandlerExt
TIFFSetErrorHandlerExt(TIFFErrorHandlerExt handler)
{
     TIFFErrorHandlerExt prev = _TIFFerrorHandlerExt;
     _TIFFerrorHandlerExt = handler;
     return (prev);
}

In my application which is using LibTIFF directly there are also several
third-party libraries which are using LibTIFF internally (gdcm, ITK,
libopenjpeg, wxWidgets, etc). Depending on the type of linking
(static/shared) and the platform-dependent visibility rules of the
symbols the LibTIFF library together with the global static variable may
become shared by several libraries. This means that an error handler for
a wrong library may get called. A wrong error handler would most
probably crash when it tries to interpret the tif->tif_clientdata pointer.

In principle problems with accessing tif->tif_clientdata could be
circumvented to some extent by using the older error handler
(TIFFSetErrorHandler()) and thread-specific data, but the error could
still end up in a wrong place. If all error handlers would take care of
calling the previously installed error handler, then it would at least
be forwarded to the correct place, but this depends on the client code
and for example is not done in wxWidgets.

Even if the client code would always take care to call the previous
error handler, this still would not be a 100% reliable solution because
TIFFSetErrorHandler() is not multithread-safe and if called from
multiple threads in parallel, it may return wrong values.

These problems are all caused by using global variables. Why not just
add error and warning handler function pointers to the tiff struct,
similar to tif_seekproc et al? The same goes for other global variables
(_TIFFextender, ...?)

Sharing of libraries is in principle a good thing and stipulated by open
source communities, but currently it seems LibTIFF is working against
this goal. I see for example that ITK has taken the trouble of mangling
all LibTIFF symbols by some macro chemistry, probably for working around
such issues.

Cheers
Paavo

_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Bob Friesenhahn
On Fri, 11 Nov 2016, Paavo Helde wrote:

>
> I upgraded LibTIFF to current last stable version 4.0.6 and saw it still
> relies on global variables. In particular I am concerned about error and
> warning handlers. From tif_error.c :

The error/warning handlers are a known issue.  Errors and warnings may
occur when a TIFF is opened so the most appropriate solution would
have been to allow the user to specify them as arguments to
TIFFClientOpen(), TIFFOpen(), and other opening functions.  This
should have been done for the 4.0 release but the older interfaces
would still have needed to remain in order to avoid needing to change
existing valid source code.

It is likely that quite a lot of small changes would need to be made
to libtiff in order to allow error/warning arguments to pass to the
code which needs it.

Linking both shared and static libtiff to an application is a major
no-no regardless of this issue.  If you can't be sure of what storage
will be used then you also can't be sure of which function
implementations will be used.

Bob
--
Bob Friesenhahn
[hidden email], http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,    http://www.GraphicsMagick.org/
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Olivier Paquet-2
In reply to this post by Paavo Helde
2016-11-11 9:03 GMT-05:00 Paavo Helde <[hidden email]>:
I upgraded LibTIFF to current last stable version 4.0.6 and saw it still
relies on global variables. In particular I am concerned about error and
warning handlers. From tif_error.c :

We've had this discussion a few months ago: http://www.asmail.be/msg0055058625.html  I'm all for putting the error handler in the TIFF struct. It doesn't fix everything but I think it would be a major step forward. There remains the problem of finding time to actually do it.

Olivier

_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Even Rouault-2
Le vendredi 11 novembre 2016 15:47:03, Olivier Paquet a écrit :

> 2016-11-11 9:03 GMT-05:00 Paavo Helde <[hidden email]>:
> > I upgraded LibTIFF to current last stable version 4.0.6 and saw it still
> > relies on global variables. In particular I am concerned about error and
>
> > warning handlers. From tif_error.c :
> We've had this discussion a few months ago:
> http://www.asmail.be/msg0055058625.html  I'm all for putting the error
> handler in the TIFF struct. It doesn't fix everything but I think it would
> be a major step forward. There remains the problem of finding time to
> actually do it.

Seems someone proposed a related patch 6 years ago... :
http://bugzilla.maptools.org/show_bug.cgi?id=2255

--
Spatialys - Geospatial professional services
http://www.spatialys.com
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Bob Friesenhahn
On Fri, 11 Nov 2016, Even Rouault wrote:
>
> Seems someone proposed a related patch 6 years ago... :
> http://bugzilla.maptools.org/show_bug.cgi?id=2255

This patch has good ideas (and covers more than the error/warning
handlers) but it is against libtiff 3.9.4.  Libtiff's error
reporting/handling was updated for 4.X.

Bob
--
Bob Friesenhahn
[hidden email], http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,    http://www.GraphicsMagick.org/
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Yakov Galka
Hey all,

Following our discussion of this issue a few month ago I actually went implementing it. You can look at the patches based on my current code on the following links. I've split it into two patches for easier review: one of the manual edits and one of the autoreplace of the TIFFError/Warning calls:

http://stannum.co.il/data/tiff-changes.diff
http://stannum.co.il/data/tiff-autorep.diff


I don't remember the exact state of them at the moment, but here is what I can remember off the top of my head:

* I tried to solve the problem of error/warning being global callbacks.

* While doing this I encountered other bad-designed aspects of the library. So trying to make a uniform interface for all callbacks I modified a bit extra of the functionality.

* Yet I tried not to break backward compatibility with 'typical' code. Some code that assumes the interplay of clientdata, file descriptors and the read/write/seek callbacks might break.

* I tried to prevent combinatorial explosion by introducing a two step TIFF creation: first one allocates the structure, sets the relevant parameters, then issues the actual Open* call. That state separation actually happened to be useful in fax2tiff in the "FakeInput" TIFF.

* AFAIR I dug some old cases wrt. using Windows HANDLEs instead file descriptors and 64-bit issues with those, and started thinking of how to fix the library to correctly support those. This is where I stopped working on it.

* THESE AREN'T FINISHED MODIFICATIONS. Though at some point I ran it against the tests and everything passed.

* I stopped working on it because:
    * I thought that the redesign is too big so you might not be interested to merge it.
    * Despite all the efforts some backward compatibility wrt some corner cases will break anyways. Even though I estimate it to be less than 2% of the code out there.
    * Bug 2575 that I found during the refactoring was not addressed...
    * and at the same time Bob referred to libtiff as a 'dead horse' on this mailing list...
    * so...


You are welcome to look at it. If you need a detailed documentation of all re-design decisions and considerations then ask for it, I had a draft somewhere. If you think that this is the right direction then I can find the time to finish it, with your cooperation on weighting backward compatibility and new interface issues.


----------------------

Regarding the patch proposed in Bug 2255: I've looked at it at the time, but I actually consider it being NOT A SOLUTION:

* First it is not backward compatible at all. Making it backward compatible means one needs to duplicate ALL the interface (one with the TIFFApplication and one without).

* It does not solve the problem of retrieving a custom per-tiff userdata from the error handlers, which in my case (of an error-checked libtiff c++ wrapper at my work) does not help.

----------------------


I hope you find this useful.

Cheers,

_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Bob Friesenhahn
On Sat, 12 Nov 2016, Yakov Galka wrote:
>    * Bug 2575 that I found during the refactoring was not addressed...
>    * and at the same time Bob referred to libtiff as a 'dead horse' on
> this mailing list...

You must be referring to this statement by Edward Lam (not by me):

"Sometimes I feel like the only reason why I'm still on this mailing
list is to beat dead horses. :)"

We need to take care that libtiff API improvements don't break the ABI
since existing users are depending on libtiff releases (when they
occur) to fix libtiff security issues and bugs without needing to
rebuild the "world", and (particularly) without needing to patch
packages using libtiff so that they work again.

API improvements should be layered on top of, or adjacent to, the
existing API, acting as alternatives without breaking the existing
API/ABI.  Changes made for libtiff 4.X (e.g. hiding implementation
structures) make this possible whereas before it was clearly
impossible.

Libtiff is 28 years old already and we can not afford to upset
the substantial body of existing software.

Bob
--
Bob Friesenhahn
[hidden email], http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,    http://www.GraphicsMagick.org/
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Yakov Galka
On Sat, Nov 12, 2016 at 4:56 PM, Bob Friesenhahn <[hidden email]> wrote:
You must be referring to this statement by Edward Lam (not by me):

Oh, sorry about that, I don't really remember :)

 
We need to take care that libtiff API improvements don't break the ABI since existing users are depending on libtiff releases (when they occur) to fix libtiff security issues and bugs without needing to rebuild the "world", and (particularly) without needing to patch packages using libtiff so that they work again.

API improvements should be layered on top of, or adjacent to, the existing API, acting as alternatives without breaking the existing API/ABI.  Changes made for libtiff 4.X (e.g. hiding implementation structures) make this possible whereas before it was clearly impossible.

Libtiff is 28 years old already and we can not afford to upset the substantial body of existing software.

This is all obvious, and if you looked at my patches you've seen that I introduced the new functionality without breaking the current ABI. The problems I'm talking about is the (undocumented) API *behavior* wrt the interplay of some of the library features.

For example:

    int fd = ...;
    TIFF *tiff0 = TIFFOpen("0.tiff", "r");
    TIFFSetClientdata(tiff0, thandle_t(fd));
    TIFFRead(...); // <<<<<<<<<<<<<< This fails on today's libTIFF!

Another example:

    TIFF *tiff0 = TIFFOpen("0.tiff", "r");
    int fd = open("1.tiff", O_RDONLY);
    TIFF *tiff1 = TIFFClientOpen("1.tiff", "r", thandle_t(fd),
        TIFFGetReadProc(tiff0), TIFFGetWriteProc(tiff0),
        TIFFGetSeekProc(tiff0), TIFFGetCloseProc(tiff0),
        TIFFGetSizeProc(tiff0), TIFFGetMapFileProc(tiff0),
        TIFFGetUnmapFileProc(tiff0)
    ); // <<<<<<<<<<<<<<<<<<<<<<<<< Should this work?


Specifically, current libTIFF has the following problems (relevant to this discussion):

* The built-in i/o procedures use tif_clientdata for recovering the file-descriptor.
    * Users want to store arbitrary clientdata that will be passed to their callbacks.
    * Users want to use the error callbacks without reimplementing their own i/o functions.
    => We need to either free-up tif_clientdata for their exclusive use or to introduce tif_clientdataV2.
    ! Note that we already have tif_fd, though it's useless on Windows!

* i/o callbacks receive a thandle_t rather than the TIFF* structure.
    => Users implementing i/o callbacks cannot access the properties of the TIFF*, like mode or filename for the purpose of error reporting.

Both examples above will work without recompilation with my patch. However, now users will be able to use the new interface as follows:

    TIFF *tiff = TIFFCreateNamedContext("0.tiff");
    TIFFSetClientdata(tiff, my_error_log_pointer);
    TIFFSetSetErrorProcs(tiff, my_error_proc, my_warning_proc);
    _TIFFOpen(tiff, "0.tiff", "r"); // should be TIFFOpenV2

--

_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Paavo Helde
In reply to this post by Yakov Galka

On 12.11.2016 2:24, Yakov Galka wrote:
> Hey all,
>
> Following our discussion of this issue a few month ago I actually went
> implementing it. You can look at the patches based on my current code
> on the following links. I've split it into two patches for easier
> review: one of the manual edits and one of the autoreplace of the
> TIFFError/Warning calls:

>
> * I tried to prevent combinatorial explosion by introducing a two step
> TIFF creation: first one allocates the structure, sets the relevant
> parameters, then issues the actual Open* call. That state separation
> actually happened to be useful in fax2tiff in the "FakeInput" TIFF.

Two-step initialization seems very fine to me. At first you initialize
the library/engine, then use this initialized engine to actually
open/initialize TIFF files. These are two separate operations logically
and this should be exposed at the SDK level as well.

Of course, backwards compatibility with previous versions is a must.

just my 2 euro cents
Paavo



_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Gerben Vos-2
In reply to this post by Yakov Galka
Why is the context named? It smells like magic behavior at a distance (even though it doesn't seem to be) that the name of the context and the opened tiff are the same.

I did a thread-safety analysis a few years ago, but did not contact the mailing list about it. My implementation ideas for fixing those were more among the lines of those in the patch attached to http://bugzilla.maptools.org/show_bug.cgi?id=2255 , except that I thought of calling the context object with the formerly global variables a TIFFContext instead of a TIFFApplication. There would then be a default context (that would still be the, only, global variable) that would be used by code that didn’t create their own context. I don't have an implementation, though.

Here are the main notes from my thread-safety analysis at that time (September 2014; I think it was for libtiff 4.0.3):
* TIFFSetErrorHandler/TIFFSetErrorHandlerExt/TIFFError/TIFFErrorExt: TIFFErrorHandlerExt _TIFFerrorHandlerExt; TIFFErrorHandler _TIFFerrorHandler;
* TIFFSetWarningHandler/TIFFSetWarningHandlerExt/TIFFWarning/TIFFWarningExt: TIFFErrorHandlerExt _TIFFwarningHandlerExt; TIFFErrorHandler _TIFFwarningHandler;
* TIFFSetTagExtender/TIFFDefaultDirectory: static TIFFExtendProc _TIFFextender;
* TIFFRegisterCODEC/TIFFUnRegisterCODEC: static codec_t* registeredCODECS; (linked list)
* TIFFFindCODEC/TIFFIsCODECConfigured/TIFFGetConfiguredCODECs: static codec_t* registeredCODECS; (linked list)
* TIFFPrintDirectory: static uint16 dotrange[2];
* oog_encode: static int oog_table[NANGLES]; static int initialized = 0;
* PixarLogMakeTables/PixarLogEncode: static float Fltsize; static float LogK1, LogK2;

Note that there are also a few static variables whose initialization might be a problem in a multithreaded environment, although they always initialize the same data every time.

Gerben.

DISCLAIMER: This e-mail and any attachments are intended only for the individual or company to which it is addressed and may contain information which is privileged, confidential and prohibited from disclosure or unauthorized use under applicable law. If you are not the intended recipient of this e-mail, you are hereby notified, any use, dissemination, or copying of this e-mail or the information contained in this e-mail is strictly prohibited by the sender. If you have received this transmission in error, please return the material received to the sender and delete all copies from your system. This email has been scanned by the Email Security.cloud service.
_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Global variables in LibTIFF

Yakov Galka

On Wed, Nov 16, 2016 at 2:04 PM, Gerben Vos <[hidden email]> wrote:
Why is the context named? It smells like magic behavior at a distance (even though it doesn't seem to be) that the name of the context and the opened tiff are the same.

I don't understand your complaints. It is already in the libTIFF API that you can create a context with one name and use it to read a file of another name. E.g.:

    TIFFClientOpen("junk", ... functions which read from whatever ...);

... or:

    TIFFFdOpen(open("a.txt"), "b.txt", "r);
 
It is just a feature of the existing API that the TIFF structure has an associated name, and this name is used mostly for error reporting.

The term "filename" used currently by the library is actually misleading, because as with the case of TIFFClientOpen it might correspond to something that's not really an FS file.

It is also a feature of the existing implementation that this name is allocated as part of the TIFF structure (in one malloc call), so it has to be passed as part of TIFF creation rather than set later by a different API call.


I did a thread-safety analysis a few years ago, but did not contact the mailing list about it. My implementation ideas for fixing those were more among the lines of those in the patch attached to http://bugzilla.maptools.org/show_bug.cgi?id=2255 , except that I thought of calling the context object with the formerly global variables a TIFFContext instead of a TIFFApplication. There would then be a default context (that would still be the, only, global variable) that would be used by code that didn’t create their own context. I don't have an implementation, though.

Here are the main notes from my thread-safety analysis at that time (September 2014; I think it was for libtiff 4.0.3):
* TIFFSetErrorHandler/TIFFSetErrorHandlerExt/TIFFError/TIFFErrorExt: TIFFErrorHandlerExt _TIFFerrorHandlerExt; TIFFErrorHandler _TIFFerrorHandler;
* TIFFSetWarningHandler/TIFFSetWarningHandlerExt/TIFFWarning/TIFFWarningExt: TIFFErrorHandlerExt _TIFFwarningHandlerExt; TIFFErrorHandler _TIFFwarningHandler;
* TIFFSetTagExtender/TIFFDefaultDirectory: static TIFFExtendProc _TIFFextender;
* TIFFRegisterCODEC/TIFFUnRegisterCODEC: static codec_t* registeredCODECS; (linked list)
* TIFFFindCODEC/TIFFIsCODECConfigured/TIFFGetConfiguredCODECs: static codec_t* registeredCODECS; (linked list)
* TIFFPrintDirectory: static uint16 dotrange[2];
* oog_encode: static int oog_table[NANGLES]; static int initialized = 0;
* PixarLogMakeTables/PixarLogEncode: static float Fltsize; static float LogK1, LogK2;

I would add a future feature request: custom memory allocator.

Even though it is more logical to put CODECs, extenders, etc into a separate context that is shared by multiple TIFF structures, I believe that TIFF-specific callbacks should be part of the TIFF, or receive the TIFF* that caused the callback as one of their arguments.

My patch addresses only the Error/Warning part of these because it is the most important part for me at this point in time. (Currently we have an ugly TLS workaround here, which causes me mental pain every time I accidentally see it, especially in contrast with other libraries, like openjpeg, which handle it properly.)

--

_______________________________________________
Tiff mailing list: [hidden email]
http://lists.maptools.org/mailman/listinfo/tiff
http://www.remotesensing.org/libtiff/
Loading...