one little ill-advised change in v4.0.7 broke functionality for HylaFAX

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

one little ill-advised change in v4.0.7 broke functionality for HylaFAX

Lee Howard-2
See:

http://bugzilla.maptools.org/show_bug.cgi?id=2636

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

So, for a few weeks now a couple of folks on the HylaFAX mailing lists have
complained that HylaFAX does not work for them after they upgrade to libtiff
v4.0.7.  After looking into it I have found the culprit...

*******************************************************************

Index: ChangeLog
===================================================================
RCS file: /cvs/maptools/cvsroot/libtiff/ChangeLog,v
retrieving revision 1.1109
retrieving revision 1.1110
diff -u -r1.1109 -r1.1110
--- ChangeLog   3 Jan 2016 10:01:25 -0000    1.1109
+++ ChangeLog   9 Jan 2016 22:19:21 -0000    1.1110
@@ -1,3 +1,8 @@
+2016-01-09  Even Rouault <even.rouault at spatialys.com>
+
+    * libtiff/tif_fax3.h: make Param member of TIFFFaxTabEnt structure
+    a uint16 to reduce size of the binary.
+
  2016-01-03  Even Rouault <even.rouault at spatialys.com>

         * libtiff/tif_read.c, tif_dirread.c: fix indentation issues raised
Index: libtiff/tif_fax3.h
===================================================================
RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_fax3.h,v
retrieving revision 1.9
retrieving revision 1.10
diff -u -r1.9 -r1.10
--- libtiff/tif_fax3.h  10 Mar 2011 20:23:07 -0000    1.9
+++ libtiff/tif_fax3.h  9 Jan 2016 22:19:21 -0000    1.10
@@ -1,4 +1,4 @@
-/* $Id: tif_fax3.h,v 1.9 2011-03-10 20:23:07 fwarmerdam Exp $ */
+/* $Id: tif_fax3.h,v 1.10 2016-01-09 22:19:21 erouault Exp $ */

  /*
   * Copyright (c) 1990-1997 Sam Leffler
@@ -84,7 +84,7 @@
  typedef struct {                /* state table entry */
         unsigned char State;    /* see above */
         unsigned char Width;    /* width of code in bits */
-    uint32 Param;           /* unsigned 32-bit run length in bits */
+    uint16 Param;           /* unsigned 16-bit run length in bits */
  } TIFFFaxTabEnt;

  extern const TIFFFaxTabEnt TIFFFaxMainTable[];

*******************************************************************

I cannot see this change to reduce the size of the binary being discussed at
all on the libtiff mailing list... and unfortunately I did not test libtiff CVS
code in HylaFAX this year until now.

Anyway, the "Param" field here in TIFFFaxTabEnt corresponds to
TIFFTAG_FAXRECVPARAMS which elsewhere in libtiff (like libtiff/tif_dirinfo.c)
is known to be 32-bits long.

The consequence of this change is that any software that utilizes
TIFFTAG_FAXRECVPARAMS (and HylaFAX does so quite heavily) will likely see their
"params" data corrupted/lost... leading to all sorts of problems with image
data operations.

Please reverse the attached patch.


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

tiff-broke407.patch (492 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: one little ill-advised change in v4.0.7 broke functionality for HylaFAX

Even Rouault-2

Lee,

 

sorry for the inconvenience. I'm all for reverting if that broke things of course, but even with your explanations and looking at the code, I don't understand how it may affect external software.

 

My grep'ing of the code shows that the TIFFFaxTabEnt structure and the 3 tables TIFFFaxMainTable, TIFFFaxWhiteTable and TIFFFaxBlackTable are only used by tif_fax3.h, tif_fax3.c & tif_fax3sm.c. And I don't see any connection at all with TIFFTAG_FAXRECVPARAMS, which has no specific (AFAICS) processing in libtiff.

 

Am I missing something or is it HylaFAX that uses libtiff internals in some ways ?

 

Even

 

 

On lundi 12 d├ęcembre 2016 20:56:49 CET Lee Howard wrote:

> See:

>

> http://bugzilla.maptools.org/show_bug.cgi?id=2636

>

> ---------------------------------------------------------

>

> So, for a few weeks now a couple of folks on the HylaFAX mailing lists have

> complained that HylaFAX does not work for them after they upgrade to libtiff

> v4.0.7. After looking into it I have found the culprit...

>

> *******************************************************************

>

> Index: ChangeLog

> ===================================================================

> RCS file: /cvs/maptools/cvsroot/libtiff/ChangeLog,v

> retrieving revision 1.1109

> retrieving revision 1.1110

> diff -u -r1.1109 -r1.1110

> --- ChangeLog 3 Jan 2016 10:01:25 -0000 1.1109

> +++ ChangeLog 9 Jan 2016 22:19:21 -0000 1.1110

> @@ -1,3 +1,8 @@

> +2016-01-09 Even Rouault <even.rouault at spatialys.com>

> +

> + * libtiff/tif_fax3.h: make Param member of TIFFFaxTabEnt structure

> + a uint16 to reduce size of the binary.

> +

> 2016-01-03 Even Rouault <even.rouault at spatialys.com>

>

> * libtiff/tif_read.c, tif_dirread.c: fix indentation issues raised

> Index: libtiff/tif_fax3.h

> ===================================================================

> RCS file: /cvs/maptools/cvsroot/libtiff/libtiff/tif_fax3.h,v

> retrieving revision 1.9

> retrieving revision 1.10

> diff -u -r1.9 -r1.10

> --- libtiff/tif_fax3.h 10 Mar 2011 20:23:07 -0000 1.9

> +++ libtiff/tif_fax3.h 9 Jan 2016 22:19:21 -0000 1.10

> @@ -1,4 +1,4 @@

> -/* $Id: tif_fax3.h,v 1.9 2011-03-10 20:23:07 fwarmerdam Exp $ */

> +/* $Id: tif_fax3.h,v 1.10 2016-01-09 22:19:21 erouault Exp $ */

>

> /*

> * Copyright (c) 1990-1997 Sam Leffler

> @@ -84,7 +84,7 @@

> typedef struct { /* state table entry */

> unsigned char State; /* see above */

> unsigned char Width; /* width of code in bits */

> - uint32 Param; /* unsigned 32-bit run length in bits */

> + uint16 Param; /* unsigned 16-bit run length in bits */

> } TIFFFaxTabEnt;

>

> extern const TIFFFaxTabEnt TIFFFaxMainTable[];

>

> *******************************************************************

>

> I cannot see this change to reduce the size of the binary being discussed at

> all on the libtiff mailing list... and unfortunately I did not test libtiff

> CVS code in HylaFAX this year until now.

>

> Anyway, the "Param" field here in TIFFFaxTabEnt corresponds to

> TIFFTAG_FAXRECVPARAMS which elsewhere in libtiff (like

> libtiff/tif_dirinfo.c) is known to be 32-bits long.

>

> The consequence of this change is that any software that utilizes

> TIFFTAG_FAXRECVPARAMS (and HylaFAX does so quite heavily) will likely see

> their "params" data corrupted/lost... leading to all sorts of problems with

> image data operations.

>

> Please reverse the attached patch.

 

 

--

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: one little ill-advised change in v4.0.7 broke functionality for HylaFAX

Lee Howard-2
On 12/13/2016 12:54 AM, Even Rouault wrote:

Lee,

 

sorry for the inconvenience. I'm all for reverting if that broke things of course, but even with your explanations and looking at the code, I don't understand how it may affect external software.

 

My grep'ing of the code shows that the TIFFFaxTabEnt structure and the 3 tables TIFFFaxMainTable, TIFFFaxWhiteTable and TIFFFaxBlackTable are only used by tif_fax3.h, tif_fax3.c & tif_fax3sm.c. And I don't see any connection at all with TIFFTAG_FAXRECVPARAMS, which has no specific (AFAICS) processing in libtiff.

 

Am I missing something or is it HylaFAX that uses libtiff internals in some ways ?


Those tables are exported.

# nm -D /usr/lib64/libtiff.so | grep TIFFFax
000000000001b550 T _TIFFFax3fillruns
0000000000049740 R TIFFFaxBlackCodes
0000000000049f00 R TIFFFaxBlackTable
0000000000061f00 R TIFFFaxMainTable
00000000000499e0 R TIFFFaxWhiteCodes
0000000000059f00 R TIFFFaxWhiteTable

HylaFAX has its own set of those tables...

# nm -D /usr/lib64/libfaxserver.so.5.5.9 | grep TIFFFax
                 U _TIFFFax3fillruns
                 U TIFFFaxBlackCodes
                 U TIFFFaxBlackTable
                 U TIFFFaxMainTable
                 U TIFFFaxWhiteCodes
                 U TIFFFaxWhiteTable

I'm not yet sure why HylaFAX is preferring to use libtiff's tables over its own, but the fact remains that the tables exported by libtiff (is this considered ABI?) changed due to the referenced code change... and such a change really should not be done lightly because of the consequences to other software that utilize those exported tables.  Yes, TIFFTAG_FAXRECVPARAMS is really just a HylaFAX thing... but as HylaFAX and libtiff were born of the same cloth by the same author - the correlation between TIFFTAG_FAXRECVPARAMS and the "Param" field in these tables is not imagined.

Thanks,

Lee.


_______________________________________________
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: one little ill-advised change in v4.0.7 broke functionality for HylaFAX

Even Rouault-2

On mardi 13 d├ęcembre 2016 09:40:14 CET Lee Howard wrote:

> On 12/13/2016 12:54 AM, Even Rouault wrote:

> > Lee,

> >

> > sorry for the inconvenience. I'm all for reverting if that broke

> > things of course, but even with your explanations and looking at the

> > code, I don't understand how it may affect external software.

> >

> > My grep'ing of the code shows that the TIFFFaxTabEnt structure and the

> > 3 tables TIFFFaxMainTable, TIFFFaxWhiteTable and TIFFFaxBlackTable are

> > only used by tif_fax3.h, tif_fax3.c & tif_fax3sm.c. And I don't see

> > any connection at all with TIFFTAG_FAXRECVPARAMS, which has no

> > specific (AFAICS) processing in libtiff.

> >

> > Am I missing something or is it HylaFAX that uses libtiff internals in

> > some ways ?

>

> Those tables are exported.

>

> # nm -D /usr/lib64/libtiff.so | grep TIFFFax

> 000000000001b550 T _TIFFFax3fillruns

> 0000000000049740 R TIFFFaxBlackCodes

> 0000000000049f00 R TIFFFaxBlackTable

> 0000000000061f00 R TIFFFaxMainTable

> 00000000000499e0 R TIFFFaxWhiteCodes

> 0000000000059f00 R TIFFFaxWhiteTable

>

> HylaFAX has its own set of those tables...

>

> # nm -D /usr/lib64/libfaxserver.so.5.5.9 | grep TIFFFax

> U _TIFFFax3fillruns

> U TIFFFaxBlackCodes

> U TIFFFaxBlackTable

> U TIFFFaxMainTable

> U TIFFFaxWhiteCodes

> U TIFFFaxWhiteTable

>

> I'm not yet sure why HylaFAX is preferring to use libtiff's tables over

> its own, but the fact remains that the tables exported by libtiff (is

> this considered ABI?) changed due to the referenced code change...

 

There was no hint that it was supposed to be part of the ABI (the "extern" declaration seemed only there because the tables are defined in tif_fax3sm.c and used in tif_fax3.c). Ideally HylaFAX should not rely on that, or there should be a cleaner way in libtiff to export the tables (a get function) and the structure should be defined in a public header of libtiff.

 

> and

> such a change really should not be done lightly because of the

> consequences to other software that utilize those exported tables.

 

Hum "I see" (not completely to be honest). Looks like in the above Hylafax imports libtiff tables (the U must be undefined, hence imported). OK so it must also have its redefinition of typedef struct TIFFFaxTabEnt in its code, since tif_fax3.h is not installed (or it has a copy of tif_fax3.h) ?

 

OK, I'll revert that with a prominent warning so that the next one that touches this code is aware of those tricky aspects that cannot be anticipated from libtiff code itself.

 

Even

 

 

--

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: one little ill-advised change in v4.0.7 broke functionality for HylaFAX

Lee Howard-2
On 12/13/2016 10:06 AM, Even Rouault wrote:
Ideally HylaFAX should not rely on that, or there should be a cleaner way in libtiff to export the tables (a get function) and the structure should be defined in a public header of libtiff.

You'll have no argument from me that HylaFAX shouldn't be doing things differently from the way that it is.  All I'm trying to say is that a dependency existed for a long, long time now originating with the author of both software packages.

Looks like in the above Hylafax imports libtiff tables (the U must be undefined, hence imported). OK so it must also have its redefinition of typedef struct TIFFFaxTabEnt in its code, since tif_fax3.h is not installed (or it has a copy of tif_fax3.h) ?


It has its own copy of tif_fax3.h (which includes TIFFFaxTabEnt)...

https://sourceforge.net/p/hylafax/HylaFAX+/HEAD/tree/trunk/faxd/tif_fax3.h

Again, I have no doubt whatsoever that this could all be done cleaner/better.  I'm just saying that it is what it is.

OK, I'll revert that with a prominent warning so that the next one that touches this code is aware of those tricky aspects that cannot be anticipated from libtiff code itself.


Thanks.  If you want to provide a code change for HylaFAX then I'm happy to consider it.

Thanks,

Lee.

_______________________________________________
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: one little ill-advised change in v4.0.7 broke functionality for HylaFAX

mickey.rose
Od: Lee Howard <[hidden email]>

Datum: 13. 12. 2016 19:55:04


It has its own copy of tif_fax3.h (which includes TIFFFaxTabEnt)...

https://sourceforge.net/p/hylafax/HylaFAX+/HEAD/tree/trunk/faxd/tif_fax3.h

Again, I have no doubt whatsoever that this could all be done cleaner/better.  I'm just saying that it is what it is.

OK, I'll revert that with a prominent warning so that the next one that touches this code is aware of those tricky aspects that cannot be anticipated from libtiff code itself.


Thanks.  If you want to provide a code change for HylaFAX then I'm happy to consider it.


In my opinion that'd be better, as at this point affected users will need to upgrade *something*. Given that hylafax includes a "fork" of tif_fax3.h, it should also include a fork of tif_fax3sm.c, not import the tables (which are arguably leaked implementation detail) from libtiff.



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